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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> >>> --- a/xen/arch/x86/msr.c
> >>> +++ b/xen/arch/x86/msr.c
> >>> @@ -697,7 +697,15 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, 
> >>> uint64_t val)
> >>>                  msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> >>>          }
> >>>          else
> >>> +        {
> >>>              msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
> >>> +            if ( v == curr )
> >>> +                /*
> >>> +                 * Propagate the value to hardware, as it won't be 
> >>> context
> >>> +                 * switched on vmentry.
> >>> +                 */
> >>
> >> I have to admit that I find "on vmentry" in the comment misleading: Reading
> >> it I first thought you're still alluding to the old model. Plus I also find
> >> the combination of "context switched" and "on vmentry" problematic, as we
> >> generally mean something else when we say "context switch".
> > 
> > I had a hard time wording this, because of the Xen/guest vs vCPU
> > context switches.
> > 
> > What about:
> > 
> > "Propagate the value to hardware, as it won't we set on guest resume
> > path."
> 
> Sounds better, thanks (with s/we/be/).

Oh, yes, sorry.

> 
> >>> +                goto set_reg;
> >>
> >> It's not clear why you want to use hvm_set_reg() in the first place - the
> >> comment says "propagate to hardware", which would mean wrmsrl() in the
> >> usual case. Here it would mean a direct call to amd_set_ssbd() imo. That
> >> would then also be in line with all other "v == curr" conditionals, none
> >> of which apply to any "goto set_reg". ..._set_reg(), aiui, is meant only
> >> for use in cases where vCPU state needs updating such that proper state
> >> would be loaded later (e.g. during VM entry).
> > 
> > I thought it was better to hide those vendor specific calls in the
> > already existing vendor hooks (set_reg).  I don't mind calling
> > amd_set_ssbd() directly here if that's preferred, it seemed kind of a
> > layering violation when we have vendor specific hooks in place.
> 
> Well, Andrew of course should correct me if I'm wrong, but my understanding
> of the get/set-reg interface is as described. On which grounds I don't see
> any layering violation here - doing the call right here is merely a more
> involved flavor of wrmsrl().

OK, will change, but first we need an agreement on the SSBD context
switch comment.

Thanks, Roger.