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

[Wei Liu <wl@xxxxxxx>]

On Mon, Feb 17, 2020 at 12:21:09PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Sent: 17 February 2020 12:08
> > To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>; Xen Development List <xen-
> > devel@xxxxxxxxxxxxxxxxxxxx>; Michael Kelley <mikelley@xxxxxxxxxxxxx>; Wei
> > Liu <liuwe@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx>
> > Subject: Re: [PATCH v2 2/3] x86/hyperv: skeleton for L0 assisted TLB flush
> > 
> > On Mon, Feb 17, 2020 at 12:01:23PM +0000, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > Sent: 17 February 2020 11:41
> > > > To: Wei Liu <wl@xxxxxxx>
> > > > Cc: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; Xen Development List <xen-
> > > > devel@xxxxxxxxxxxxxxxxxxxx>; Michael Kelley <mikelley@xxxxxxxxxxxxx>;
> > Wei
> > > > Liu <liuwe@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew
> > Cooper
> > > > <andrew.cooper3@xxxxxxxxxx>
> > > > Subject: Re: [PATCH v2 2/3] x86/hyperv: skeleton for L0 assisted TLB
> > flush
> > > >
> > > > 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@xxxxxxxxx> On Behalf Of Wei Liu
> > > > > > > Sent: 14 February 2020 13:34
> > > > > > > To: Xen Development List <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> > > > > > > Cc: Michael Kelley <mikelley@xxxxxxxxxxxxx>; Durrant, Paul
> > > > > > > <pdurrant@xxxxxxxxxxxx>; Wei Liu <liuwe@xxxxxxxxxxxxx>; Wei Liu
> > > > > > > <wl@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> > > > > > > <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné
> > <roger.pau@xxxxxxxxxx>
> > > > > > > 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@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >  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?
> > > >
> > >
> > > Not really, for the purpose I had in mind. The hv_hcall_page_ready
> > global is specific to this code and we need to make sure the page is
> > actually ready before the code says it is.
> > 
> > But anything that modifies the page tables should already have a
> > barrier if required in order to prevent accesses from being moved
> > ahead of it, or else things would certainly go wrong in many other
> > places?
> 
> Oh. I'm not saying that we don't need a barrier there too (and more
> than a compiler one in that case).
> 

The argument Roger has is that set_fixmap_x also contains strong enough
barriers to prevent hcall_page_ready to be set before page table is
correctly set up.

Since you asked for it, there must be something on your mind that
prompted this (maybe it is simply because you were bitten by similar
things and wants to be extra sure, maybe you think it is harder to grasp
the side effect of set_fixmap_x, maybe something else).

Code is written to be read by humans after all. I would rather be more
explicit / redundant to make humans happy than to save a potential
barrier / some typing in a code path.

Wei.

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