Re: [Xen-devel] [PATCH v2 1/3] x86/hypervisor: pass flags to hypervisor_flush_tlb

2020-02-14 Thread Durrant, Paul
> -Original Message-
> From: Wei Liu  On Behalf Of Wei Liu
> Sent: 14 February 2020 13:34
> To: Xen Development List 
> Cc: Michael Kelley ; Durrant, Paul
> ; Wei Liu ; Jan Beulich
> ; Andrew Cooper ; Wei Liu
> ; Roger Pau Monné 
> Subject: [PATCH v2 1/3] x86/hypervisor: pass flags to hypervisor_flush_tlb
> 
> 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 

Reviewed-by: Paul Durrant 

> ---
> v2:
> 1. Introduce FLUSH_TLB_FLAGS_MASK
> ---
>  xen/arch/x86/guest/hypervisor.c|  7 +--
>  xen/arch/x86/guest/xen/xen.c   |  2 +-
>  xen/arch/x86/smp.c |  5 ++---
>  xen/include/asm-x86/flushtlb.h |  3 +++
>  xen/include/asm-x86/guest/hypervisor.h | 10 +-
>  5 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/guest/hypervisor.c
> b/xen/arch/x86/guest/hypervisor.c
> index 47e938e287..6ee28c9df1 100644
> --- a/xen/arch/x86/guest/hypervisor.c
> +++ b/xen/arch/x86/guest/hypervisor.c
> @@ -75,10 +75,13 @@ 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 ( flags & ~FLUSH_TLB_FLAGS_MASK )
> +return -EINVAL;
> +
>  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..2ab0e30eef 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -258,9 +258,8 @@ void flush_area_mask(const cpumask_t *mask, const void
> *va, unsigned int flags)
>   !cpumask_subset(mask, cpumask_of(cpu)) )
>  {
>  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) )
> + !(flags & ~FLUSH_TLB_FLAGS_MASK) &&
> + !hypervisor_flush_tlb(mask, va, flags) )
>  {
>  if ( tlb_clk_enabled )
>  tlb_clk_enabled = false;
> diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-
> x86/flushtlb.h
> index 9773014320..a4de317452 100644
> --- a/xen/include/asm-x86/flushtlb.h
> +++ b/xen/include/asm-x86/flushtlb.h
> @@ -123,6 +123,9 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long
> cr4);
>   /* Flush all HVM guests linear TLB (using ASID/VPID) */
>  #define FLUSH_GUESTS_TLB 0x4000
> 
> +#define FLUSH_TLB_FLAGS_MASK (FLUSH_TLB | FLUSH_TLB_GLOBAL |
> FLUSH_VA_VALID | \
> +  FLUSH_ORDER_MASK)
> +
>  /* Flush local TLBs/caches. */
>  unsigned int flush_area_local(const void *va, unsigned int flags);
>  #define flush_local(flags) flush_area_local(NULL, flags)
> 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;
> }
>  sta

Re: [Xen-devel] [PATCH v2 1/3] x86/hypervisor: pass flags to hypervisor_flush_tlb

2020-02-14 Thread Roger Pau Monné
On Fri, Feb 14, 2020 at 12:34:28PM +, 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.
> 
> Signed-off-by: Wei Liu 

LGTM:

Reviewed-by: Roger Pau Monné 

Just one comment below.

> ---
> v2:
> 1. Introduce FLUSH_TLB_FLAGS_MASK
> ---
>  xen/arch/x86/guest/hypervisor.c|  7 +--
>  xen/arch/x86/guest/xen/xen.c   |  2 +-
>  xen/arch/x86/smp.c |  5 ++---
>  xen/include/asm-x86/flushtlb.h |  3 +++
>  xen/include/asm-x86/guest/hypervisor.h | 10 +-
>  5 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
> index 47e938e287..6ee28c9df1 100644
> --- a/xen/arch/x86/guest/hypervisor.c
> +++ b/xen/arch/x86/guest/hypervisor.c
> @@ -75,10 +75,13 @@ 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 ( flags & ~FLUSH_TLB_FLAGS_MASK )

I think an ASSERT_UNREACHABLE() would be good here, since you are not
supposed to call hypervisor_flush_tlb with non TLB related flags.

Thanks, Roger.

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