Re: [PATCH for-4.17 v2.1 2/3] amd/virt_ssbd: set SSBD at vCPU context switch

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2022 11:36, Roger Pau Monné wrote:
> On Thu, Nov 03, 2022 at 10:01:49AM +0100, Jan Beulich wrote:
>> On 03.11.2022 09:52, Roger Pau Monné wrote:
>>> On Thu, Nov 03, 2022 at 09:09:41AM +0100, Jan Beulich wrote:
>>>> On 02.11.2022 18:38, Roger Pau Monné wrote:
>>>>> On Wed, Nov 02, 2022 at 12:49:17PM +0100, Jan Beulich wrote:
>>>>>> On 29.10.2022 15:12, Roger Pau Monne wrote:
>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>>>> @@ -973,6 +973,16 @@ static void cf_check svm_ctxt_switch_from(struct 
>>>>>>> vcpu *v)
>>>>>>>  
>>>>>>>      /* Resume use of ISTs now that the host TR is reinstated. */
>>>>>>>      enable_each_ist(idt_tables[cpu]);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Clear previous guest selection of SSBD if set.  Note that 
>>>>>>> SPEC_CTRL.SSBD
>>>>>>> +     * is already cleared by svm_vmexit_spec_ctrl.
>>>>>>> +     */
>>>>>>> +    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>>>>>> +    {
>>>>>>> +        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>>>>>> +        amd_set_ssbd(false);
>>>>>>> +    }
>>>>>>>  }
>>>>>>
>>>>>> Aren't you potentially turning off SSBD here just to ...
>>>>>>
>>>>>>> @@ -1000,6 +1010,13 @@ static void cf_check svm_ctxt_switch_to(struct 
>>>>>>> vcpu *v)
>>>>>>>  
>>>>>>>      if ( cpu_has_msr_tsc_aux )
>>>>>>>          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
>>>>>>> +
>>>>>>> +    /* Load SSBD if set by the guest. */
>>>>>>> +    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>>>>>> +    {
>>>>>>> +        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>>>>>> +        amd_set_ssbd(true);
>>>>>>> +    }
>>>>>>>  }
>>>>>>
>>>>>> ... turn it on here again? IOW wouldn't switching better be isolated to
>>>>>> just svm_ctxt_switch_to(), doing nothing if already in the intended mode?
>>>>>
>>>>> What if we switch from a HVM vCPU into a PV one?  AFAICT then
>>>>> svm_ctxt_switch_to() won't get called and we would be running the PV
>>>>> guest with the previous HVM domain SSBD selection.
>>>>
>>>> Would that be a problem? Or in other words: What is the intended behavior
>>>> for PV? PV domains can control SSBD via SPEC_CTRL (only), so all we need
>>>> to guarantee is that we respect their choice there.
>>>
>>> If the hardware only supports non-architectural way (LS_CFG) or
>>> VIRT_SPEC_CTRL to set SSBD then PV guests won't be able to change the
>>> setting inherited from a previously running HVM guest. IMO it's fine
>>> to run Xen code with the guest selection of SSBD, but carrying such
>>> selection (ie: SSBD set) across guest context switches will be a too
>>> big penalty.
>>
>> Hmm, perhaps. Question then is whether to better turn it off from
>> paravirt_ctxt_switch_to() (which would take care of the idle domain as
>> well, if we want it off there rather than considering the idle domain
>> as "Xen context"). Or, yet another option, don't use
>> *_ctxt_switch_{from,to}() at all but invoke it directly from
>> __context_switch().
> 
> I consider it fine to run the idle domain with the guest SSBD
> selection, or else switching to/from the idle domain could cause
> toggling of SSBD which is an unneeded penalty.
> 
> If there's an specific issue that needs dealing with I'm happy to
> adjust, otherwise I think the proposed approach is an acceptable
> compromise to avoid excessive toggling of SSBD when not using
> SPEC_CTRL.

Well, perhaps. What I was suggesting would further limit the toggling,
but I'm not going to insist on you going that route.

Jan