[v2,2/3] x86/hyperv: skeleton for L0 assisted TLB flush

Commit Message

Wei Liu Feb. 14, 2020, 12:34 p.m. UTC

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 <[e-mail]>
---
 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

Comments

Roger Pau Monné Feb. 14, 2020, 2:46 p.m. UTC | #1

On Fri, Feb 14, 2020 at 12:34:29PM +0000, Wei Liu wrote:
> 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 <[e-mail]>

Reviewed-by: Roger Pau Monné <[e-mail]>

Just one comment below.

> ---
>  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;

Since this is static, I would drop the hv_ prefix.

Thanks, Roger.

Durrant, Paul Feb. 14, 2020, 4:55 p.m. UTC | #2

> -----Original Message-----
> From: Wei Liu <[e-mail]> On Behalf Of Wei Liu
> Sent: 14 February 2020 13:34
> To: Xen Development List <[e-mail]>
> Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> 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 <[e-mail]>
> ---
>  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?

  Paul

> +
> +    hv_hcall_page_ready = true;
>  }
> 
>  static int setup_hypercall_pcpu_arg(void)
> @@ -199,11 +203,24 @@ static void __init e820_fixup(struct e820map *e820)
>          panic("Unable to reserve Hyper-V hypercall range\n");
>  }
> 
> +static int flush_tlb(const cpumask_t *mask, const void *va,
> +                     unsigned int flags)
> +{
> +    if ( !(ms_hyperv.hints & HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED) )
> +        return -EOPNOTSUPP;
> +
> +    if ( !hv_hcall_page_ready || !this_cpu(hv_input_page) )
> +        return -ENXIO;
> +
> +    return hyperv_flush_tlb(mask, va, flags);
> +}
> +
>  static const struct hypervisor_ops __initdata ops = {
>      .name = "Hyper-V",
>      .setup = setup,
>      .ap_setup = ap_setup,
>      .e820_fixup = e820_fixup,
> +    .flush_tlb = flush_tlb,
>  };
> 
>  /*
> diff --git a/xen/arch/x86/guest/hyperv/private.h
> b/xen/arch/x86/guest/hyperv/private.h
> index 956eff831f..509bedaafa 100644
> --- a/xen/arch/x86/guest/hyperv/private.h
> +++ b/xen/arch/x86/guest/hyperv/private.h
> @@ -22,10 +22,14 @@
>  #ifndef __XEN_HYPERV_PRIVIATE_H__
>  #define __XEN_HYPERV_PRIVIATE_H__
> 
> +#include <xen/cpumask.h>
>  #include <xen/percpu.h>
> 
>  DECLARE_PER_CPU(void *, hv_input_page);
>  DECLARE_PER_CPU(void *, hv_vp_assist);
>  DECLARE_PER_CPU(unsigned int, hv_vp_index);
> 
> +int hyperv_flush_tlb(const cpumask_t *mask, const void *va,
> +                     unsigned int flags);
> +
>  #endif /* __XEN_HYPERV_PRIVIATE_H__  */
> diff --git a/xen/arch/x86/guest/hyperv/tlb.c
> b/xen/arch/x86/guest/hyperv/tlb.c
> new file mode 100644
> index 0000000000..48f527229e
> --- /dev/null
> +++ b/xen/arch/x86/guest/hyperv/tlb.c
> @@ -0,0 +1,41 @@
> +/************************************************************************
> ******
> + * arch/x86/guest/hyperv/tlb.c
> + *
> + * Support for TLB management using hypercalls
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2020 Microsoft.
> + */
> +
> +#include <xen/cpumask.h>
> +#include <xen/errno.h>
> +
> +#include "private.h"
> +
> +int hyperv_flush_tlb(const cpumask_t *mask, const void *va,
> +                     unsigned int flags)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.20.1

Wei Liu Feb. 17, 2020, 11:34 a.m. UTC | #3

