Re: [PATCH 3/5] x86/PV: restrict guest-induced WBINVD (or alike) to cache writeback

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 22:10, Andrew Cooper wrote:
> On 19/04/2023 11:45 am, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -3772,7 +3772,7 @@ long do_mmuext_op(
>>              else if ( unlikely(!cache_flush_permitted(currd)) )
>>                  rc = -EACCES;
>>              else
>> -                wbinvd();
>> +                wbnoinvd();
>>              break;
> 
> It occurs to me that this is fundamentally broken.
> 
> The guest is not in any position to know (or control) whether it gets
> rescheduled elsewhere between now and it logically continuing execution.
> 
> So if there is actually any cache maintenance to do, it can't be a
> WB{...} of any form on just this CPU alone.

Aren't you merely confirming the respective paragraph in the cover letter
here? (The implication would be that at least the data added in the last
patch wants to be guest-type-independent, so that it can be re-used for
PV, even if maybe not in the same patch.)

>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -238,7 +238,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(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A  (F)X{SAVE,RSTOR} always 
>> saves/restores FPU Error pointers */
>> -XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*S  WBNOINVD instruction */
>> +XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*A  WBNOINVD instruction */
> 
> Given that PV guests do have several real hypercalls for doing this, I'm
> not particularly inclined to let them do it via an emulated path,
> however easy it might be at a technical level.
> 
> I doubt anything good can come from it.

We permit use of WBINVD; it looks inconsistent to me to not also permit
WBNOINVD then. Furthermore exposing the feature then can serve as a
(better) hint that behind the scenes we actually carry out the mmuext
ops as write-back. not evict.

Jan