| Message ID | 20221011160245.56735-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | amd/virt_ssbd: refactoring and cleanup | expand |
On 11.10.2022 18:02, Roger Pau Monne wrote: > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable) > wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0); > else if ( amd_legacy_ssbd ) > core_set_legacy_ssbd(enable); > - else > + else if ( cpu_has_ssb_no ) { Nit: While already an issue in patch 1, it is actually the combination of inner blanks and brace placement which made me spot the style issue here. > + /* Nothing to do. */ How is the late placement here in line with ... > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void) > __clear_bit(X86_FEATURE_IBRSB, hvm_featureset); > __clear_bit(X86_FEATURE_IBRS, hvm_featureset); > } > - else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ) > + else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) || > + boot_cpu_has(X86_FEATURE_SSB_NO) ) > /* > * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed > * and implemented using the former. Expose in the max policy only as > * the preference is for guests to use SPEC_CTRL.SSBD if available. > + * > + * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration > + * compatibility reasons. If SSB_NO is present setting > + * VIRT_SPEC_CTRL.SSBD is a no-op. > */ > __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); ... this comment addition talking about "no-op"? Jan
On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote: > On 11.10.2022 18:02, Roger Pau Monne wrote: > > --- a/xen/arch/x86/cpu/amd.c > > +++ b/xen/arch/x86/cpu/amd.c > > @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable) > > wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0); > > else if ( amd_legacy_ssbd ) > > core_set_legacy_ssbd(enable); > > - else > > + else if ( cpu_has_ssb_no ) { > > Nit: While already an issue in patch 1, it is actually the combination > of inner blanks and brace placement which made me spot the style issue > here. Oh, indeed, extra spaces. > > + /* Nothing to do. */ > > How is the late placement here in line with ... > > > --- a/xen/arch/x86/cpuid.c > > +++ b/xen/arch/x86/cpuid.c > > @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void) > > __clear_bit(X86_FEATURE_IBRSB, hvm_featureset); > > __clear_bit(X86_FEATURE_IBRS, hvm_featureset); > > } > > - else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ) > > + else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) || > > + boot_cpu_has(X86_FEATURE_SSB_NO) ) > > /* > > * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed > > * and implemented using the former. Expose in the max policy only as > > * the preference is for guests to use SPEC_CTRL.SSBD if available. > > + * > > + * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration > > + * compatibility reasons. If SSB_NO is present setting > > + * VIRT_SPEC_CTRL.SSBD is a no-op. > > */ > > __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); > > ... this comment addition talking about "no-op"? We need the empty `else if ...` body in order to avoid hitting the ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has SSB_NO will not result in any setting being propagated to the hardware. I can make that clearer. In any case I'm unable to test the patch because there's no hw with the feature that I'm aware of. Thanks, Roger.
On 13.10.2022 16:06, Roger Pau Monné wrote: > On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote: >> On 11.10.2022 18:02, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/cpu/amd.c >>> +++ b/xen/arch/x86/cpu/amd.c >>> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable) >>> wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0); >>> else if ( amd_legacy_ssbd ) >>> core_set_legacy_ssbd(enable); >>> - else >>> + else if ( cpu_has_ssb_no ) { >> >> Nit: While already an issue in patch 1, it is actually the combination >> of inner blanks and brace placement which made me spot the style issue >> here. > > Oh, indeed, extra spaces. > >>> + /* Nothing to do. */ >> >> How is the late placement here in line with ... >> >>> --- a/xen/arch/x86/cpuid.c >>> +++ b/xen/arch/x86/cpuid.c >>> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void) >>> __clear_bit(X86_FEATURE_IBRSB, hvm_featureset); >>> __clear_bit(X86_FEATURE_IBRS, hvm_featureset); >>> } >>> - else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ) >>> + else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) || >>> + boot_cpu_has(X86_FEATURE_SSB_NO) ) >>> /* >>> * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed >>> * and implemented using the former. Expose in the max policy only as >>> * the preference is for guests to use SPEC_CTRL.SSBD if available. >>> + * >>> + * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration >>> + * compatibility reasons. If SSB_NO is present setting >>> + * VIRT_SPEC_CTRL.SSBD is a no-op. >>> */ >>> __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); >> >> ... this comment addition talking about "no-op"? > > We need the empty `else if ...` body in order to avoid hitting the > ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has > SSB_NO will not result in any setting being propagated to the > hardware. I can make that clearer. I guess my question was more towards: Shouldn't that check in amd_set_ssbd() move ahead? As an aside I notice you use cpu_has_ssb_no there but not here. I might guess this is because of the adjacent existing boot_cpu_has(), but it still strikes me as a little odd (as in: undue open-coding). Jan
On Thu, Oct 13, 2022 at 04:43:15PM +0200, Jan Beulich wrote: > On 13.10.2022 16:06, Roger Pau Monné wrote: > > On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote: > >> On 11.10.2022 18:02, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/cpu/amd.c > >>> +++ b/xen/arch/x86/cpu/amd.c > >>> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable) > >>> wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0); > >>> else if ( amd_legacy_ssbd ) > >>> core_set_legacy_ssbd(enable); > >>> - else > >>> + else if ( cpu_has_ssb_no ) { > >> > >> Nit: While already an issue in patch 1, it is actually the combination > >> of inner blanks and brace placement which made me spot the style issue > >> here. > > > > Oh, indeed, extra spaces. > > > >>> + /* Nothing to do. */ > >> > >> How is the late placement here in line with ... > >> > >>> --- a/xen/arch/x86/cpuid.c > >>> +++ b/xen/arch/x86/cpuid.c > >>> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void) > >>> __clear_bit(X86_FEATURE_IBRSB, hvm_featureset); > >>> __clear_bit(X86_FEATURE_IBRS, hvm_featureset); > >>> } > >>> - else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ) > >>> + else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) || > >>> + boot_cpu_has(X86_FEATURE_SSB_NO) ) > >>> /* > >>> * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed > >>> * and implemented using the former. Expose in the max policy only as > >>> * the preference is for guests to use SPEC_CTRL.SSBD if available. > >>> + * > >>> + * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration > >>> + * compatibility reasons. If SSB_NO is present setting > >>> + * VIRT_SPEC_CTRL.SSBD is a no-op. > >>> */ > >>> __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); > >> > >> ... this comment addition talking about "no-op"? > > > > We need the empty `else if ...` body in order to avoid hitting the > > ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has > > SSB_NO will not result in any setting being propagated to the > > hardware. I can make that clearer. > > I guess my question was more towards: Shouldn't that check in > amd_set_ssbd() move ahead? Right, I assumed that cpu_has_ssb_no would be exclusive with any other SSBD mechanism, but that doesn't need to be true. > As an aside I notice you use cpu_has_ssb_no there but not here. I > might guess this is because of the adjacent existing > boot_cpu_has(), but it still strikes me as a little odd (as in: > undue open-coding). Indeed, the whole function uses boot_cpu_has() so it seemed clearer to also use it for SSB_NO. Thanks, Roger.
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index cfeb8d1daf..74cfeffc29 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable) wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0); else if ( amd_legacy_ssbd ) core_set_legacy_ssbd(enable); - else + else if ( cpu_has_ssb_no ) { + /* Nothing to do. */ + } else ASSERT_UNREACHABLE(); } diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index acc2f606ce..e394dbe669 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void) __clear_bit(X86_FEATURE_IBRSB, hvm_featureset); __clear_bit(X86_FEATURE_IBRS, hvm_featureset); } - else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ) + else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) || + boot_cpu_has(X86_FEATURE_SSB_NO) ) /* * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed * and implemented using the former. Expose in the max policy only as * the preference is for guests to use SPEC_CTRL.SSBD if available. + * + * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration + * compatibility reasons. If SSB_NO is present setting + * VIRT_SPEC_CTRL.SSBD is a no-op. */ __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
Hardware that exposes SSB_NO can implement the setting of SSBD as a no-op because it's not affected by SSB. Take advantage of that and allow exposing VIRT_SPEC_CTRL.SSBD to guest running on hadrware that has SSB_NO. Only set VIRT_SSBD on the max policy though, as the feature is only intended to be used for migration compatibility. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- As there's no hardware with SSB_NO so far the patch is untested. Post it for reference if there's hardware with the bit set. --- xen/arch/x86/cpu/amd.c | 4 +++- xen/arch/x86/cpuid.c | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-)