On Fri, Feb 14, 2020 at 04:55:44PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Wei Liu <[e-mail]> On Behalf Of Wei Liu
> > Sent: 14 February 2020 13:34
> > To: Xen Development List <[e-mail]>
> > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > 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 <[e-mail]>
> > ---
> >  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.

Wei.

Roger Pau Monné Feb. 17, 2020, 11:40 a.m. UTC | #4

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 <[e-mail]> On Behalf Of Wei Liu
> > > Sent: 14 February 2020 13:34
> > > To: Xen Development List <[e-mail]>
> > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > 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 <[e-mail]>
> > > ---
> > >  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.

Roger.

Wei Liu Feb. 17, 2020, 11:45 a.m. UTC | #5

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 <[e-mail]> On Behalf Of Wei Liu
> > > > Sent: 14 February 2020 13:34
> > > > To: Xen Development List <[e-mail]>
> > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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.

Wei.

> 
> Roger.

Roger Pau Monné Feb. 17, 2020, noon UTC | #6

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 <[e-mail]> On Behalf Of Wei Liu
> > > > > Sent: 14 February 2020 13:34
> > > > > To: Xen Development List <[e-mail]>
> > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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.

Durrant, Paul Feb. 17, 2020, 12:01 p.m. UTC | #7

> -----Original Message-----
> From: Roger Pau Monné <[e-mail]>
> Sent: 17 February 2020 11:41
> To: Wei Liu <[e-mail]>
> Cc: Durrant, Paul <[e-mail]>; Xen Development List <xen-
> [e-mail]>; Michael Kelley <[e-mail]>; Wei
> Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> <[e-mail]>
> 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 <[e-mail]> On Behalf Of Wei Liu
> > > > Sent: 14 February 2020 13:34
> > > > To: Xen Development List <[e-mail]>
> > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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.

  Paul

> Note that map_pages_to_xen already performs atomic writes.
> 
> Roger.

Wei Liu Feb. 17, 2020, 12:08 p.m. UTC | #8

On Mon, Feb 17, 2020 at 01:00:54PM +0100, Roger Pau Monné wrote:
> 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > Sent: 14 February 2020 13:34
> > > > > > To: Xen Development List <[e-mail]>
> > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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;

That's different though. Compiler can't make the connection between
hcall_page_ready and the address returned by set_fixmap_x.

Wei.

> 
> Roger.

Roger Pau Monné Feb. 17, 2020, 12:08 p.m. UTC | #9

On Mon, Feb 17, 2020 at 12:01:23PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <[e-mail]>
> > Sent: 17 February 2020 11:41
> > To: Wei Liu <[e-mail]>
> > Cc: Durrant, Paul <[e-mail]>; Xen Development List <xen-
> > [e-mail]>; Michael Kelley <[e-mail]>; Wei
> > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > <[e-mail]>
> > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > Sent: 14 February 2020 13:34
> > > > > To: Xen Development List <[e-mail]>
> > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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?

Roger.

Roger Pau Monné Feb. 17, 2020, 12:13 p.m. UTC | #10

On Mon, Feb 17, 2020 at 12:08:01PM +0000, Wei Liu wrote:
> On Mon, Feb 17, 2020 at 01:00:54PM +0100, Roger Pau Monné wrote:
> > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > > Sent: 14 February 2020 13:34
> > > > > > > To: Xen Development List <[e-mail]>
> > > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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;
> 
> That's different though. Compiler can't make the connection between
> hcall_page_ready and the address returned by set_fixmap_x.

I'm not sure the compiler can make a connection between set_fixmap_x
and fix_x_to_virt either (as fix_x_to_virt is a simple mathematical
operation and FIX_X_HYPERV_HCALL is a constant known at build time).

Roger.

Durrant, Paul Feb. 17, 2020, 12:21 p.m. UTC | #11

