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

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

[Haozhong Zhang]

[I just copy the commit message from patch 2]

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 by lookup_memtype() in
addition and only treat UC/UC- pages as MMIO.

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: expose lookup_memtype()
  KVM: MMU: consider host cache mode in MMIO page check

 arch/x86/include/asm/pat.h |  2 ++
 arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
 arch/x86/mm/pat.c          |  3 ++-
 3 files changed, 31 insertions(+), 4 deletions(-)

-- 
2.14.1

[PATCH v2 1/2] x86/mm: expose lookup_memtype()

[Haozhong Zhang]

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

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

diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index fffb2794dd89..990d955972b8 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);
 
+enum page_cache_mode lookup_memtype(u64 paddr);
+
 #endif /* _ASM_X86_PAT_H */
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index fe7d57a8fb60..e85987aaf5d5 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -650,7 +650,7 @@ int free_memtype(u64 start, u64 end)
  * Returns _PAGE_CACHE_MODE_WB, _PAGE_CACHE_MODE_WC, _PAGE_CACHE_MODE_UC_MINUS
  * or _PAGE_CACHE_MODE_WT.
  */
-static enum page_cache_mode lookup_memtype(u64 paddr)
+enum page_cache_mode lookup_memtype(u64 paddr)
 {
 	enum page_cache_mode rettype = _PAGE_CACHE_MODE_WB;
 	struct memtype *entry;
@@ -676,6 +676,7 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
 	spin_unlock(&memtype_lock);
 	return rettype;
 }
+EXPORT_SYMBOL_GPL(lookup_memtype);
 
 /**
  * io_reserve_memtype - Request a memory type mapping for a region of memory
-- 
2.14.1

[PATCH v2 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 by lookup_memtype() 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 | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 0b481cc9c725..206828d18857 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2707,10 +2707,34 @@ 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));
+
+		/*
+		 * 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 by lookup_memtype() in addition and
+		 * only treat UC/UC- pages as MMIO.
+		 *
+		 * lookup_memtype() works only when PAT is enabled, so
+		 * add pat_enabled() check here.
+		 */
+		if (is_mmio && pat_enabled()) {
+			enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
+
+			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 v2 0/2] KVM: MMU: fix kvm_is_mmio_pfn()

[Haozhong Zhang]

On 10/31/17 19:48 +0800, Haozhong Zhang wrote:
> [I just copy the commit message from patch 2]
> 
> 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 by lookup_memtype() in
> addition and only treat UC/UC- pages as MMIO.
> 
> 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: expose lookup_memtype()
>   KVM: MMU: consider host cache mode in MMIO page check
> 
>  arch/x86/include/asm/pat.h |  2 ++
>  arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
>  arch/x86/mm/pat.c          |  3 ++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> -- 
> 2.14.1
> 

Hi Paolo,

This patchset fixed a performance drop issue when using NVDIMM on KVM,
so I think it's nice to have in 4.14. Can you help to review it?

Thanks,
Haozhong

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

[Xiao Guangrong]

On 10/31/2017 07:48 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 by lookup_memtype() 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 | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 0b481cc9c725..206828d18857 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2707,10 +2707,34 @@ 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));
> +
> +		/*
> +		 * 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 by lookup_memtype() in addition and
> +		 * only treat UC/UC- pages as MMIO.
> +		 *
> +		 * lookup_memtype() works only when PAT is enabled, so
> +		 * add pat_enabled() check here.
> +		 */
> +		if (is_mmio && pat_enabled()) {
> +			enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> +
> +			is_mmio = (cm == _PAGE_CACHE_MODE_UC ||
> +				   cm == _PAGE_CACHE_MODE_UC_MINUS);
> +		}
> +	}

You can move all of these detailed stuffs to pat.c and abstract them by introducing
a function, maybe named pat_pfn_is_uc(). I think this is what Ingo wants.

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

[Haozhong Zhang]

On 11/02/17 15:56 +0800, Xiao Guangrong wrote:
> 
> 
> On 10/31/2017 07:48 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 by lookup_memtype() 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 | 30 +++++++++++++++++++++++++++---
> >   1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 0b481cc9c725..206828d18857 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -2707,10 +2707,34 @@ 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));
> > +
> > +		/*
> > +		 * 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 by lookup_memtype() in addition and
> > +		 * only treat UC/UC- pages as MMIO.
> > +		 *
> > +		 * lookup_memtype() works only when PAT is enabled, so
> > +		 * add pat_enabled() check here.
> > +		 */
> > +		if (is_mmio && pat_enabled()) {
> > +			enum page_cache_mode cm = lookup_memtype(PFN_PHYS(pfn));
> > +
> > +			is_mmio = (cm == _PAGE_CACHE_MODE_UC ||
> > +				   cm == _PAGE_CACHE_MODE_UC_MINUS);
> > +		}
> > +	}
> 
> You can move all of these detailed stuffs to pat.c and abstract them by introducing
> a function, maybe named pat_pfn_is_uc(). I think this is what Ingo wants.
> 

OK, I'll move the cache mode check to a function in pat.c.

Let me wait for Paolo and others' comments. If no additional comments,
I'll send another version ASAP.

Thanks,
Haozhong

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

[Konrad Rzeszutek Wilk]

On Tue, Oct 31, 2017 at 07:48:38PM +0800, Haozhong Zhang wrote:
> [I just copy the commit message from patch 2]
> 
> 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 by lookup_memtype() in
> addition and only treat UC/UC- pages as MMIO.

Is there a specific workload you used to detect this?

Thanks!
> 
> 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: expose lookup_memtype()
>   KVM: MMU: consider host cache mode in MMIO page check
> 
>  arch/x86/include/asm/pat.h |  2 ++
>  arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
>  arch/x86/mm/pat.c          |  3 ++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> -- 
> 2.14.1
> 

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

[Haozhong Zhang]

On 11/02/17 13:37 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 31, 2017 at 07:48:38PM +0800, Haozhong Zhang wrote:
> > [I just copy the commit message from patch 2]
> > 
> > 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 by lookup_memtype() in
> > addition and only treat UC/UC- pages as MMIO.
> 
> Is there a specific workload you used to detect this?
>

Creating files on NVDIMM in VM is must slower than the same operation
on the baremetal. As the wrong EPT memory type (UC vs. WB) is used,
every guest access to NVDIMM would be slower than baremetal.

Haozhong

> Thanks!
> > 
> > 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: expose lookup_memtype()
> >   KVM: MMU: consider host cache mode in MMIO page check
> > 
> >  arch/x86/include/asm/pat.h |  2 ++
> >  arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
> >  arch/x86/mm/pat.c          |  3 ++-
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.14.1
> > 

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

[Mikulas Patocka]

On Tue, 31 Oct 2017, Haozhong Zhang wrote:

> [I just copy the commit message from patch 2]
> 
> 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

Note that cached memory type on persistent memory has horrible 
performance. The clwb instruction on Broadwell is very slow - when you 
write to persistent memory and use clwb to flush cache, the performance is 
about 350MB/s.

Using write-combining memory type for persistent memory is much faster, it 
can sustain performance of one 8-byte write per tick.

Mikulas

> 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 by lookup_memtype() in
> addition and only treat UC/UC- pages as MMIO.
> 
> 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: expose lookup_memtype()
>   KVM: MMU: consider host cache mode in MMIO page check
> 
>  arch/x86/include/asm/pat.h |  2 ++
>  arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
>  arch/x86/mm/pat.c          |  3 ++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> -- 
> 2.14.1
> 

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

[Haozhong Zhang]

On 11/03/17 10:15 -0400, Mikulas Patocka wrote:
> 
> 
> On Tue, 31 Oct 2017, Haozhong Zhang wrote:
> 
> > [I just copy the commit message from patch 2]
> > 
> > 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
> 
> Note that cached memory type on persistent memory has horrible 
> performance. The clwb instruction on Broadwell is very slow - when you 
> write to persistent memory and use clwb to flush cache, the performance is 
> about 350MB/s.

Wasn't clwb first introduced on Skylake?

Haozhong

> 
> Using write-combining memory type for persistent memory is much faster, it 
> can sustain performance of one 8-byte write per tick.
> 
> Mikulas
> 
> > 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 by lookup_memtype() in
> > addition and only treat UC/UC- pages as MMIO.
> > 
> > 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: expose lookup_memtype()
> >   KVM: MMU: consider host cache mode in MMIO page check
> > 
> >  arch/x86/include/asm/pat.h |  2 ++
> >  arch/x86/kvm/mmu.c         | 30 +++++++++++++++++++++++++++---
> >  arch/x86/mm/pat.c          |  3 ++-
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.14.1
> > 

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

[Mikulas Patocka]

On Wed, 8 Nov 2017, Haozhong Zhang wrote:

> On 11/03/17 10:15 -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Tue, 31 Oct 2017, Haozhong Zhang wrote:
> > 
> > > [I just copy the commit message from patch 2]
> > > 
> > > 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
> > 
> > Note that cached memory type on persistent memory has horrible 
> > performance. The clwb instruction on Broadwell is very slow - when you 
> > write to persistent memory and use clwb to flush cache, the performance is 
> > about 350MB/s.
> 
> Wasn't clwb first introduced on Skylake?
> 
> Haozhong

You're right - the clwb() function uses clflushopt on Broadwell - that's 
what I meant is slow.

Mikulas