[PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[David Vrabel]

Introduce the FLUSH_CACHE_BY_VA flag to flush_area_mask() and friends
to say that it is safe to use CLFLUSH (i.e., the virtual address is
still valid).

Use this when changing the cachability of the Xen direct mappings (in
response to the guest changing the cachability of its mappings). This
significantly improves performance by avoiding an expensive WBINVD.

This fixes a performance regression introduced by
c61a6f74f80eb36ed83a82f713db3143159b9009 (x86: enforce consistent
cachability of MMIO mappings), the fix for XSA-154.

e.g., A set_memory_wc() call in Linux:

before: 4097 us
after:    47 us

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/flushtlb.c        | 3 ++-
 xen/arch/x86/mm.c              | 2 +-
 xen/include/asm-x86/flushtlb.h | 3 +++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 582a1e2..b5aa989 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -140,7 +140,8 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
         if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
             sz = 1UL << (order + PAGE_SHIFT);
 
-        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
+        if ( (!(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) ||
+              flags & FLUSH_VA_VALID) &&
              c->x86_clflush_size && c->x86_cache_size && sz &&
              ((sz >> 10) < c->x86_cache_size) )
         {
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 36c4487..1981ca0 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5641,7 +5641,7 @@ int map_pages_to_xen(
         flush_flags |= FLUSH_TLB_GLOBAL;       \
     if ( (flags & _PAGE_PRESENT) &&            \
          (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
-        flush_flags |= FLUSH_CACHE;            \
+        flush_flags |= FLUSH_CACHE_BY_VA;      \
 } while (0)
 
     while ( nr_mfns != 0 )
diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h
index 4ea31c2..ae90a37 100644
--- a/xen/include/asm-x86/flushtlb.h
+++ b/xen/include/asm-x86/flushtlb.h
@@ -85,6 +85,9 @@ void write_cr3(unsigned long cr3);
 #define FLUSH_TLB_GLOBAL 0x200
  /* Flush data caches */
 #define FLUSH_CACHE      0x400
+ /* VA for the flush has a valid mapping */
+#define FLUSH_VA_VALID   0x800
+#define FLUSH_CACHE_BY_VA (FLUSH_CACHE | FLUSH_VA_VALID)
 
 /* Flush local TLBs/caches. */
 unsigned int flush_area_local(const void *va, unsigned int flags);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[David Vrabel]

Obviously the subject should not have contained an internal ticket
reference.

Please remove before committing or, if preferred, I can resend.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[Jan Beulich]

>>> On 08.03.16 at 19:44, <david.vrabel@citrix.com> wrote:
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -140,7 +140,8 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>          if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
>              sz = 1UL << (order + PAGE_SHIFT);
>  
> -        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
> +        if ( (!(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) ||
> +              flags & FLUSH_VA_VALID) &&

The & wants to be parenthesized.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5641,7 +5641,7 @@ int map_pages_to_xen(
>          flush_flags |= FLUSH_TLB_GLOBAL;       \
>      if ( (flags & _PAGE_PRESENT) &&            \
>           (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
> -        flush_flags |= FLUSH_CACHE;            \
> +        flush_flags |= FLUSH_CACHE_BY_VA;      \
>  } while (0)

No, that's too simple. You may flush by VA only if the MFN didn't
change (or else you flush the wrong page).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[David Vrabel]

On 09/03/16 14:01, Jan Beulich wrote:
>>>> On 08.03.16 at 19:44, <david.vrabel@citrix.com> wrote:
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -140,7 +140,8 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>          if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
>>              sz = 1UL << (order + PAGE_SHIFT);
>>  
>> -        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
>> +        if ( (!(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) ||
>> +              flags & FLUSH_VA_VALID) &&
> 
> The & wants to be parenthesized.

Style nit-picks really should come with a patch to CODING_STYLE.  The
existing code isn't consistent enough to deduce the preferred style.

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5641,7 +5641,7 @@ int map_pages_to_xen(
>>          flush_flags |= FLUSH_TLB_GLOBAL;       \
>>      if ( (flags & _PAGE_PRESENT) &&            \
>>           (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
>> -        flush_flags |= FLUSH_CACHE;            \
>> +        flush_flags |= FLUSH_CACHE_BY_VA;      \
>>  } while (0)
> 
> No, that's too simple. You may flush by VA only if the MFN didn't
> change (or else you flush the wrong page).

Cachability changes goes through update_xen_mappings() which always uses
the same MFN -> virt mapping so the MFN never changes.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[Jan Beulich]

>>> On 09.03.16 at 15:36, <david.vrabel@citrix.com> wrote:
> On 09/03/16 14:01, Jan Beulich wrote:
>>>>> On 08.03.16 at 19:44, <david.vrabel@citrix.com> wrote:
>>> --- a/xen/arch/x86/flushtlb.c
>>> +++ b/xen/arch/x86/flushtlb.c
>>> @@ -140,7 +140,8 @@ unsigned int flush_area_local(const void *va, unsigned 
> int flags)
>>>          if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
>>>              sz = 1UL << (order + PAGE_SHIFT);
>>>  
>>> -        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
>>> +        if ( (!(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) ||
>>> +              flags & FLUSH_VA_VALID) &&
>> 
>> The & wants to be parenthesized.
> 
> Style nit-picks really should come with a patch to CODING_STYLE.  The
> existing code isn't consistent enough to deduce the preferred style.

Well, I would have silently added the parentheses upon commit
if there wasn't that other issue below. (Besides that I don't think
you'll find that many unparenthesized & or | which are themselves
operands to another operator. Others, e.g. Andrew, are even
more strict than me in requiring parentheses around all binary
operations that are themselves operands; I personally think that
some precedence rules can be assumed to be commonly known,
but the relationship of & and | vs && and || is clearly not among
those.)

>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5641,7 +5641,7 @@ int map_pages_to_xen(
>>>          flush_flags |= FLUSH_TLB_GLOBAL;       \
>>>      if ( (flags & _PAGE_PRESENT) &&            \
>>>           (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
>>> -        flush_flags |= FLUSH_CACHE;            \
>>> +        flush_flags |= FLUSH_CACHE_BY_VA;      \
>>>  } while (0)
>> 
>> No, that's too simple. You may flush by VA only if the MFN didn't
>> change (or else you flush the wrong page).
> 
> Cachability changes goes through update_xen_mappings() which always uses
> the same MFN -> virt mapping so the MFN never changes.

But the code you change affects map_pages_to_xen(), of which
only one caller is update_xen_mappings(). It would be calling for
hard to debug bugs if we baked in an assumption that only the
latter can do cachability adjustments.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[Jan Beulich]

>>> On 09.03.16 at 15:52, <JBeulich@suse.com> wrote:
>>>> On 09.03.16 at 15:36, <david.vrabel@citrix.com> wrote:
>> On 09/03/16 14:01, Jan Beulich wrote:
>>>>>> On 08.03.16 at 19:44, <david.vrabel@citrix.com> wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -5641,7 +5641,7 @@ int map_pages_to_xen(
>>>>          flush_flags |= FLUSH_TLB_GLOBAL;       \
>>>>      if ( (flags & _PAGE_PRESENT) &&            \
>>>>           (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
>>>> -        flush_flags |= FLUSH_CACHE;            \
>>>> +        flush_flags |= FLUSH_CACHE_BY_VA;      \
>>>>  } while (0)
>>> 
>>> No, that's too simple. You may flush by VA only if the MFN didn't
>>> change (or else you flush the wrong page).
>> 
>> Cachability changes goes through update_xen_mappings() which always uses
>> the same MFN -> virt mapping so the MFN never changes.
> 
> But the code you change affects map_pages_to_xen(), of which
> only one caller is update_xen_mappings(). It would be calling for
> hard to debug bugs if we baked in an assumption that only the
> latter can do cachability adjustments.

And there's a second issue here: map_pages_to_xen() updates
the idle page tables, only part of which is actually in use on a
huge machine in the context of a PV guest, so the above is also
risking a #PF on a guest address for the CLFLUSH. Setting the
new flag only when the virtual address the mapping of which
gets changed is within
[DIRECTMAP_VIRT_START, HYPERVISOR_VIRT_END) would
seem an acceptable assumption: We know we're never changing
the VFN <-> PFN relationship inside the 1:1 mapping.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[David Vrabel]

On 09/03/16 15:23, Jan Beulich wrote:
> 
> And there's a second issue here: map_pages_to_xen() updates
> the idle page tables, only part of which is actually in use on a
> huge machine in the context of a PV guest, so the above is also
> risking a #PF on a guest address for the CLFLUSH. Setting the
> new flag only when the virtual address the mapping of which
> gets changed is within
> [DIRECTMAP_VIRT_START, HYPERVISOR_VIRT_END) would
> seem an acceptable assumption: We know we're never changing
> the VFN <-> PFN relationship inside the 1:1 mapping.

Andrew suggested [DIRECTMAP_VIRT_START, DIRECTMAP_VIRT_END) which makes
more sense to me.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

[Jan Beulich]

>>> On 09.03.16 at 19:46, <david.vrabel@citrix.com> wrote:
> On 09/03/16 15:23, Jan Beulich wrote:
>> 
>> And there's a second issue here: map_pages_to_xen() updates
>> the idle page tables, only part of which is actually in use on a
>> huge machine in the context of a PV guest, so the above is also
>> risking a #PF on a guest address for the CLFLUSH. Setting the
>> new flag only when the virtual address the mapping of which
>> gets changed is within
>> [DIRECTMAP_VIRT_START, HYPERVISOR_VIRT_END) would
>> seem an acceptable assumption: We know we're never changing
>> the VFN <-> PFN relationship inside the 1:1 mapping.
> 
> Andrew suggested [DIRECTMAP_VIRT_START, DIRECTMAP_VIRT_END) which makes
> more sense to me.

But which is wrong, as it would again cover both the always visible
part and that visible only outside of the context of PV guests.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel