[PATCH 0/3] KVM: MMU: fix kvm_is_mmio_pfn()

[PATCH 0/3] KVM: MMU: fix kvm_is_mmio_pfn()

[Haozhong Zhang]

[I just copy the commit message from patch 3]

By default, KVM treats a reserved page as for MMIO purpose, and maps
it to guest with UC memory type.  However, some reserved pages are not
for MMIO, such as pages of DAX device (e.g., /dev/daxX.Y). Mapping
them with UC memory type will harm the performance.  In order to
exclude those cases, we check the host cache mode in addition and only
treat UC/UC- pages as MMIO.

Haozhong Zhang (3):
  x86/mm: expose track_pfn_insert()
  KVM: add converters between pfn_t and kvm_pfn_t
  KVM: MMU: consider host cache type in MMIO pfn check

 arch/x86/kvm/mmu.c       | 32 +++++++++++++++++++++++++++++---
 arch/x86/mm/pat.c        |  1 +
 include/linux/kvm_host.h |  3 +++
 3 files changed, 33 insertions(+), 3 deletions(-)

-- 
2.14.1

[PATCH 1/3] x86/mm: expose track_pfn_insert()

[Haozhong Zhang]

KVM MMU will use it to get the cache mode of the host pfn.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 arch/x86/mm/pat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index fe7d57a8fb60..cab593ea8956 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -998,6 +998,7 @@ void track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot, pfn_t pfn)
 	*prot = __pgprot((pgprot_val(*prot) & (~_PAGE_CACHE_MASK)) |
 			 cachemode2protval(pcm));
 }
+EXPORT_SYMBOL_GPL(track_pfn_insert);
 
 /*
  * untrack_pfn is called while unmapping a pfnmap for a region.
-- 
2.14.1

[PATCH 2/3] KVM: add converters between pfn_t and kvm_pfn_t

[Haozhong Zhang]

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 include/linux/kvm_host.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538eda32..759fe498c89e 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -67,6 +67,9 @@
 #define KVM_PFN_ERR_HWPOISON	(KVM_PFN_ERR_MASK + 1)
 #define KVM_PFN_ERR_RO_FAULT	(KVM_PFN_ERR_MASK + 2)
 
+#define kvm_pfn_to_pfn(x) ((pfn_t){ .val = (x)})
+#define pfn_to_kvm_pfn(x) ((kvm_pfn_t)((x).val))
+
 /*
  * error pfns indicate that the gfn is in slot but faild to
  * translate it to pfn on host.
-- 
2.14.1

[PATCH 3/3] KVM: MMU: consider host cache type in MMIO pfn check

[Haozhong Zhang]

By default, KVM treats a reserved page as for MMIO purpose, and maps
it to guest with UC memory type.  However, some reserved pages are not
for MMIO, such as pages of DAX device (e.g., /dev/daxX.Y). Mapping
them with UC memory type will harm the performance.  In order to
exclude those cases, we check the host cache mode in addition and only
treat UC/UC- pages as MMIO.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reported-by: Cuevas Escareno, Ivan D <ivan.d.cuevas.escareno@intel.com>
Reported-by: Kumar, Karthik <karthik.kumar@intel.com>
---
 arch/x86/kvm/mmu.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0b481cc9c725..d4c821a6df3d 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2707,10 +2707,36 @@ static bool mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 
 static bool kvm_is_mmio_pfn(kvm_pfn_t pfn)
 {
-	if (pfn_valid(pfn))
-		return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
+	bool is_mmio = true;
 
-	return true;
+	if (pfn_valid(pfn)) {
+		is_mmio = !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
+
+		/*
+		 * By default, KVM treats a reserved page as for MMIO
+		 * purpose, and maps it to guest with UC memory type.
+		 * However, some reserved pages are not for MMIO, such
+		 * as pages of DAX device (e.g., /dev/daxX.Y). Mapping
+		 * them with UC memory type will harm the performance.
+		 * In order to exclude those cases, we check the host
+		 * cache mode in addition and only treat UC/UC- pages
+		 * as MMIO.
+		 *
+		 * track_pfn_insert() works only when PAT is enabled,
+		 * so add pat_enabled() here.
+		 */
+		if (is_mmio && pat_enabled()) {
+			pgprot_t prot;
+			enum page_cache_mode cm;
+
+			track_pfn_insert(NULL, &prot, kvm_pfn_to_pfn(pfn));
+			cm = pgprot2cachemode(prot);
+			is_mmio = (cm == _PAGE_CACHE_MODE_UC ||
+				   cm == _PAGE_CACHE_MODE_UC_MINUS);
+		}
+	}
+
+	return is_mmio;
 }
 
 static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
-- 
2.14.1

Re: [PATCH 3/3] KVM: MMU: consider host cache type in MMIO pfn check

[Ingo Molnar]

* Haozhong Zhang <haozhong.zhang@intel.com> wrote:

