Re: [Xen-devel] [PATCH v2 2/3] x86/hyperv: skeleton for L0 assisted TLB flush

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Feb 17, 2020 at 11:45:38AM +0000, Wei Liu wrote:
> On Mon, Feb 17, 2020 at 12:40:31PM +0100, Roger Pau Monné wrote:
> > On Mon, Feb 17, 2020 at 11:34:41AM +0000, Wei Liu wrote:
> > > On Fri, Feb 14, 2020 at 04:55:44PM +0000, Durrant, Paul wrote:
> > > > > -----Original Message-----
> > > > > From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu
> > > > > Sent: 14 February 2020 13:34
> > > > > To: Xen Development List <xen-devel@lists.xenproject.org>
> > > > > Cc: Michael Kelley <mikelley@microsoft.com>; Durrant, Paul
> > > > > <pdurrant@amazon.co.uk>; Wei Liu <liuwe@microsoft.com>; Wei Liu
> > > > > <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> > > > > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>
> > > > > Subject: [PATCH v2 2/3] x86/hyperv: skeleton for L0 assisted TLB flush
> > > > > 
> > > > > Implement a basic hook for L0 assisted TLB flush. The hook needs to
> > > > > check if prerequisites are met. If they are not met, it returns an error
> > > > > number to fall back to native flushes.
> > > > > 
> > > > > Introduce a new variable to indicate if hypercall page is ready.
> > > > > 
> > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com>
> > > > > ---
> > > > >  xen/arch/x86/guest/hyperv/Makefile  |  1 +
> > > > >  xen/arch/x86/guest/hyperv/hyperv.c  | 17 ++++++++++++
> > > > >  xen/arch/x86/guest/hyperv/private.h |  4 +++
> > > > >  xen/arch/x86/guest/hyperv/tlb.c     | 41 +++++++++++++++++++++++++++++
> > > > >  4 files changed, 63 insertions(+)
> > > > >  create mode 100644 xen/arch/x86/guest/hyperv/tlb.c
> > > > > 
> > > > > diff --git a/xen/arch/x86/guest/hyperv/Makefile
> > > > > b/xen/arch/x86/guest/hyperv/Makefile
> > > > > index 68170109a9..18902c33e9 100644
> > > > > --- a/xen/arch/x86/guest/hyperv/Makefile
> > > > > +++ b/xen/arch/x86/guest/hyperv/Makefile
> > > > > @@ -1 +1,2 @@
> > > > >  obj-y += hyperv.o
> > > > > +obj-y += tlb.o
> > > > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> > > > > b/xen/arch/x86/guest/hyperv/hyperv.c
> > > > > index 70f4cd5ae0..f9d1f11ae3 100644
> > > > > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > > > > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > > > > @@ -33,6 +33,8 @@ DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page);
> > > > >  DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist);
> > > > >  DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index);
> > > > > 
> > > > > +static bool __read_mostly hv_hcall_page_ready;
> > > > > +
> > > > >  static uint64_t generate_guest_id(void)
> > > > >  {
> > > > >      union hv_guest_os_id id = {};
> > > > > @@ -119,6 +121,8 @@ static void __init setup_hypercall_page(void)
> > > > >      BUG_ON(!hypercall_msr.enable);
> > > > > 
> > > > >      set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
> > > > 
> > > > Shouldn't this have at least a compiler barrier here?
> > > > 
> > > 
> > > OK. I will add a write barrier here.
> > 
> > Hm, shouldn't such barrier be part of set_fixmap_x itself?
> > 
> > Note that map_pages_to_xen already performs atomic writes.
> 
> I don't mind making things more explicit though. However unlikely, I
> may end up putting something in between set_fixmap_x and setting
> hcall_page_ready, I will need the barrier by then, I may as well put it
> in now.

IMO set_fixmap_x should have the necessary barriers (or other
synchronization methods) so that on return the address is correctly
mapped across all processors, and that it prevents the compiler from
moving accesses past it. I would consider a bug of set_fixmap_x
not having this behavior and requiring callers to do extra work in
order to ensure this.

Ie: something like the snipped below should not require an extra
barrier IMO:

set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);
*((unsigned int *)fix_x_to_virt(FIX_X_HYPERV_HCALL)) = 0;

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel