Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2022 16:03, Roger Pau Monné wrote:
> On Mon, Feb 14, 2022 at 04:07:09PM +0100, Jan Beulich wrote:
>> On 01.02.2022 17:46, Roger Pau Monne wrote:
>>> Use the logic to set shadow SPEC_CTRL values in order to implement
>>> support for VIRT_SPEC_CTRL (signaled by VIRT_SSBD CPUID flag) for HVM
>>> guests. This includes using the spec_ctrl vCPU MSR variable to store
>>> the guest set value of VIRT_SPEC_CTRL.SSBD.
>>
>> This leverages the guest running on the OR of host and guest values,
>> aiui. If so, this could do with spelling out.
>>
>>> Note that VIRT_SSBD is only set in the HVM max CPUID policy, as the
>>> default should be to expose SPEC_CTRL only and support VIRT_SPEC_CTRL
>>> for migration compatibility.
>>
>> I'm afraid I don't understand this last statement: How would this be
>> about migration compatibility? No guest so far can use VIRT_SPEC_CTRL,
>> and a future guest using it is unlikely to be able to cope with the
>> MSR "disappearing" during migration.
> 
> Maybe I didn't express this correctly. What I meant to explain is that
> on hardware having SPEC_CTRL VIRT_SPEC_CTRL will not be offered by
> default to guests. VIRT_SPEC_CTRL will only be part of the max CPUID
> policy so it can be enabled for compatibility purposes. Does this make
> sense?

Yes. Can you re-word along these lines?

>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -2273,8 +2273,9 @@ to use.
>>>  * `pv=` and `hvm=` offer control over all suboptions for PV and HVM guests
>>>    respectively.
>>>  * `msr-sc=` offers control over Xen's support for manipulating 
>>> `MSR_SPEC_CTRL`
>>> -  on entry and exit.  These blocks are necessary to virtualise support for
>>> -  guests and if disabled, guests will be unable to use IBRS/STIBP/SSBD/etc.
>>> +  and/or `MSR_VIRT_SPEC_CTRL` on entry and exit.  These blocks are 
>>> necessary to
>>
>> Why would Xen be manipulating an MSR it only brings into existence for its
>> guests?
> 
> Well, that's not exactly true. Xen does use VIRT_SPEC_CTRL (see
> amd_init_ssbd).
> 
> I'm unsure how to express support for VIRT_SPEC_CTRL, as it does rely
> on SPEC_CTRL when available.

I wonder whether the command line doc needs to go into this level of
detail.

>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -543,6 +543,13 @@ static void __init calculate_hvm_max_policy(void)
>>>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
>>>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
>>>      }
>>> +    else
>>> +        /*
>>> +         * If SPEC_CTRL is available VIRT_SPEC_CTRL can also be 
>>> implemented as
>>> +         * it's a subset of the controls exposed in SPEC_CTRL (SSBD only).
>>> +         * Expose in the max policy for compatibility migration.
>>> +         */
>>> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
>>
>> This means even Intel guests can use the feature then? I thought it was
>> meanwhile deemed bad to offer such cross-vendor features?
> 
> No, we shouldn't expose to Intel.
> 
>> Additionally, is SPEC_CTRL (i.e. IBRS) availability enough? Don't you
>> need AMD_SSBD as a prereq (which may want expressing in gen-cpuid.py)?
> 
> We need AMD_SSBD if we implement VIRT_SPEC_CTRL on top of SPEC_CTRL,
> but we could also implement it on top of VIRT_SPEC_CTRL (if Xen runs
> virtualized) or even using the legacy SSBD setting mechanisms found in
> amd_init_ssbd, so I don't think VIRT_SSBD should explicitly depend on
> AMD_SSBD in gen-cpuid.py.

Hmm, yes, good point. But when the prereqs cannot be expressed in
gen-cpuid.py, I guess they need to be encoded here.