> By default, KVM treats a reserved page as for MMIO purpose, and maps
> it to guest with UC memory type.  However, some reserved pages are not
> for MMIO, such as pages of DAX device (e.g., /dev/daxX.Y). Mapping
> them with UC memory type will harm the performance.  In order to
> exclude those cases, we check the host cache mode in addition and only
> treat UC/UC- pages as MMIO.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> Reported-by: Cuevas Escareno, Ivan D <ivan.d.cuevas.escareno@intel.com>
> Reported-by: Kumar, Karthik <karthik.kumar@intel.com>
> ---
>  arch/x86/kvm/mmu.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 0b481cc9c725..d4c821a6df3d 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2707,10 +2707,36 @@ static bool mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
>  
>  static bool kvm_is_mmio_pfn(kvm_pfn_t pfn)
>  {
> -	if (pfn_valid(pfn))
> -		return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
> +	bool is_mmio = true;
>  
> -	return true;
> +	if (pfn_valid(pfn)) {
> +		is_mmio = !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
> +
> +		/*
> +		 * By default, KVM treats a reserved page as for MMIO
> +		 * purpose, and maps it to guest with UC memory type.
> +		 * However, some reserved pages are not for MMIO, such
> +		 * as pages of DAX device (e.g., /dev/daxX.Y). Mapping
> +		 * them with UC memory type will harm the performance.
> +		 * In order to exclude those cases, we check the host
> +		 * cache mode in addition and only treat UC/UC- pages
> +		 * as MMIO.
> +		 *
> +		 * track_pfn_insert() works only when PAT is enabled,
> +		 * so add pat_enabled() here.
> +		 */
> +		if (is_mmio && pat_enabled()) {
> +			pgprot_t prot;
> +			enum page_cache_mode cm;
> +
> +			track_pfn_insert(NULL, &prot, kvm_pfn_to_pfn(pfn));
> +			cm = pgprot2cachemode(prot);
> +			is_mmio = (cm == _PAGE_CACHE_MODE_UC ||
> +				   cm == _PAGE_CACHE_MODE_UC_MINUS);
> +		}
> +	}
> +
> +	return is_mmio;
>  }
>  
>  static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,

s/harm the performance
 /harm performance> 

But I suspect the rest of the comment should be rewritten too to be more fluid.

Beyond that - I think instead of exposing these low level details a properly named 
helper function should be put into pat.c instead - and KVM can use that.

Thanks,

	Ingo

Re: [PATCH 3/3] KVM: MMU: consider host cache type in MMIO pfn check

[Haozhong Zhang]

On 10/27/17 10:40 +0200, Ingo Molnar wrote:
> 
> * Haozhong Zhang <haozhong.zhang@intel.com> wrote:
> 
> > By default, KVM treats a reserved page as for MMIO purpose, and maps
> > it to guest with UC memory type.  However, some reserved pages are not
> > for MMIO, such as pages of DAX device (e.g., /dev/daxX.Y). Mapping
> > them with UC memory type will harm the performance.  In order to
> > exclude those cases, we check the host cache mode in addition and only
> > treat UC/UC- pages as MMIO.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > Reported-by: Cuevas Escareno, Ivan D <ivan.d.cuevas.escareno@intel.com>
> > Reported-by: Kumar, Karthik <karthik.kumar@intel.com>
> > ---
> >  arch/x86/kvm/mmu.c | 32 +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 0b481cc9c725..d4c821a6df3d 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -2707,10 +2707,36 @@ static bool mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
> >  
> >  static bool kvm_is_mmio_pfn(kvm_pfn_t pfn)
> >  {
> > -	if (pfn_valid(pfn))
> > -		return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
> > +	bool is_mmio = true;
> >  
> > -	return true;
> > +	if (pfn_valid(pfn)) {
> > +		is_mmio = !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn));
> > +
> > +		/*
> > +		 * By default, KVM treats a reserved page as for MMIO
> > +		 * purpose, and maps it to guest with UC memory type.
> > +		 * However, some reserved pages are not for MMIO, such
> > +		 * as pages of DAX device (e.g., /dev/daxX.Y). Mapping
> > +		 * them with UC memory type will harm the performance.
> > +		 * In order to exclude those cases, we check the host
> > +		 * cache mode in addition and only treat UC/UC- pages
> > +		 * as MMIO.
> > +		 *
> > +		 * track_pfn_insert() works only when PAT is enabled,
> > +		 * so add pat_enabled() here.
> > +		 */
> > +		if (is_mmio && pat_enabled()) {
> > +			pgprot_t prot;
> > +			enum page_cache_mode cm;
> > +
> > +			track_pfn_insert(NULL, &prot, kvm_pfn_to_pfn(pfn));
> > +			cm = pgprot2cachemode(prot);
> > +			is_mmio = (cm == _PAGE_CACHE_MODE_UC ||
> > +				   cm == _PAGE_CACHE_MODE_UC_MINUS);
> > +		}
> > +	}
> > +
> > +	return is_mmio;
> >  }
> >  
> >  static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> 
> s/harm the performance
>  /harm performance> 
> 
> But I suspect the rest of the comment should be rewritten too to be more fluid.

I'll refactor the comment.

> 
> Beyond that - I think instead of exposing these low level details a properly named 
> helper function should be put into pat.c instead - and KVM can use that.
>

KVM only needs the memory type information, so lookup_memtype() in
pat.c is probably a better one to be exposed.

Haozhong

Re: [PATCH 0/3] KVM: MMU: fix kvm_is_mmio_pfn()

[Xiao Guangrong]

On 10/27/2017 10:25 AM, Haozhong Zhang wrote:
> [I just copy the commit message from patch 3]
> 
> By default, KVM treats a reserved page as for MMIO purpose, and maps
> it to guest with UC memory type.  However, some reserved pages are not
> for MMIO, such as pages of DAX device (e.g., /dev/daxX.Y). Mapping
> them with UC memory type will harm the performance.  In order to
> exclude those cases, we check the host cache mode in addition and only
> treat UC/UC- pages as MMIO.
> 

I am afraid that is not only a performance issue but also a architecture
bug - it could trigger MCE as there is alias memory mapping (a page mapped
as both WB and UC).

It may hurt mdev device as well as the device memory may be both mapped
at host and VM.