Re: [Xen-devel] [PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/05/18 08:17, Jan Beulich wrote:
>>>> On 24.05.18 at 18:44, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 22/05/18 11:33, Jan Beulich wrote:
>>> Other than in the feature sets, where we indeed want to offer the
>>> feature even if not enumerated on hardware, we shouldn't dictate the
>>> feature being available if tool stack or host admin have decided not
>>> to expose it (for whatever [questionable?] reason).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> This is effectively accompanying the discussion rooted at the 4.8/4.7
>>> patch at
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
>>> dealing with a feature leveling issue.
>>>
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
>>>      recalculate_xstate(p);
>>>      recalculate_misc(p);
>>>  
>>> -    /*
>>> -     * Override STIBP to match IBRS.  Guests can safely use STIBP
>>> -     * functionality on non-HT hardware, but can't necesserily protect
>>> -     * themselves from SP2/Spectre/Branch Target Injection if STIBP is 
>>> hidden
>>> -     * on HT-capable hardware.
>>> -     */
>>> -    p->feat.stibp = p->feat.ibrsb;
>> You've deleted a comment explaining why this is needed for safety
>> reasons, without addressing the safety argument.  Simply "because we
>> shouldn't override the toolstack" isn't a reasonable argument.
> It very much is: Policy decisions belong, as far as possible, in the tool
> stack rather than the hypervisor, and in the admin's hands rather than
> pre-programmed tool stack behavior.

Policy decisions, absolutely, but a patch like this needs justify why it
is deleting a safety check.  One valid justification would be "the
safety property this override is trying to achieve is actually already
safe because of $X".

>
>> With the SP2 microcode, we have the following situations which can occur:
>>  * No mitigations
>>  * IBRSB visible
>>  * IBRSB and STIBP visible
>>
>> IBSRB enumerates MSR_SPEC_CTRL, MSR_SPEC_CTRL.IBRS, and MSR_PRED_CMD.
>> STIBP enumerates MSR_SPEC_CTRL.STIBP
>>
>> SPEC_CTRL.STIBP is specified as usable (albeit, as a nop) even if STIBP
>> isn't enumerated.  This is deliberately and explicitly for heterogeneous
>> migration scenarios, as it won't be a nop on other processors.  In
>> practice, this is so hypervisors can offer the feature unilaterally, and
>> have it usable on non-HT hardware.
>>
>> However, to safely level it, dom0 needs to see it set in the information
>> used to construct the guest policies.  In principle, this should just be
>> in the guest policy which needs adjusting.
>>
>> However, due to still not having got the CPUID policy improvements
>> finished, dom0 is still excluded from cpuid faulting for the exclusive
>> benefit of the CPUID logic in the domain builder, because it uses native
>> CPUID to construct the guests CPUID policy.  Therefore, dom0 is unable
>> to create a safe CPUID policy for the guest, and Xen must override the
>> setting.
> I don't understand this. Both xc_cpuid_{hvm,pv}_policy() have
>
>     case 0x00000007: /* Intel-defined CPU features */
>         if ( input[1] == 0 )
>         {
>             regs[1] = info->featureset[featureword_of(X86_FEATURE_FSGSBASE)];
>             regs[2] = 
> info->featureset[featureword_of(X86_FEATURE_PREFETCHWT1)];
>             regs[3] = 
> info->featureset[featureword_of(X86_FEATURE_AVX512_4VNNIW)];
>         }
>
> Where is the use of native CPUID here?

Oh - I'd completely forgotten I'd fixed up the xc_cpuid_set() path
(which libxl uses for cpuid=[]) as well as the xc_cpuid_apply_policy()
path which is used for "I'd like a default setup please".

Therefore, c/s 3e0c8272f200457d9a735070b66a0da808ac3924 looks to be the
point after which libxc becomes safe (WRT to these native CPUID
concerns), so long as the featureset do have a IBRSB -> STIBP override
visible in them.

In staging at the moment, the IBRSB -> STIBP override is performed in
guest_common_feature_adjustments(), but this gets rather more
complicated on older branches.  Perhaps the easiest way to test is with
`cpuid=no-stibp` and looking at xen-cpuid.

Here is a sample from one of my test boxes:

[root@fusebot ~]# xen-cpuid 
nr_features: 10
                      KEY 1d       1c       e1d      e1c      Da1      7b0      
7c0      e7d      e8b      7d0      

Static sets:
Known                     
bfebfbff:fffef3ff:ee500800:2469bfff:0000000f:fdbfffff:0040401f:00000500:00001001:8c00000c
Special                   
10000200:88200000:00000000:00000002:00000000:00002040:00000010:00000000:00000000:08000000
PV Mask                   
1fc9cbf5:f6f83203:e2500800:042109e3:00000007:fdaf0b39:00404003:00000000:00001001:8c00000c
HVM Shadow Mask           
1fcbfbff:f7f83223:ea500800:042189f7:0000000f:fdbf4bbb:00404007:00000000:00001001:8c00000c
HVM Hap Mask              
1fcbfbff:f7fa3223:ee500800:042189f7:0000000f:fdbf4fbb:0040400f:00000000:00001001:8c00000c

Dynamic sets:
Raw                       
bfebfbff:7ffafbff:2c100800:00000021:00000001:000027ab:00000000:00000100:00000000:8c000000
Host                      
bfebfbff:77faf3ff:2c100800:00000021:00000001:000027ab:00000000:00000100:00000000:84000000
PV                        
1fc9cbf5:f6f83203:20100800:00000021:00000001:00000329:00000000:00000000:00001000:8c000000
HVM                       
1fcbfbff:f7fa3223:2c100800:00000021:00000001:000007ab:00000000:00000000:00001000:8c000000

In the raw dynamic set, we see IBRSB, STIBP (and SSBD, seeing as I've
got some microcode).  STIBP is clear in the Host policy (because of the
cpuid= override), yet still set in the PV and HVM featuresets.

Therefore, a default guest constructed on non-HT hardware will see STIBP
visible, per the intention of its special case.

However, if you are going to make this change, then you're missing the
following hunk:

diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index c721c12..f1a5ed9 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -243,7 +243,7 @@ XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support 
only (no IBRS, used by
 XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A  AVX512 Neural Network Instructions 
*/
 XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply Accumulation 
Single Precision */
 XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by 
Intel) */
-XEN_CPUFEATURE(STIBP,         9*32+27) /*A! STIBP */
+XEN_CPUFEATURE(STIBP,         9*32+27) /*A  STIBP */
 XEN_CPUFEATURE(ARCH_CAPS,     9*32+29) /*   IA32_ARCH_CAPABILITIES MSR */
 XEN_CPUFEATURE(SSBD,          9*32+31) /*A  MSR_SPEC_CTRL.SSBD available */
 

which is the signal that:

...
 * Special: '!'
 *   This bit has special properties and is not a straight indication of a
 *   piece of new functionality.  Xen will handle these differently,
 *   and may override toolstack settings completely.
...

~Andrew

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