> -----Original Message-----
> From: Roger Pau Monné <[e-mail]>
> Sent: 17 February 2020 12:08
> To: Durrant, Paul <[e-mail]>
> Cc: Wei Liu <[e-mail]>; Xen Development List <xen-
> [e-mail]>; Michael Kelley <[e-mail]>; Wei
> Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> <[e-mail]>
> 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é <[e-mail]>
> > > Sent: 17 February 2020 11:41
> > > To: Wei Liu <[e-mail]>
> > > Cc: Durrant, Paul <[e-mail]>; Xen Development List <xen-
> > > [e-mail]>; Michael Kelley <[e-mail]>;
> Wei
> > > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew
> Cooper
> > > <[e-mail]>
> > > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > Sent: 14 February 2020 13:34
> > > > > > To: Xen Development List <[e-mail]>
> > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > <[e-mail]>; Roger Pau Monné
> <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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).

  Paul

> 
> Roger.

Wei Liu Feb. 17, 2020, 12:38 p.m. UTC | #12

On Mon, Feb 17, 2020 at 01:13:28PM +0100, Roger Pau Monné wrote:
> On Mon, Feb 17, 2020 at 12:08:01PM +0000, Wei Liu wrote:
> > On Mon, Feb 17, 2020 at 01:00:54PM +0100, Roger Pau Monné wrote:
> > > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > > > Sent: 14 February 2020 13:34
> > > > > > > > To: Xen Development List <[e-mail]>
> > > > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > > > <[e-mail]>; Roger Pau Monné <[e-mail]>
> > > > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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;
> > 
> > That's different though. Compiler can't make the connection between
> > hcall_page_ready and the address returned by set_fixmap_x.
> 
> I'm not sure the compiler can make a connection between set_fixmap_x
> and fix_x_to_virt either (as fix_x_to_virt is a simple mathematical
> operation and FIX_X_HYPERV_HCALL is a constant known at build time).

Oh, I misread your example, sorry.

Wei.

> 
> Roger.

Wei Liu Feb. 17, 2020, 12:48 p.m. UTC | #13

On Mon, Feb 17, 2020 at 12:21:09PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <[e-mail]>
> > Sent: 17 February 2020 12:08
> > To: Durrant, Paul <[e-mail]>
> > Cc: Wei Liu <[e-mail]>; Xen Development List <xen-
> > [e-mail]>; Michael Kelley <[e-mail]>; Wei
> > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > <[e-mail]>
> > 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é <[e-mail]>
> > > > Sent: 17 February 2020 11:41
> > > > To: Wei Liu <[e-mail]>
> > > > Cc: Durrant, Paul <[e-mail]>; Xen Development List <xen-
> > > > [e-mail]>; Michael Kelley <[e-mail]>;
> > Wei
> > > > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew
> > Cooper
> > > > <[e-mail]>
> > > > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > > Sent: 14 February 2020 13:34
> > > > > > > To: Xen Development List <[e-mail]>
> > > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei Liu
> > > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > > <[e-mail]>; Roger Pau Monné
> > <[e-mail]>
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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.

Durrant, Paul Feb. 17, 2020, 12:56 p.m. UTC | #14

