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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan