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

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

On 27/08/2019 16:08, Jan Beulich wrote:
> On 27.08.2019 16:47, Andrew Cooper wrote:
>> On 01/07/2019 12:56, Jan Beulich wrote:
>>> --- 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.
>
> I agree, but wanted to make the code look complete / consistent.
> Would you be okay with the && ... being retained, but in a
> comment?

Ok.

>
>>> --- 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.
>
> It has to stay, as gas rejects use of REP on insns it doesn't think
> permit use of REP. H.J. actually proposes even more strict (or should
> I say hostile) gas behavior, which would then also reject the above
> construct:
> https://sourceware.org/ml/binutils/2019-07/msg00186.html

Oh.  That's dull, especially considering the vendors propensity to
create new instructions by using prefixes in this way.

>
>>> --- 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.
>
> On AMD, but not (so far at least, judging from the SDM) on Intel.

Intel have leaf 4 which specifies WBINVD's behaviour on different cache
levels.  TBH, I'd expect to see an adjustment to the leaf 4
documentation to include WBNOINVD.

AMD explicitly doesn't have leaf 4.  Their leaf 0x8000001d is similar in
behaviour (by having subleafs), and is mostly compatible, but
irritatingly doesn't have an identical data layout.

I've got another query out with AMD because the documentation for this
leaf alone claims that CPUID will #UD with an out-of-range subleaf, and
I don't believe this can be true.

>
>> 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.
>
> I've dropped the A for now, but as per above I'm not entirely
> certain that's appropriate; it's certainly the more defensive step.
> My uncertainty is also because people are free to use the WBNOINVD
> encoding even without the feature flag set, as it won't #UD (as you
> also suggest elsewhere in your reply).

And the emulate behaviour matches, by falling back to WBINVD.

I'd prefer to avoid advertising the feature when we have known work
still to do, because otherwise we will inevitably forget to do it.

Having functionality in the emulator without the feature being
advertised is fine in general, because that still behaves like a
pipeline which has had CPUID levelled down.

~Andrew

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