> -----Original Message-----
> From: Wei Liu <[e-mail]>
> Sent: 17 February 2020 12:48
> To: Durrant, Paul <[e-mail]>
> Cc: Roger Pau Monné <[e-mail]>; Wei Liu <[e-mail]>; Xen
> Development List <[e-mail]>; Michael Kelley
> <[e-mail]>; Wei Liu <[e-mail]>; Jan Beulich
> <[e-mail]>; Andrew Cooper <[e-mail]>
> Subject: Re: [PATCH v2 2/3] x86/hyperv: skeleton for L0 assisted TLB flush
> 
> On Mon, Feb 17, 2020 at 12:21:09PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monné <[e-mail]>
> > > Sent: 17 February 2020 12:08
> > > To: Durrant, Paul <[e-mail]>
> > > Cc: Wei Liu <[e-mail]>; Xen Development List <xen-
> > > [e-mail]>; Michael Kelley <[e-mail]>;
> Wei
> > > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew
> Cooper
> > > <[e-mail]>
> > > 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é <[e-mail]>
> > > > > Sent: 17 February 2020 11:41
> > > > > To: Wei Liu <[e-mail]>
> > > > > Cc: Durrant, Paul <[e-mail]>; Xen Development List
> <xen-
> > > > > [e-mail]>; Michael Kelley
> <[e-mail]>;
> > > Wei
> > > > > Liu <[e-mail]>; Jan Beulich <[e-mail]>; Andrew
> > > Cooper
> > > > > <[e-mail]>
> > > > > 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 <[e-mail]> On Behalf Of Wei Liu
> > > > > > > > Sent: 14 February 2020 13:34
> > > > > > > > To: Xen Development List <[e-mail]>
> > > > > > > > Cc: Michael Kelley <[e-mail]>; Durrant, Paul
> > > > > > > > <[e-mail]>; Wei Liu <[e-mail]>; Wei
> Liu
> > > > > > > > <[e-mail]>; Jan Beulich <[e-mail]>; Andrew Cooper
> > > > > > > > <[e-mail]>; Roger Pau Monné
> > > <[e-mail]>
> > > > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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.

If set_fixmap_x() is a barriering operation, and that is reasonably obvious (from comments or cursory examination of the code) then that's fine. I have indeed been bitten by this kind of thing in writing PV drivers so just wanted to ensure we didn't run into it here.

  Paul

> 
> Wei.

Patch

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);
+
+    hv_hcall_page_ready = true;
 }
 
 static int setup_hypercall_pcpu_arg(void)
@@ -199,11 +203,24 @@  static void __init e820_fixup(struct e820map *e820)
         panic("Unable to reserve Hyper-V hypercall range\n");
 }
 
+static int flush_tlb(const cpumask_t *mask, const void *va,
+                     unsigned int flags)
+{
+    if ( !(ms_hyperv.hints & HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED) )
+        return -EOPNOTSUPP;
+
+    if ( !hv_hcall_page_ready || !this_cpu(hv_input_page) )
+        return -ENXIO;
+
+    return hyperv_flush_tlb(mask, va, flags);
+}
+
 static const struct hypervisor_ops __initdata ops = {
     .name = "Hyper-V",
     .setup = setup,
     .ap_setup = ap_setup,
     .e820_fixup = e820_fixup,
+    .flush_tlb = flush_tlb,
 };
 
 /*
diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h
index 956eff831f..509bedaafa 100644
--- a/xen/arch/x86/guest/hyperv/private.h
+++ b/xen/arch/x86/guest/hyperv/private.h
@@ -22,10 +22,14 @@ 
 #ifndef __XEN_HYPERV_PRIVIATE_H__
 #define __XEN_HYPERV_PRIVIATE_H__
 
+#include <xen/cpumask.h>
 #include <xen/percpu.h>
 
 DECLARE_PER_CPU(void *, hv_input_page);
 DECLARE_PER_CPU(void *, hv_vp_assist);
 DECLARE_PER_CPU(unsigned int, hv_vp_index);
 
+int hyperv_flush_tlb(const cpumask_t *mask, const void *va,
+                     unsigned int flags);
+
 #endif /* __XEN_HYPERV_PRIVIATE_H__  */
diff --git a/xen/arch/x86/guest/hyperv/tlb.c b/xen/arch/x86/guest/hyperv/tlb.c
new file mode 100644
index 0000000000..48f527229e
--- /dev/null
+++ b/xen/arch/x86/guest/hyperv/tlb.c
@@ -0,0 +1,41 @@ 
+/******************************************************************************
+ * arch/x86/guest/hyperv/tlb.c
+ *
+ * Support for TLB management using hypercalls
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2020 Microsoft.
+ */
+
+#include <xen/cpumask.h>
+#include <xen/errno.h>
+
+#include "private.h"
+
+int hyperv_flush_tlb(const cpumask_t *mask, const void *va,
+                     unsigned int flags)
+{
+    return -EOPNOTSUPP;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */