Messages in this thread Patch in this message | | | From | "Compostella, Jeremy" <> | Subject | Re: Reserved bits and commit x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach | Date | Thu, 19 Oct 2023 13:01:16 -0700 |
| |
"Compostella, Jeremy" <jeremy.compostella@intel.com> writes:
> Adam Dunlap <acdunlap@google.com> writes: > >> On Wed, Oct 18, 2023 at 3:27 PM Compostella, Jeremy >> <jeremy.compostella@intel.com> wrote: >>> In the light of commit fbf6449f84bf I am wondering what is the right >>> approach to fix the regression for AMD and then fix the MTRR check for >>> Intel. Should we introduce a new cpu_dev callback to read the number >>> of reserved bits and take it into account in get_cpu_address_sizes() ? >> >> I think this approach makes sense. It seems better to have one >> function that simply sets it to the right thing rather than setting >> it to one value and then adjusting it (fbf6449f84bf did that for >> x86_virt_bits, although it caused some other problems). However, I'm >> not sure it would solve the problem your original patch tried to >> fix, since x86_phys_bits would still be set after intel_init, which >> apparently uses the value. > > Using cscope, I don't see any evidence of any vendor init code using > `x86_phys_bits'. To my knowledge, they seem to be only setting > x86_phys_bits or adjusting it. > > >> Would it work to move the call to get_cpu_address_sizes() to nearer >> the start of early_identify_cpu()? We could also add a cpu_dev >> callback so it doesn't need the 2-phase approach, but this would at >> least bring it back into parity with v6.6-rc6. > > Such a change should resolve the issue I reported on this thread. I > can run a quick smoke test later tonight or tomorrow.
It turns out that your suggestion does not work because `get_cpu_address_sizes()' relies on `c->extended_cpuid_level' (set by `get_cpu_cap(c)') and the `X86_FEATURE_CPUID' cpu capability (set by `setup_force_cpu_cap(X86_FEATURE_CPUID)').
The following change works perfectly well for me:
,---- | @@ -1589,6 +1591,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c) | get_cpu_vendor(c); | get_cpu_cap(c); | setup_force_cpu_cap(X86_FEATURE_CPUID); | + get_cpu_address_sizes(c); | cpu_parse_early_param(); | | if (this_cpu->c_early_init) | @@ -1603,7 +1606,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c) | setup_clear_cpu_cap(X86_FEATURE_CPUID); | } | | - get_cpu_address_sizes(c); | | setup_force_cpu_cap(X86_FEATURE_ALWAYS); `----
Looking at fbf6449f84bf I am under the impression it should not hurt it either but I'll let you verify.
-- *Jeremy* /One Emacs to rule them all/
| |