Re: [Xen-devel] [PATCH v9 02/11] x86/cpuid: Handling of IBRS/IBPB, STIBP and IBRS for guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/01/18 10:40, Jan Beulich wrote:
>>>> On 18.01.18 at 16:46, <andrew.cooper3@xxxxxxxxxx> wrote:
>> For guest safety, we treat STIBP as special, always override the toolstack
>> choice, and always advertise STIBP if IBRS is available.  This removes the
>> corner case where STIBP is not advertised, but the guest is running on
>> HT-capable hardware where it does matter.
> I guess the answer to my question may live somewhere later in the
> series, but since I haven't got there yet: Is this based on the
> assumption that on HT-capable hardware they would always be
> available together? Otherwise, how do you emulate STIBP for the
> guest if all you've got is IBRS/IBPB?

The safety depends on the guest seeing STIBP and using it if it wants
to.  (Not that I've seen any sign of STIBP in the Linux code, or from
observing what Windows appears to do).

For topology reasons (despite the other cans of worms in this area), we
unilaterally set HT, so all guests should find themselves on HT-capable
systems.

>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -383,6 +383,16 @@ static void __init calculate_pv_max_policy(void)
>>      /* Unconditionally claim to be able to set the hypervisor bit. */
>>      __set_bit(X86_FEATURE_HYPERVISOR, pv_featureset);
>>  
>> +    /* On hardware with IBRS/IBPB support, there are further adjustments. */
>> +    if ( test_bit(X86_FEATURE_IBRSB, pv_featureset) )
>> +    {
>> +        /* Offer STIBP unconditionally.  It is a nop on non-HT hardware. */
>> +        __set_bit(X86_FEATURE_STIBP, pv_featureset);
>> +
>> +        /* AMD's IBPB is a subset of IBRS/IBPB. */
>> +        __set_bit(X86_FEATURE_IBPB, pv_featureset);
>> +    }
>> +
>>      sanitise_featureset(pv_featureset);
>>      cpuid_featureset_to_policy(pv_featureset, p);
>>      recalculate_xstate(p);
>> @@ -440,6 +450,16 @@ static void __init calculate_hvm_max_policy(void)
>>              __clear_bit(X86_FEATURE_XSAVES, hvm_featureset);
>>      }
>>  
>> +    /* On hardware with IBRS/IBPB support, there are further adjustments. */
>> +    if ( test_bit(X86_FEATURE_IBRSB, hvm_featureset) )
>> +    {
>> +        /* Offer STIBP unconditionally.  It is a nop on non-HT hardware. */
>> +        __set_bit(X86_FEATURE_STIBP, hvm_featureset);
>> +
>> +        /* AMD's IBPB is a subset of IBRS/IBPB. */
>> +        __set_bit(X86_FEATURE_IBPB, hvm_featureset);
>> +    }
> As long as we don't expect this logic to grow, having it duplicated
> like this is probably fine. Otherwise a helper function might be
> better.

I'll see about this when I get back to the CPUID work.  There probably
will be others actions which are common across guest types, so a helper
is probably worthwhile, but some bits of this will change when we expose
full cpuid_policies to the toolstack.

~Andrew

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