| Message ID | 20240130091400.50622-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/intel: expose additional SPEC_CTRL MSR controls | expand |
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. Jan
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? Thanks, Roger.
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. Jan
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. Thanks, Roger.
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@suse.com> for all three patches. Jan
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@suse.com> > for all three patches. Thanks.
On 30/01/2024 12:59 pm, 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 guests can only see MSR_SPEC_CTRL when Xen knows (and is) context switching them properly. The logic is weird for legacy IBRS reasons, but I don't think any further justification is necessary. ~Andrew
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index c33dc78cd8f6..d500f87a5fd1 100644 --- 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); } diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index c897c2040136..e586e141c329 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -302,7 +302,7 @@ XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */ XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A MSR_SPEC_CTRL.PSFD */ -XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /* MSR_SPEC_CTRL.IPRED_DIS_* */ +XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /*A MSR_SPEC_CTRL.IPRED_DIS_* */ XEN_CPUFEATURE(RRSBA_CTRL, 13*32+ 2) /* MSR_SPEC_CTRL.RRSBA_DIS_* */ XEN_CPUFEATURE(DDP_CTRL, 13*32+ 3) /* MSR_SPEC_CTRL.DDP_DIS_U */ XEN_CPUFEATURE(BHI_CTRL, 13*32+ 4) /* MSR_SPEC_CTRL.BHI_DIS_S */ diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index df5222a3cdd0..45fab5e75d1c 100755 --- a/xen/tools/gen-cpuid.py +++ b/xen/tools/gen-cpuid.py @@ -319,7 +319,8 @@ def crunch_numbers(state): # IBRSB/IBRS, and we pass this MSR directly to guests. Treating them # as dependent features simplifies Xen's logic, and prevents the guest # from seeing implausible configurations. - IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS], + IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS, + IPRED_CTRL], IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS, IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE], IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/msr.c | 3 +++ xen/include/public/arch-x86/cpufeatureset.h | 2 +- xen/tools/gen-cpuid.py | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-)