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

Jan