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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.05.18 at 15:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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".

That's what I would have hoped the initial part of the description conveys.

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

I did consider this before submitting, but the special casing in the feature set
construction left me uncertain how special "special" really means. I notice I
forgot to add a respective remark after the --- marker. I'm fine dropping the !.

Now that I think we've managed to settle on a way forward for this patch,
what about the proposed alternative patch for 4.8 and 4.7? It has the same
net effect as the one here, after all (and afaict).

Jan



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