Re: [PATCH 1/4] x86/hvm: allow guest to use clflushopt and clwb

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/12/2015 11:31, Haozhong Zhang wrote:
> Pass CPU features CLFLUSHOPT and CLWB into HVM domain so that those two
> instructions can be used by guest.
>
> The specification of above two instructions can be found in
> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Please be aware that my cpuid rework series completely changes all of 
this code.   As this patch is small and self contained, it would be best 
to get it accepted early and for me to rebase over the result.

As part of my cpuid work, I had come to the conclusion that CLFLUSHOPT, 
CLWB and PCOMMIT were all safe for all guests to use, as they deemed 
safe for cpl3 code to use.  Is there any reason why these wouldn't be 
safe for PV guests to use?

> ---
>   tools/libxc/xc_cpufeature.h      | 3 ++-
>   tools/libxc/xc_cpuid_x86.c       | 4 +++-
>   xen/arch/x86/hvm/hvm.c           | 7 +++++++
>   xen/include/asm-x86/cpufeature.h | 5 +++++
>   4 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_cpufeature.h b/tools/libxc/xc_cpufeature.h
> index c3ddc80..5288ac6 100644
> --- a/tools/libxc/xc_cpufeature.h
> +++ b/tools/libxc/xc_cpufeature.h
> @@ -140,6 +140,7 @@
>   #define X86_FEATURE_RDSEED      18 /* RDSEED instruction */
>   #define X86_FEATURE_ADX         19 /* ADCX, ADOX instructions */
>   #define X86_FEATURE_SMAP        20 /* Supervisor Mode Access Protection */
> -
> +#define X86_FEATURE_CLFLUSHOPT  23 /* CLFLUSHOPT instruction */
> +#define X86_FEATURE_CLWB        24 /* CLWB instruction */
>   
>   #endif /* __LIBXC_CPUFEATURE_H */
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index 8882c01..fecfd6c 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -426,7 +426,9 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
>                           bitmaskof(X86_FEATURE_RDSEED)  |
>                           bitmaskof(X86_FEATURE_ADX)  |
>                           bitmaskof(X86_FEATURE_SMAP) |
> -                        bitmaskof(X86_FEATURE_FSGSBASE));
> +                        bitmaskof(X86_FEATURE_FSGSBASE) |
> +                        bitmaskof(X86_FEATURE_CLWB) |
> +                        bitmaskof(X86_FEATURE_CLFLUSHOPT));
>           } else
>               regs[1] = 0;
>           regs[0] = regs[2] = regs[3] = 0;

The entry for CLFLUSHOPT in the ISA Extension manual (August 2015) talks 
about CPUID.7(ECX=1).EBX[8:15] indicating the cache line size affected 
by the instruction.  However, I can't find any other reference to this 
information, nor an extension of the CPUID instruction in the ISA 
manual.  Should the Xen cpuid handling code be updated not to clobber this?

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 21470ec..58c83a5 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4598,6 +4598,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>           /* Don't expose INVPCID to non-hap hvm. */
>           if ( (count == 0) && !hap_enabled(d) )
>               *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> +
> +        if ( (count == 0) && !cpu_has_clflushopt )
> +            *ebx &= ~cpufeat_mask(X86_FEATURE_CLFLUSHOPT);
> +
> +        if ( (count == 0) && !cpu_has_clwb )
> +            *ebx &= ~cpufeat_mask(X86_FEATURE_CLWB);

Please refactor this code along with if() in context above, to only 
check count once.

~Andrew