>>> --- a/xen/arch/x86/include/asm/msr.h
>>> +++ b/xen/arch/x86/include/asm/msr.h
>>> @@ -291,6 +291,7 @@ struct vcpu_msrs
>>>  {
>>>      /*
>>>       * 0x00000048 - MSR_SPEC_CTRL
>>> +     * 0xc001011f - MSR_VIRT_SPEC_CTRL
>>>       *
>>>       * For PV guests, this holds the guest kernel value.  It is accessed on
>>>       * every entry/exit path.
>>> @@ -301,7 +302,10 @@ struct vcpu_msrs
>>>       * For SVM, the guest value lives in the VMCB, and hardware 
>>> saves/restores
>>>       * the host value automatically.  However, guests run with the OR of 
>>> the
>>>       * host and guest value, which allows Xen to set protections behind the
>>> -     * guest's back.
>>> +     * guest's back.  Use such functionality in order to implement support 
>>> for
>>> +     * VIRT_SPEC_CTRL as a shadow value of SPEC_CTRL and thus store the 
>>> value
>>> +     * of VIRT_SPEC_CTRL in this field, taking advantage of both MSRs 
>>> having
>>> +     * compatible layouts.
>>
>> I guess "shadow value" means more like an alternative value, but
>> (see above) this is about setting for now just one bit behind the
>> guest's back.
> 
> Well, the guest sets the bit in VIRT_SPEC_CTRL and Xen sets it on
> SPEC_CTRL in order for it to have effect. I can use 'alternative
> value' if that's clearer.

Well, as I tried to express in my earlier reply, I view "shadow value"
to mean "alternative value", so replacing wouldn't help. The question
whether it acts like the shadow values we know elsewhere (VMX'es CR0
and CR4, for example). If it does, using the same term is of course
fine. But it didn't look to me as if it would, hence I'd prefer to
avoid ambiguity. But please realize that I may have misunderstood
things ...

>>> --- a/xen/arch/x86/spec_ctrl.c
>>> +++ b/xen/arch/x86/spec_ctrl.c
>>> @@ -395,12 +395,13 @@ static void __init print_details(enum ind_thunk 
>>> thunk, uint64_t caps)
>>>       * mitigation support for guests.
>>>       */
>>>  #ifdef CONFIG_HVM
>>> -    printk("  Support for HVM VMs:%s%s%s%s%s\n",
>>> +    printk("  Support for HVM VMs:%s%s%s%s%s%s\n",
>>>             (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
>>>              boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ||
>>>              boot_cpu_has(X86_FEATURE_MD_CLEAR)   ||
>>>              opt_eager_fpu)                           ? ""               : 
>>> " None",
>>>             boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_SPEC_CTRL" : 
>>> "",
>>> +           boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " 
>>> MSR_VIRT_SPEC_CTRL" : "",
>>>             boot_cpu_has(X86_FEATURE_SC_RSB_HVM)      ? " RSB"           : 
>>> "",
>>>             opt_eager_fpu                             ? " EAGER_FPU"     : 
>>> "",
>>>             boot_cpu_has(X86_FEATURE_MD_CLEAR)        ? " MD_CLEAR"      : 
>>> "");
>>
>> The output getting longish, can the two SC_MSR_HVM dependent items
>> perhaps be folded, e.g. by making it "MSR_{,VIRT_}SPEC_CTRL"?
> 
> OK, but further patches will add MSR_VIRT_SPEC_CTRL to hardware that
> doesn't expose MSR_SPEC_CTRL to guests, at which point it could be
> confusing?

Yeah, I obviously hadn't seen adjustments done here by later patches.
When I saw those, I think I understood why you things do this way.

>>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>>> @@ -265,7 +265,7 @@ XEN_CPUFEATURE(IBRS_SAME_MODE, 8*32+19) /*S  IBRS 
>>> provides same-mode protection
>>>  XEN_CPUFEATURE(NO_LMSL,       8*32+20) /*S  EFER.LMSLE no longer 
>>> supported. */
>>>  XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory 
>>> Number */
>>>  XEN_CPUFEATURE(AMD_SSBD,      8*32+24) /*S  MSR_SPEC_CTRL.SSBD available */
>>> -XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*   MSR_VIRT_SPEC_CTRL.SSBD */
>>> +XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*!s MSR_VIRT_SPEC_CTRL.SSBD */
>>
>> What is the ! intended to cover here? From guest perspective the
>> MSR acts entirely normally afaict.
> 
> I've used the ! to note that VIRT_SSBD might be exposed on hardware
> whether it's not available as part of the host featureset. It did seem
> to me that using just 's' didn't reflect this properly.

I wouldn't have assigned such meaning to !. In fact if we emulated
a feature completely, I think it could legitimately show up here
without !. But then again I may also not fully be aware of all of
Andrew's intentions ...

Jan

> According to my reading of the comment at the top '!' is not used to
> signal that the feature might act differently, but just that it's
> presence cannot be properly expressed with just the A, S, H flags,
> which would be the case for VIRT_SSBD I think.
> 
> Thanks, Roger.
>