Re: [Xen-devel] [PATCH 2/6] x86emul: support WBNOINVD

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

On 01/07/2019 12:56, Jan Beulich wrote:
> Rev 035 of Intel's ISA extensions document does not state intercept
> behavior for the insn (I've been in-officially told that the distinction

unofficially.

> is going to be by exit qualification, as I would have assumed
> considering that this way it's sufficiently transparent to unaware
> software, and using WBINVD in place of WBNOINVD is always correct, just
> less efficient), so in the HVM case for now it'll be backed by the same
> ->wbinvd_intercept() handlers.

It turns out that AMD reuses the WBINVD vmexit code for WBNOINVD, and
provide no distinguishing information.  There may or may not be an
instruction stream to inspect, depending on other errata.

I have a question out with AMD as to what to do here, but in the
meantime we have no option but to assume WBINVD.

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -1121,7 +1121,7 @@ static int write_msr(unsigned int reg, u
> @@ -1130,6 +1130,8 @@ static int cache_op(enum x86emul_cache_o
>            * newer linux uses this in some start-of-day timing loops.
>            */
>           ;
> +    else if ( op == x86emul_wbnoinvd && cpu_has_wbnoinvd )
> +        wbnoinvd();
>       else
>           wbinvd();

The cpu_has_wbnoinvd check isn't necessary.  The encoding was chosen
because it does get interpreted as wbinvd on older processors.

Given this property, I expect kernels to perform a blanked
s/wbinvd/wbnoinvd/ transformation in appropriate locations, because it
is the lowest overhead option to start using this new feature.

> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -16,6 +16,11 @@ static inline void wbinvd(void)
>       asm volatile ( "wbinvd" ::: "memory" );
>   }
>   
> +static inline void wbnoinvd(void)
> +{
> +    asm volatile ( "repe; wbinvd" : : : "memory" );

Semicolon.

> +}
> +
>   static inline void clflush(const void *p)
>   {
>       asm volatile ( "clflush %0" :: "m" (*(const char *)p) );
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -243,6 +244,7 @@ XEN_CPUFEATURE(EFRO,          7*32+10) /
>   
>   /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
>   XEN_CPUFEATURE(CLZERO,        8*32+ 0) /*A  CLZERO instruction */
> +XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*A  WBNOINVD instruction */

This is implicitly linked with CPUID.8000001d which we don't expose yet.

To get the emulation side of things sorted, I'd be happy with this going
in without the A for now, and "exposing WBNOINVD to guests" can be a
followup task.

~Andrew

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