| Message ID | 1457462655-21631-1-git-send-email-david.vrabel@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Obviously the subject should not have contained an internal ticket reference. Please remove before committing or, if preferred, I can resend. David
>>> 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
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
>>> 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
>>> 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
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
>>> 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
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);
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(-)