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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/06/2023 11:20 am, Jan Beulich wrote:
> On 01.06.2023 16:48, Andrew Cooper wrote:
>> The RSBA bit, "RSB Alternative", means that the RSB may use alternative
>> predictors when empty.  From a practical point of view, this mean "Retpoline
>> not safe".
>>
>> Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
>> statement that IBRS is implemented in hardware (as opposed to the form
>> retrofitted to existing CPUs in microcode).
>>
>> The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
>> property that predictions are tagged with the mode in which they were learnt.
>> Therefore, it means "when eIBRS is active, the RSB may fall back to
>> alternative predictors but restricted to the current prediction mode".  As
>> such, it's stronger statement than RSBA, but still means "Retpoline not 
>> safe".
>>
>> CPUs are not expected to enumerate both RSBA and RRSBA.
>>
>> Add feature dependencies for EIBRS and RRSBA.  While technically they're not
>> linked, absolutely nothing good can of letting the guest see RRSBA without
> Nit: missing "come"?

Yes.  Will fix.

>> @@ -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.

There's quite a bit of other cleanup which ought to be done, like
uniformly adding new bits first, then taking bits away (I suffered two
bugs in init_dom0_cpuid_policy() getting this wrong during development),
so I was planning to leave any decisions until then.

> What about a tool stack request leading to us setting the 2nd of the two
> bits here, while the other was already set? IOW wouldn't we better clear
> the other bit explicitly? (Due to the EIBRS dependency or RRSBA I think
> this can really only happen when the tool stack requests RSBA+EIBRS, as
> the deep deps clearing doesn't know the concept of "negative"
> dependencies.)

Hmm - I think there is a bug here, but it's not this simple.  I think
the only reasonable thing we can do is start rejecting bad input because
I don't think Xen can fix up safely.

Xen must not ever clear RSBA, or we've potentially made the VM unsafe
behind the toolstack's back.

If EIBRS != RRSBA, the toolstack has made a mistake.  Equally too for
RSBA && EIBRS.

I think this is going to take more coffee to solve...

> 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.)

~Andrew