Re: [PATCH v2 3/3] x86/cpu-policy: Derive RSBA/RRSBA for guest policies

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.06.2023 17:29, Andrew Cooper wrote:
> On 02/06/2023 11:20 am, Jan Beulich wrote:
>> On 01.06.2023 16:48, Andrew Cooper wrote:
>>> @@ -786,6 +825,20 @@ void recalculate_cpuid_policy(struct domain *d)
>>>  
>>>      sanitise_featureset(fs);
>>>  
>>> +    /*
>>> +     * If the host suffers from RSBA of any form, and the guest can see
>>> +     * MSR_ARCH_CAPS, reflect the appropriate RSBA/RRSBA property to the 
>>> guest
>>> +     * depending on the visibility of eIBRS.
>>> +     */
>>> +    if ( test_bit(X86_FEATURE_ARCH_CAPS, fs) &&
>>> +         (cpu_has_rsba || cpu_has_rrsba) )
>>> +    {
>>> +        bool eibrs = test_bit(X86_FEATURE_EIBRS, fs);
>>> +
>>> +        __set_bit(eibrs ? X86_FEATURE_RRSBA
>>> +                        : X86_FEATURE_RSBA, fs);
>>> +    }
>> Now that we have the same code and comment even a 3rd time, surely this
>> wants to be put in a helper?
> 
> I did consider that, and chose not to in this case.
> 
> One of these is going to disappear again in due course, when we start
> handing errors back to the toolstack instead of fixing up behind it.
> 
> The requirement to be after sanitise_featureset() is critically
> important here for safety, and out-of-lining makes that connection less
> obvious.
> 
> I considered having guest_common_default_late_feature_adjustments(), but
> that name is getting silly and it's already somewhat hard to navigate.

Well, okay then. But may I then please ask that the three instances each
cross-reference one another, so touching one will stand a chance of the
others also being touched?

>> Similarly what about a tool stack (and ultimately admin) request setting
>> both RSBA and RRSBA? Wouldn't we better clear the respectively wrong bit
>> then? People may do this in their guest configs "just to be on the safe
>> side" (once expressing this in guest configs is possible, of course),
>> and due to the max policies having both bits set this also won't occur
>> "automatically".
> 
> The only reason this series doesn't have a final patch turning
> ARCH_CAPS's "a" into "A" is because libxl can't currently operate these
> bits at all, let alone safely.  Roger is kindly looking into that side
> of things.
> 
> It is an error to be modifying bits behind the toolstack's back to start
> with.  We get away with it previously because hiding bits that the
> toolstack thinks the guest saw is goes in the safe direction WRT
> migrate.  But no more with the semantics of RSBA/RRSBA.
> 
> I explicitly don't care about people wanting to set RSBA && RRSBA "just
> in case" - this is too complicated already.  The only non-default thing
> an admin needs to be able to express is +rsba,-eibrs,-rrsba to mean
> "please be compatible with pre-EIBRS hardware".  (In reality, there will
> also need to be some FOO_NO bits taken out too, depending on the CPUs in
> question.)

Not caring about such people would mean documenting very clearly that the
two bits used together is not supported (and then also their respective
invalid combinations with EIBRS). Otherwise I'm afraid "such people" would
likely include one of our bigger customers.

Jan