Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2023 15:25, Andrew Cooper wrote:
> On 30/05/2023 10:40 am, Jan Beulich wrote:
>> On 26.05.2023 13:06, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu-policy.c
>>> +++ b/xen/arch/x86/cpu-policy.c
>>> @@ -423,8 +423,14 @@ static void __init 
>>> guest_common_max_feature_adjustments(uint32_t *fs)
>>>           * Retpoline not safe)", so these need to be visible to a guest in 
>>> all
>>>           * cases, even when it's only some other server in the pool which
>>>           * suffers the identified behaviour.
>>> +         *
>>> +         * We can always run any VM which has previously (or will
>>> +         * subsequently) run on hardware where Retpoline is not safe.  
>>> Note:
>>> +         * The dependency logic may hide RRSBA for other reasons.
>>>           */
>>>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
>>> +        __set_bit(X86_FEATURE_RSBA, fs);
>>> +        __set_bit(X86_FEATURE_RRSBA, fs);
>>>      }
>>>  }
>> Similar question to what I raised before: Can't this lead to both bits being
>> set, which according to descriptions earlier in the series and elsewhere
>> ought to not be possible?
> 
> In this case, no, because the max values are fully discarded when
> establishing the default policy.
> 
> Remember this value is used to decide whether an incoming VM can be
> accepted.  It doesn't reflect a sensible configuration to run.

Right. I should have dropped the question when seeing the "default"
counterpart's behavior.

> Whether or not both values ought to be visible is the subject of the
> outstanding question.

Pending the answer there (and whichever easy adjustment)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

>>> --- a/xen/tools/gen-cpuid.py
>>> +++ b/xen/tools/gen-cpuid.py
>>> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>>>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating 
>>> them
>>>          # as dependent features simplifies Xen's logic, and prevents the 
>>> guest
>>>          # from seeing implausible configurations.
>>> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
>>> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
>> Is this really an architecturally established dependency? From an abstract
>> pov having just eIBRS ought to be enough of an indicator?
> 
> This is the same as asking "can we hide AVX512F but expose AVX512_*"...
> 
>> And hence it would
>> be wrong to hide it just because IBRSB isn't also set.
> 
> EIBRS means "you should set MSR_SPEC_CTRL.IBRS once at the start of day
> and leave it set", which to me firmly states a dependency.

Hmm, yes, now that you put it that way, I agree.

Jan