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

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

[Haozhong Zhang]

Some reserved pages, such as those from NVDIMM DAX devices, are not
for MMIO, and can be mapped with cached memory type for better
performance. However, the above check misconceives those pages as
MMIO.  Because KVM maps MMIO pages with UC memory type, the
performance of guest accesses to those pages would be harmed.
Therefore, we check the host memory type in addition and only treat
UC/UC- pages as MMIO.

Changes in v5:
 * Rename pat_pfn_is_uc() into pat_pfn_is_uc_or_uc_minus() to avoid
   confusion.
 * Drop converters between kvm_pfn_t and pfn_t, because they are not
   necessary. pat_pfn_is_uc_or_uc_minus() does not need flags in
   pfn_t, so we can only pass a raw unsigned long to it.

Changes in v4:
 * Mask pfn_t and kvm_pfn_t specific flags in conversion.

Changes in v3:
 * Move cache mode check to pat.c as pat_pfn_is_uc()
 * Reintroduce converters between kvm_pfn_t and pfn_t.

Changes in v2:
 * Switch to lookup_memtype() to get host memory type.
 * Rewrite the comment in KVM MMU patch.
 * Remove v1 patch 2, which is not necessary in v2.


Haozhong Zhang (2):
  x86/mm: add functions to check if a pfn is UC/UC-
  KVM: MMU: consider host cache mode in MMIO page check

 arch/x86/include/asm/pat.h |  2 ++
 arch/x86/kvm/mmu.c         | 15 ++++++++++++++-
 arch/x86/mm/pat.c          | 16 ++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.14.1

[PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Haozhong Zhang]

It will be used by KVM to check whether a pfn should be
mapped to guest as UC.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 arch/x86/include/asm/pat.h |  2 ++
 arch/x86/mm/pat.c          | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index fffb2794dd89..fabb0cf00e77 100644
--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -21,4 +21,6 @@ int io_reserve_memtype(resource_size_t start, resource_size_t end,
 
 void io_free_memtype(resource_size_t start, resource_size_t end);
 
+bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn);
+
 #endif /* _ASM_X86_PAT_H */
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index fe7d57a8fb60..e1282dd4eeb8 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -677,6 +677,22 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
 	return rettype;
 }
 
+/**
+ * Check with PAT whether the memory type of a pfn is UC or UC-.
+ *
+ * Only to be called when PAT is enabled.
+ *
+ * Returns true, if the memory type of @pfn is UC or UC-.
+ * Otherwise, returns false.
+ */
+bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
+{
+	enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
+
+	return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
+}
+EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
+
 /**
  * io_reserve_memtype - Request a memory type mapping for a region of memory
  * @start: start (physical address) of the region
-- 
2.14.1

[PATCH v5 2/2] KVM: MMU: consider host cache mode in MMIO page check

[Haozhong Zhang]

Some reserved pages, such as those from NVDIMM DAX devices, are not
for MMIO, and can be mapped with cached memory type for better
performance. However, the above check misconceives those pages as
MMIO.  Because KVM maps MMIO pages with UC memory type, the
performance of guest accesses to those pages would be harmed.
Therefore, we check the host memory type 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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0b481cc9c725..7715476bc5c9 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2708,7 +2708,20 @@ 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));
+		return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)) &&
+			/*
+			 * Some reserved pages, such as those from
+			 * NVDIMM DAX devices, are not for MMIO, and
+			 * can be mapped with cached memory type for
+			 * better performance. However, the above
+			 * check misconceives those pages as MMIO.
+			 * Because KVM maps MMIO pages with UC memory
+			 * type, the performance of guest accesses to
+			 * those pages would be harmed. Therefore, we
+			 * check the host memory type in addition and
+			 * only treat UC/UC- pages as MMIO.
+			 */
+			(!pat_enabled() || pat_pfn_is_uc_or_uc_minus(pfn));
 
 	return true;
 }
-- 
2.14.1

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

[Xiao Guangrong]

On 11/08/2017 03:56 PM, Haozhong Zhang wrote:
> Some reserved pages, such as those from NVDIMM DAX devices, are not
> for MMIO, and can be mapped with cached memory type for better
> performance. However, the above check misconceives those pages as
> MMIO.  Because KVM maps MMIO pages with UC memory type, the
> performance of guest accesses to those pages would be harmed.
> Therefore, we check the host memory type in addition and only treat
> UC/UC- pages as MMIO.
> 

Reviewed-by: Xiao Guangrong <xiaoguangrong@tencent.com>

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[David Hildenbrand]

On 08.11.2017 08:56, Haozhong Zhang wrote:
> It will be used by KVM to check whether a pfn should be
> mapped to guest as UC.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  arch/x86/include/asm/pat.h |  2 ++
>  arch/x86/mm/pat.c          | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> index fffb2794dd89..fabb0cf00e77 100644
> --- a/arch/x86/include/asm/pat.h
> +++ b/arch/x86/include/asm/pat.h
> @@ -21,4 +21,6 @@ int io_reserve_memtype(resource_size_t start, resource_size_t end,
>  
>  void io_free_memtype(resource_size_t start, resource_size_t end);
>  
> +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn);
> +
>  #endif /* _ASM_X86_PAT_H */
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index fe7d57a8fb60..e1282dd4eeb8 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -677,6 +677,22 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
>  	return rettype;
>  }
>  
> +/**
> + * Check with PAT whether the memory type of a pfn is UC or UC-.
> + *
> + * Only to be called when PAT is enabled.
> + *
> + * Returns true, if the memory type of @pfn is UC or UC-.
> + * Otherwise, returns false.
> + */
> +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> +{
> +	enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> +
> +	return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> +}
> +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
> +
>  /**
>   * io_reserve_memtype - Request a memory type mapping for a region of memory
>   * @start: start (physical address) of the region
> 

Wonder if we should check for pat internally. And if we should simply
return the memtype via lookup_memtype() instead of creating such a
strange named function (by providing e.g. a lookup_memtype() variant
that can be called with !pat_enabled()).

The caller can easily check against _PAGE_CACHE_MODE_UC ...


-- 

Thanks,

David / dhildenb

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Dan Williams]

On Tue, Nov 7, 2017 at 11:56 PM, Haozhong Zhang
<haozhong.zhang@intel.com> wrote:
> It will be used by KVM to check whether a pfn should be
> mapped to guest as UC.
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  arch/x86/include/asm/pat.h |  2 ++
>  arch/x86/mm/pat.c          | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> index fffb2794dd89..fabb0cf00e77 100644
> --- a/arch/x86/include/asm/pat.h
> +++ b/arch/x86/include/asm/pat.h
> @@ -21,4 +21,6 @@ int io_reserve_memtype(resource_size_t start, resource_size_t end,
>
>  void io_free_memtype(resource_size_t start, resource_size_t end);
>
> +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn);
> +
>  #endif /* _ASM_X86_PAT_H */
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index fe7d57a8fb60..e1282dd4eeb8 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -677,6 +677,22 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
>         return rettype;
>  }
>
> +/**
> + * Check with PAT whether the memory type of a pfn is UC or UC-.
> + *
> + * Only to be called when PAT is enabled.
> + *
> + * Returns true, if the memory type of @pfn is UC or UC-.
> + * Otherwise, returns false.
> + */
> +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> +{
> +       enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> +
> +       return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> +}
> +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);

Why do we need this strangely named new accessor? It seems to be
open-coding a new / more limited version of track_pfn_insert().

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Haozhong Zhang]

On 11/15/17 07:17 -0800, Dan Williams wrote:
> On Tue, Nov 7, 2017 at 11:56 PM, Haozhong Zhang
> <haozhong.zhang@intel.com> wrote:
> > It will be used by KVM to check whether a pfn should be
> > mapped to guest as UC.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  arch/x86/include/asm/pat.h |  2 ++
> >  arch/x86/mm/pat.c          | 16 ++++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> > index fffb2794dd89..fabb0cf00e77 100644
> > --- a/arch/x86/include/asm/pat.h
> > +++ b/arch/x86/include/asm/pat.h
> > @@ -21,4 +21,6 @@ int io_reserve_memtype(resource_size_t start, resource_size_t end,
> >
> >  void io_free_memtype(resource_size_t start, resource_size_t end);
> >
> > +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn);
> > +
> >  #endif /* _ASM_X86_PAT_H */
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index fe7d57a8fb60..e1282dd4eeb8 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> > @@ -677,6 +677,22 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
> >         return rettype;
> >  }
> >
> > +/**
> > + * Check with PAT whether the memory type of a pfn is UC or UC-.
> > + *
> > + * Only to be called when PAT is enabled.
> > + *
> > + * Returns true, if the memory type of @pfn is UC or UC-.
> > + * Otherwise, returns false.
> > + */
> > +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> > +{
> > +       enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> > +
> > +       return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> > +}
> > +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
> 
> Why do we need this strangely named new accessor? It seems to be
> open-coding a new / more limited version of track_pfn_insert().

In the first version patchset, KVM did extract and check the cache
mode got from track_pfn_insert(), but Ingo thought it was better to
keep all the low-level details out of KVM, so I encapsulated them in a
function in mm in subsequent versions.

Haozhong

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Haozhong Zhang]

On 11/15/17 11:44 +0100, David Hildenbrand wrote:
> On 08.11.2017 08:56, Haozhong Zhang wrote:
> > It will be used by KVM to check whether a pfn should be
> > mapped to guest as UC.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  arch/x86/include/asm/pat.h |  2 ++
> >  arch/x86/mm/pat.c          | 16 ++++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> > index fffb2794dd89..fabb0cf00e77 100644
> > --- a/arch/x86/include/asm/pat.h
> > +++ b/arch/x86/include/asm/pat.h
> > @@ -21,4 +21,6 @@ int io_reserve_memtype(resource_size_t start, resource_size_t end,
> >  
> >  void io_free_memtype(resource_size_t start, resource_size_t end);
> >  
> > +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn);
> > +
> >  #endif /* _ASM_X86_PAT_H */
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index fe7d57a8fb60..e1282dd4eeb8 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> > @@ -677,6 +677,22 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
> >  	return rettype;
> >  }
> >  
> > +/**
> > + * Check with PAT whether the memory type of a pfn is UC or UC-.
> > + *
> > + * Only to be called when PAT is enabled.
> > + *
> > + * Returns true, if the memory type of @pfn is UC or UC-.
> > + * Otherwise, returns false.
> > + */
> > +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> > +{
> > +	enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> > +
> > +	return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> > +}
> > +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
> > +
> >  /**
> >   * io_reserve_memtype - Request a memory type mapping for a region of memory
> >   * @start: start (physical address) of the region
> > 
> 
> Wonder if we should check for pat internally. And if we should simply
> return the memtype via lookup_memtype() instead of creating such a
> strange named function (by providing e.g. a lookup_memtype() variant
> that can be called with !pat_enabled()).
>
> The caller can easily check against _PAGE_CACHE_MODE_UC ...
>

Yes, the better solution should work for both PAT enabled and disabled
cases, like what __vm_insert_mixed() does: use vma->vm_page_prot if
PAT is disabled, and refer to track_pfn_insert() in addition if PAT is
enabled.

The early RFC patch [1] got the cache mode in a similar way via a new
function kvm_vcpu_gfn_to_pgprot(). However, as explained in RFC, it
does not work, because the existing MMIO check (where kvm_vcpu_gfn_to_pgprot()
is called) in KVM is performed with a spinlock (vcpu->kvm->mmu_lock)
being taken, but kvm_vcpu_gfn_to_pgprot() has to touch a semaphore
(vcpu->kvm->mm->mmap_sem). Besides, KVM may prefetch and check MMIO of
other pfns within vcpu->kvm->mmu_lock, and the prefectched pfns cannot
be predicted in advance, which means we have to keep the MMIO check
within vcpu->kvm->mmu_lock.

Therefore, I only make a suboptimal fix in this patchset that only
fixes PAT enabled cases, which I suppose is the usual usage scenario
of NVDIMM.


[1] https://patchwork.kernel.org/patch/10016261/


Haozhong

Re: [PATCH v5 2/2] KVM: MMU: consider host cache mode in MMIO page check

[Paolo Bonzini]

On 08/11/2017 08:56, Haozhong Zhang wrote:
> Some reserved pages, such as those from NVDIMM DAX devices, are not
> for MMIO, and can be mapped with cached memory type for better
> performance. However, the above check misconceives those pages as
> MMIO.  Because KVM maps MMIO pages with UC memory type, the
> performance of guest accesses to those pages would be harmed.
> Therefore, we check the host memory type 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>

WC should be allowed as well, because the combination of EPT_PAT=UC and
gPAT=WC gives WC effective memory type.

Maybe it's better after all if the lookup_memtype call remains in
kvm_is_mmio_pfn, like

	if (!pfn_valid(pfn))
		return true;

	if (is_zero_pfn(pfn) || !PageReserved(pfn_to_page(pfn))
		return false;

	/* ... long comment ... */
	if (!pat_enabled()
		return true;
	else {
		cm = lookup_memtype(PFN_PHYS(pfn));
		return cm == ...
	}

or something like that.

Thanks,

Paolo

> ---
>  arch/x86/kvm/mmu.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 0b481cc9c725..7715476bc5c9 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2708,7 +2708,20 @@ 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));
> +		return !is_zero_pfn(pfn) && PageReserved(pfn_to_page(pfn)) &&
> +			/*
> +			 * Some reserved pages, such as those from
> +			 * NVDIMM DAX devices, are not for MMIO, and
> +			 * can be mapped with cached memory type for
> +			 * better performance. However, the above
> +			 * check misconceives those pages as MMIO.
> +			 * Because KVM maps MMIO pages with UC memory
> +			 * type, the performance of guest accesses to
> +			 * those pages would be harmed. Therefore, we
> +			 * check the host memory type in addition and
> +			 * only treat UC/UC- pages as MMIO.
> +			 */
> +			(!pat_enabled() || pat_pfn_is_uc_or_uc_minus(pfn));
>  
>  	return true;
>  }
> 

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Paolo Bonzini]

