Messages in this thread | | | Date | Fri, 24 Mar 2023 17:56:11 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v4 06/12] x86/mtrr: replace vendor tests in MTRR code |
| |
On Mon, Mar 06, 2023 at 05:34:19PM +0100, Juergen Gross wrote: > Modern CPUs all share the same MTRR interface implemented via > generic_mtrr_ops. > > At several places in MTRR code this generic interface is deduced via > is_cpu(INTEL) tests, which is only working due to X86_VENDOR_INTEL > being 0 (the is_cpu() macro is testing mtrr_if->vendor, which isn't > explicitly set in generic_mtrr_ops). > > Fix that by replacing the is_cpu(INTEL) tests with testing for mtrr_if > to be &generic_mtrr_ops.
Two things:
* is_cpu() checks also whether mtrr_if is set. And we don't set it for all vendors. I wanted to replace that thing with a vendor check recently but there's that little issue.
I guess for the cases where we have the generic MTRR implementation, we can safely assume that mtrr_if is set. Which leads me to the second thing:
* If you're going to test for &generic_mtrr_ops, then you can just as well do
cpu_feature_enabled(X86_FEATURE_MTRR)
which is a lot more telling.
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c > index 5fe62ee0361b..0c83990501f5 100644 > --- a/arch/x86/kernel/cpu/mtrr/mtrr.c > +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c > @@ -108,14 +108,12 @@ static int have_wrcomb(void) > /* This function returns the number of variable MTRRs */ > static void __init set_num_var_ranges(bool use_generic) > { > - unsigned long config = 0, dummy; > + unsigned long config, dummy; > > if (use_generic) > rdmsr(MSR_MTRRcap, config, dummy); > - else if (is_cpu(AMD) || is_cpu(HYGON)) > - config = 2; > - else if (is_cpu(CYRIX) || is_cpu(CENTAUR)) > - config = 8; > + else > + config = mtrr_if->var_regs; > > num_var_ranges = config & 0xff; > }
Since you're touching this function, you might simply expand its body in its only call site in mtrr_bp_init(), put a comment above the expanded code and remove that function.
That is, if we're going to do the ->var_regs thing.
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |