| Message ID | 20220126084452.28975-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: MSR_SPEC_CTRL support for SVM guests | expand |
On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote: > All AMD IBRS-related features are in CPUID.0x80000008.ebx. Collect them in > early_cpu_init() so init_speculative_mitigations() can use them. > > Rework the existing logic structure to fill in c->extended_cpuid_level and > separate out the ambiguous use of ebx in an otherwise 0x80000008-specific > logic block. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> It would be good to update the comment ahead of early_cpu_init to mention it now also gather speculation-related fields from CPUID in order to do early setup of mitigations. I think you could also use boot_cpu_data in spec_ctrl.c print_details instead of fetching again the cpuid leafs? Thanks, Roger.
On 26.01.2022 13:44, Roger Pau Monné wrote: > I think you could also use boot_cpu_data in spec_ctrl.c print_details > instead of fetching again the cpuid leafs? Like with pre-existing code there iirc the intention is to really log what is available in hardware, no matter what was subsequently disabled in software already (e.g. via "cpuid="). Jan
On 26/01/2022 12:44, Roger Pau Monné wrote: > On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote: >> All AMD IBRS-related features are in CPUID.0x80000008.ebx. Collect them in >> early_cpu_init() so init_speculative_mitigations() can use them. >> >> Rework the existing logic structure to fill in c->extended_cpuid_level and >> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific >> logic block. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > It would be good to update the comment ahead of early_cpu_init to > mention it now also gather speculation-related fields from CPUID in > order to do early setup of mitigations. > > I think you could also use boot_cpu_data in spec_ctrl.c print_details > instead of fetching again the cpuid leafs? Hmm - I may have a mistake here. Boot time CPUID handling is giant mess, and I haven't had time to finish my work to make BSP microcode loading dependent on xmalloc(), allowing it to move far earlier, and removing the early/late CPUID split. However, init_speculative_mitigations() is called after late CPUID setup, so e8b should be suitably collected. Let me try to figure out what's going on. For print_details(), I have a feeling that may have been an artefact of an early version of the logic, and likely can be cleaned up. ~Andrew
On 26/01/2022 13:37, Andrew Cooper wrote: > On 26/01/2022 12:44, Roger Pau Monné wrote: >> On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote: >>> All AMD IBRS-related features are in CPUID.0x80000008.ebx. Collect them in >>> early_cpu_init() so init_speculative_mitigations() can use them. >>> >>> Rework the existing logic structure to fill in c->extended_cpuid_level and >>> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific >>> logic block. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> >> >> It would be good to update the comment ahead of early_cpu_init to >> mention it now also gather speculation-related fields from CPUID in >> order to do early setup of mitigations. >> >> I think you could also use boot_cpu_data in spec_ctrl.c print_details >> instead of fetching again the cpuid leafs? > Hmm - I may have a mistake here. > > Boot time CPUID handling is giant mess, and I haven't had time to finish > my work to make BSP microcode loading dependent on xmalloc(), allowing Sorry. I mean "independent" here. ~Andrew > it to move far earlier, and removing the early/late CPUID split. > > However, init_speculative_mitigations() is called after late CPUID > setup, so e8b should be suitably collected. Let me try to figure out > what's going on. > > For print_details(), I have a feeling that may have been an artefact of > an early version of the logic, and likely can be cleaned up. > > ~Andrew
On 26/01/2022 13:47, Andrew Cooper wrote: > On 26/01/2022 13:37, Andrew Cooper wrote: >> On 26/01/2022 12:44, Roger Pau Monné wrote: >>> On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote: >>>> All AMD IBRS-related features are in CPUID.0x80000008.ebx. Collect them in >>>> early_cpu_init() so init_speculative_mitigations() can use them. >>>> >>>> Rework the existing logic structure to fill in c->extended_cpuid_level and >>>> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific >>>> logic block. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> >>> >>> It would be good to update the comment ahead of early_cpu_init to >>> mention it now also gather speculation-related fields from CPUID in >>> order to do early setup of mitigations. >>> >>> I think you could also use boot_cpu_data in spec_ctrl.c print_details >>> instead of fetching again the cpuid leafs? >> Hmm - I may have a mistake here. >> >> Boot time CPUID handling is giant mess, and I haven't had time to finish >> my work to make BSP microcode loading dependent on xmalloc(), allowing > Sorry. I mean "independent" here. > > ~Andrew > >> it to move far earlier, and removing the early/late CPUID split. >> >> However, init_speculative_mitigations() is called after late CPUID >> setup, so e8b should be suitably collected. Let me try to figure out >> what's going on. And testing shows that everything works fine without this patch. I must have had some breakage during development which has resolved itself as part of cleaning the series up. Anyway, I'll withdraw this patch. ~Andrew
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 4a163afbfc7e..866f1a516447 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -348,9 +348,13 @@ void __init early_cpu_init(void) } eax = cpuid_eax(0x80000000); - if ((eax >> 16) == 0x8000 && eax >= 0x80000008) { - ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0; - eax = cpuid_eax(0x80000008); + if ((eax >> 16) == 0x8000) + c->extended_cpuid_level = eax; + + if (c->extended_cpuid_level >= 0x80000008) { + cpuid(0x80000008, &eax, + &c->x86_capability[cpufeat_word(X86_FEATURE_CLZERO)], + &ecx, &edx); paddr_bits = eax & 0xff; if (paddr_bits > PADDR_BITS) @@ -363,10 +367,11 @@ void __init early_cpu_init(void) hap_paddr_bits = ((eax >> 16) & 0xff) ?: paddr_bits; if (hap_paddr_bits > PADDR_BITS) hap_paddr_bits = PADDR_BITS; + } + if (c->extended_cpuid_level >= 0x8000001f) /* Account for SME's physical address space reduction. */ - paddr_bits -= (ebx >> 6) & 0x3f; - } + paddr_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f; if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))) park_offline_cpus = opt_mce;
All AMD IBRS-related features are in CPUID.0x80000008.ebx. Collect them in early_cpu_init() so init_speculative_mitigations() can use them. Rework the existing logic structure to fill in c->extended_cpuid_level and separate out the ambiguous use of ebx in an otherwise 0x80000008-specific logic block. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/cpu/common.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)