On 08/11/2017 08:56, Haozhong Zhang wrote:
> +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> +{
> +	enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> +
> +	return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> +}
> +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
> +

As discussed in the reply to v2, this should include WC too.  The
function name could become something like pat_pfn_downgraded_by_uc_mtrr.

Thanks,

Paolo

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Haozhong Zhang]

On 12/18/17 13:55 +0100, Paolo Bonzini wrote:
> On 08/11/2017 08:56, Haozhong Zhang wrote:
> > +bool pat_pfn_is_uc_or_uc_minus(unsigned long pfn)
> > +{
> > +	enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> > +
> > +	return cm == _PAGE_CACHE_MODE_UC || cm == _PAGE_CACHE_MODE_UC_MINUS;
> > +}
> > +EXPORT_SYMBOL_GPL(pat_pfn_is_uc_or_uc_minus);
> > +
> 
> As discussed in the reply to v2, this should include WC too.  The
> function name could become something like pat_pfn_downgraded_by_uc_mtrr.

Or shall we just expose lookup_memtype(), and keep all other logic in
KVM? The function name still looks strange somehow, and the check of
memory type makes more sense and would be easier to understand in the
context of KVM.

Haozhong

Re: [PATCH v5 1/2] x86/mm: add a function to check if a pfn is UC/UC-

[Paolo Bonzini]

On 19/12/2017 03:40, Haozhong Zhang wrote:
>> As discussed in the reply to v2, this should include WC too.  The
>> function name could become something like pat_pfn_downgraded_by_uc_mtrr.
>
> Or shall we just expose lookup_memtype(), and keep all other logic in
> KVM? The function name still looks strange somehow, and the check of
> memory type makes more sense and would be easier to understand in the
> context of KVM.

I agree that it's a bit strange, but I don't want to go against the
suggestions of the x86 maintainer.

Paolo