Re: [Xen-devel] [PATCH] x86: invpcid support

[Juergen Gross <jgross@xxxxxxxx>]

On 02/03/18 15:47, Wei Liu wrote:
> Provide the functions needed for different modes. And cpu_has_invpcid.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> This is useful for Juergen's XPTI improvement series.
> 
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> ---
>  xen/arch/x86/Rules.mk            |  1 +
>  xen/include/asm-x86/cpufeature.h |  1 +
>  xen/include/asm-x86/invpcid.h    | 75 
> ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
>  create mode 100644 xen/include/asm-x86/invpcid.h
> 
> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> index 9897deaab9..c941059f42 100644
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -23,6 +23,7 @@ $(call as-option-add,CFLAGS,CC,"rdseed 
> %eax",-DHAVE_GAS_RDSEED)
>  $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
>                       -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
>                       '-D__OBJECT_LABEL__=$(subst 
> $(BASEDIR)/,,$(CURDIR))/$$@')
> +$(call as-insn-check,CFLAGS,CC,"invpcid 
> (%rax)$$(comma)%rax",-DHAVE_GAS_INVPCID)
>  
>  CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
>  
> diff --git a/xen/include/asm-x86/cpufeature.h 
> b/xen/include/asm-x86/cpufeature.h
> index 55b696ed07..db8072279d 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -93,6 +93,7 @@
>  #define cpu_has_avx2            boot_cpu_has(X86_FEATURE_AVX2)
>  #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
>  #define cpu_has_bmi2            boot_cpu_has(X86_FEATURE_BMI2)
> +#define cpu_has_invpcid         boot_cpu_has(X86_FEATURE_INVPCID)
>  #define cpu_has_rtm             boot_cpu_has(X86_FEATURE_RTM)
>  #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
>  #define cpu_has_mpx             boot_cpu_has(X86_FEATURE_MPX)
> diff --git a/xen/include/asm-x86/invpcid.h b/xen/include/asm-x86/invpcid.h
> new file mode 100644
> index 0000000000..65a54eab4d
> --- /dev/null
> +++ b/xen/include/asm-x86/invpcid.h
> @@ -0,0 +1,75 @@
> +#ifndef _ASM_X86_INVPCID_H_
> +#define _ASM_X86_INVPCID_H_
> +
> +#include <xen/types.h>
> +
> +#define INVPCID_TYPE_INDIV_ADDR      0
> +#define INVPCID_TYPE_SINGLE_CTXT     1
> +#define INVPCID_TYPE_ALL_INCL_GLOBAL 2
> +#define INVPCID_TYPE_ALL_NON_GLOBAL  3
> +
> +struct invpcid_desc {
> +    uint64_t pcid:12;
> +    uint64_t reserved:52;
> +    uint64_t addr;
> +};
> +
> +#define INVPCID_OPCODE ".byte 0x66, 0x0f, 0x38, 0x82\n"
> +#define MODRM_ECX_01   ".byte 0x01\n"
> +
> +static inline void invpcid(unsigned long pcid, unsigned long addr,
> +                           unsigned long type)
> +{
> +    struct invpcid_desc desc = { .pcid = pcid, .addr = addr };

As Andrew already wrote: define the structure just here.

> +
> +    asm volatile (
> +#ifdef HAVE_GAS_INVPCID
> +                  "invpcid %[desc], %[type]"
> +#else
> +                  INVPCID_OPCODE MODRM_ECX_01
> +#endif
> +                  :
> +#ifdef HAVE_GAS_INVPCID
> +                  : [desc] "m" (desc), [type] "r" (type)
> +#else
> +                  : "a" (type), "c" (&desc)
> +#endif
> +                  : "memory" );

I think this is hard to read. I'd spare one #ifdef and have rather a
redundant ":".

With those remarks addressed you can add my:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

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