| Message ID | 20200212160918.18470-3-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen on Hyper-V: Implement L0 assisted TLB flush | expand |
On Wed, Feb 12, 2020 at 04:09:16PM +0000, Wei Liu wrote: > Hyper-V's L0 assisted flush has fine-grained control over what gets > flushed. We need all the flags available to make the best decisions > possible. > > No functional change because Xen's implementation doesn't care about > what is passed to it. While it's certainly fine to pass a flags field with more information, the flush flags for Xen can also contain FLUSH_CACHE, FLUSH_VCPU_STATE or FLUSH_ROOT_PGTBL, can you add an assert that those never get passed to the flush hook? IMO we should define a mask with FLUSH_TLB, FLUSH_TLB_GLOBAL, FLUSH_VA_VALID and FLUSH_ORDER_MASK and assert that those are the only valid flags to be used for the hypervisor assisted flush hook. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 12 February 2020 18:01 > To: Wei Liu <wl@xen.org> > Cc: Xen Development List <xen-devel@lists.xenproject.org>; Durrant, Paul > <pdurrant@amazon.co.uk>; Michael Kelley <mikelley@microsoft.com>; Wei Liu > <liuwe@microsoft.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <andrew.cooper3@citrix.com> > Subject: Re: [PATCH 2/4] x86/hypervisor: pass flags to > hypervisor_flush_tlb > > On Wed, Feb 12, 2020 at 04:09:16PM +0000, Wei Liu wrote: > > Hyper-V's L0 assisted flush has fine-grained control over what gets > > flushed. We need all the flags available to make the best decisions > > possible. > > > > No functional change because Xen's implementation doesn't care about > > what is passed to it. > > While it's certainly fine to pass a flags field with more information, > the flush flags for Xen can also contain FLUSH_CACHE, FLUSH_VCPU_STATE > or FLUSH_ROOT_PGTBL, can you add an assert that those never get passed > to the flush hook? > > IMO we should define a mask with FLUSH_TLB, FLUSH_TLB_GLOBAL, > FLUSH_VA_VALID and FLUSH_ORDER_MASK and assert that those are the only > valid flags to be used for the hypervisor assisted flush hook. > Agreed that this should be abstracted; we certainly don't want to have various bits of Xen needing to know what hypervisor it is running on top of. Paul > Thanks, Roger.
On Thu, Feb 13, 2020 at 08:48:39AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Roger Pau Monné <roger.pau@citrix.com> > > Sent: 12 February 2020 18:01 > > To: Wei Liu <wl@xen.org> > > Cc: Xen Development List <xen-devel@lists.xenproject.org>; Durrant, Paul > > <pdurrant@amazon.co.uk>; Michael Kelley <mikelley@microsoft.com>; Wei Liu > > <liuwe@microsoft.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > > <andrew.cooper3@citrix.com> > > Subject: Re: [PATCH 2/4] x86/hypervisor: pass flags to > > hypervisor_flush_tlb > > > > On Wed, Feb 12, 2020 at 04:09:16PM +0000, Wei Liu wrote: > > > Hyper-V's L0 assisted flush has fine-grained control over what gets > > > flushed. We need all the flags available to make the best decisions > > > possible. > > > > > > No functional change because Xen's implementation doesn't care about > > > what is passed to it. > > > > While it's certainly fine to pass a flags field with more information, > > the flush flags for Xen can also contain FLUSH_CACHE, FLUSH_VCPU_STATE > > or FLUSH_ROOT_PGTBL, can you add an assert that those never get passed > > to the flush hook? > > > > IMO we should define a mask with FLUSH_TLB, FLUSH_TLB_GLOBAL, > > FLUSH_VA_VALID and FLUSH_ORDER_MASK and assert that those are the only > > valid flags to be used for the hypervisor assisted flush hook. > > > > Agreed that this should be abstracted; we certainly don't want to have > various bits of Xen needing to know what hypervisor it is running on > top of. OK. I can introduce a FLUSH_TLB_FLAGS for all things pertaining to TLB -- the four things mentioned in Roger's reply. Wei. > > Paul > > > > Thanks, Roger.
On Thu, Feb 13, 2020 at 10:29:16AM +0000, Wei Liu wrote: > On Thu, Feb 13, 2020 at 08:48:39AM +0000, Durrant, Paul wrote: > > > -----Original Message----- > > > From: Roger Pau Monné <roger.pau@citrix.com> > > > Sent: 12 February 2020 18:01 > > > To: Wei Liu <wl@xen.org> > > > Cc: Xen Development List <xen-devel@lists.xenproject.org>; Durrant, Paul > > > <pdurrant@amazon.co.uk>; Michael Kelley <mikelley@microsoft.com>; Wei Liu > > > <liuwe@microsoft.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > > > <andrew.cooper3@citrix.com> > > > Subject: Re: [PATCH 2/4] x86/hypervisor: pass flags to > > > hypervisor_flush_tlb > > > > > > On Wed, Feb 12, 2020 at 04:09:16PM +0000, Wei Liu wrote: > > > > Hyper-V's L0 assisted flush has fine-grained control over what gets > > > > flushed. We need all the flags available to make the best decisions > > > > possible. > > > > > > > > No functional change because Xen's implementation doesn't care about > > > > what is passed to it. > > > > > > While it's certainly fine to pass a flags field with more information, > > > the flush flags for Xen can also contain FLUSH_CACHE, FLUSH_VCPU_STATE > > > or FLUSH_ROOT_PGTBL, can you add an assert that those never get passed > > > to the flush hook? > > > > > > IMO we should define a mask with FLUSH_TLB, FLUSH_TLB_GLOBAL, > > > FLUSH_VA_VALID and FLUSH_ORDER_MASK and assert that those are the only > > > valid flags to be used for the hypervisor assisted flush hook. > > > > > > > Agreed that this should be abstracted; we certainly don't want to have > > various bits of Xen needing to know what hypervisor it is running on > > top of. > > OK. I can introduce a FLUSH_TLB_FLAGS for all things pertaining to TLB > -- the four things mentioned in Roger's reply. If I'm not mistaken flush_area_mask in Roger's patch already filters out all the unwanted flags, so my code is correct as-is. I can add #define FLUSH_TLB_FLAGS_MASK (FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_VA_VALID | \ FLUSH_ORDER_MASK) and use it in flush_area_mask to replace the longer string there. Wei. > > Wei. > > > > > Paul > > > > > > > Thanks, Roger.
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c index 47e938e287..2724fd9bad 100644 --- a/xen/arch/x86/guest/hypervisor.c +++ b/xen/arch/x86/guest/hypervisor.c @@ -75,10 +75,10 @@ void __init hypervisor_e820_fixup(struct e820map *e820) } int hypervisor_flush_tlb(const cpumask_t *mask, const void *va, - unsigned int order) + unsigned int flags) { if ( ops.flush_tlb ) - return alternative_call(ops.flush_tlb, mask, va, order); + return alternative_call(ops.flush_tlb, mask, va, flags); return -ENOSYS; } diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c index 5d3427a713..0eb1115c4d 100644 --- a/xen/arch/x86/guest/xen/xen.c +++ b/xen/arch/x86/guest/xen/xen.c @@ -324,7 +324,7 @@ static void __init e820_fixup(struct e820map *e820) pv_shim_fixup_e820(e820); } -static int flush_tlb(const cpumask_t *mask, const void *va, unsigned int order) +static int flush_tlb(const cpumask_t *mask, const void *va, unsigned int flags) { return xen_hypercall_hvm_op(HVMOP_flush_tlbs, NULL); } diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c index 9bc925616a..2df21e396a 100644 --- a/xen/arch/x86/smp.c +++ b/xen/arch/x86/smp.c @@ -260,7 +260,7 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags) if ( cpu_has_hypervisor && !(flags & ~(FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_VA_VALID | FLUSH_ORDER_MASK)) && - !hypervisor_flush_tlb(mask, va, flags & FLUSH_ORDER_MASK) ) + !hypervisor_flush_tlb(mask, va, flags) ) { if ( tlb_clk_enabled ) tlb_clk_enabled = false; diff --git a/xen/include/asm-x86/guest/hypervisor.h b/xen/include/asm-x86/guest/hypervisor.h index 432e57c2a0..48d54735d2 100644 --- a/xen/include/asm-x86/guest/hypervisor.h +++ b/xen/include/asm-x86/guest/hypervisor.h @@ -35,7 +35,7 @@ struct hypervisor_ops { /* Fix up e820 map */ void (*e820_fixup)(struct e820map *e820); /* L0 assisted TLB flush */ - int (*flush_tlb)(const cpumask_t *mask, const void *va, unsigned int order); + int (*flush_tlb)(const cpumask_t *mask, const void *va, unsigned int flags); }; #ifdef CONFIG_GUEST @@ -48,11 +48,11 @@ void hypervisor_e820_fixup(struct e820map *e820); /* * L0 assisted TLB flush. * mask: cpumask of the dirty vCPUs that should be flushed. - * va: linear address to flush, or NULL for global flushes. - * order: order of the linear address pointed by va. + * va: linear address to flush, or NULL for entire address space. + * flags: flags for flushing, including the order of va. */ int hypervisor_flush_tlb(const cpumask_t *mask, const void *va, - unsigned int order); + unsigned int flags); #else @@ -65,7 +65,7 @@ static inline int hypervisor_ap_setup(void) { return 0; } static inline void hypervisor_resume(void) { ASSERT_UNREACHABLE(); } static inline void hypervisor_e820_fixup(struct e820map *e820) {} static inline int hypervisor_flush_tlb(const cpumask_t *mask, const void *va, - unsigned int order) + unsigned int flags) { return -ENOSYS; }
Hyper-V's L0 assisted flush has fine-grained control over what gets flushed. We need all the flags available to make the best decisions possible. No functional change because Xen's implementation doesn't care about what is passed to it. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- xen/arch/x86/guest/hypervisor.c | 4 ++-- xen/arch/x86/guest/xen/xen.c | 2 +- xen/arch/x86/smp.c | 2 +- xen/include/asm-x86/guest/hypervisor.h | 10 +++++----- 4 files changed, 9 insertions(+), 9 deletions(-)