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 17:31, Roger Pau Monné wrote:
> On Wed, Mar 09, 2022 at 04:40:24PM +0100, Jan Beulich wrote:
>> 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:
>>>>> --- 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.
> 
> Right, so you would be fine with leaving the command line option
> description alone.

Yes.

>>>>> --- 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 ...
> 
> No, you are OK to ask. When developing the series I went back and
> forth myself deciding whether 'hijacking' the spec_ctrl field to
> implement VIRT_SPEC_CTRL was OK.
> 
> If host has AMD_SSBD: VIRT_SPEC_CTRL.SSBD will use the SPEC_CTRL.SSBD
> bit in the spec_ctrl field, but it will be set behind the guests back.
> If guests sets VIRT_SPEC_CTRL.SSBD but not SPEC_CTRL.SSBD, reads of
> SPEC_CTRL.SSBD from guest context will return 0, but the bit will be
> set.
> 
> I called it 'shadow' because the underlying SPEC_CTRL.SSBD bit will
> get set, but reading SPEC_CTRL.SSBD could return 0 if the bit has been
> set from VIRT_SPEC_CTRL.
> 
> Do you think that's a suitable use of 'shadow'?

Not sure, but since I don't have a good alternative suggestion, please
keep using "shadow".

Jan