Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

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

On Tue, Jan 30, 2024 at 03:47:37PM +0100, Jan Beulich wrote:
> On 30.01.2024 15:35, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
> >> On 30.01.2024 13:06, Roger Pau Monné wrote:
> >>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> >>>> On 30.01.2024 10:13, Roger Pau Monne wrote:
> >>>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} 
> >>>>> controls in
> >>>>> SPEC_CTRL MSR.
> >>>>>
> >>>>> Note that those controls are not used by the hypervisor.
> >>>>
> >>>> Despite this, ...
> >>>>
> >>>>> --- a/xen/arch/x86/msr.c
> >>>>> +++ b/xen/arch/x86/msr.c
> >>>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
> >>>>> cpu_policy *cp)
> >>>>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >>>>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> >>>>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
> >>>>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> >>>>> +                                    SPEC_CTRL_IPRED_DIS_S)
> >>>>> +                                 : 0) |
> >>>>>              0);
> >>>>>  }
> >>>>
> >>>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> >>>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> >>>> description it wants clarifying why it is acceptable to run Xen with the
> >>>> guest chosen settings for at least the DIS_S bit (assuming that it is
> >>>> okay to do so). Likely (didn't look there yet) also applicable to the
> >>>> further two patches.
> >>>
> >>> "The added feature is made dependent on IBRSB, which ensures it will
> >>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> >>> ensures the value of SPEC_CTRL will get context switched on exit/entry
> >>> to guest."
> >>>
> >>> Would adding the above to the commit message clarify the intended
> >>> implementation?
> >>
> >> It would improve things, at least hinting towards there being a connection
> >> between exposure and updating on entry to Xen. I'd like to ask though to
> >> avoid "context switch" when talking about entry from guest context. While
> >> in a way technically correct, our normal meaning of the term is the
> >> process of switching vCPU-s out/in on a pCPU.
> > 
> > "The added feature is made dependent on IBRSB, which ensures it will
> > only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> > ensures the value of SPEC_CTRL will get toggled between guest and Xen
> > values on exit/entry to guest."
> > 
> > But I wonder, we already allow guests the play with other SPEC_CTRL
> > bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
> > to Xen, so I'm unsure why adding more bits needs so much
> > justification.
> 
> Well, yes, I'm sorry, it was me forgetting the open-coded effect
> SC_MSR_{PV,HVM} has on exposing of the MSR. I guess I'd be happy with
> extending the last sentence a little, maybe "Note that those controls
> are not used by the hypervisor, and they're cleared on entry to Xen."
> If you're okay with that, I'd be happy to adjust while committing

Sure.

> (and assuming no other concerns are raised):
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> for all three patches.

Thanks.