[v5,07/15] x86/mtrr: replace vendor tests in MTRR code
Commit Message
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.
The only other place where the .vendor member of struct mtrr_ops is
being used is in set_num_var_ranges(), where depending on the vendor
the number of MTRR registers is determined. This can easily be changed
by replacing .vendor with the static number of MTRR registers.
It should be noted that the test "is_cpu(HYGON)" wasn't ever returning
true, as there is no struct mtrr_ops with that vendor information.
Signed-off-by: Juergen Gross <jgross@suse.com>
Tested-by: Michael Kelley <mikelley@microsoft.com>
---
V3:
- new patch
V4:
- use cpu_feature_enabled(X86_FEATURE_MTRR) for testing generic MTRRs
(Boris Petkov)
---
arch/x86/kernel/cpu/mtrr/amd.c | 2 +-
arch/x86/kernel/cpu/mtrr/centaur.c | 2 +-
arch/x86/kernel/cpu/mtrr/cleanup.c | 4 ++--
arch/x86/kernel/cpu/mtrr/cyrix.c | 2 +-
arch/x86/kernel/cpu/mtrr/generic.c | 2 +-
arch/x86/kernel/cpu/mtrr/mtrr.c | 8 +++-----
arch/x86/kernel/cpu/mtrr/mtrr.h | 4 +---
7 files changed, 10 insertions(+), 14 deletions(-)
Comments
On Sat, Apr 01, 2023 at 08:36:44AM +0200, 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.
Replace with:
"Test the generic CPU feature X86_FEATURE_MTRR instead."
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 1c19d67ddab3..46aae69d259e 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 & MTRR_CONFIG_NUM_VAR_MASK;
> }
From previous review which you've missed to incorporate:
"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."
On 12.04.23 10:45, Borislav Petkov wrote:
> On Sat, Apr 01, 2023 at 08:36:44AM +0200, 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.
>
> Replace with:
>
> "Test the generic CPU feature X86_FEATURE_MTRR instead."
>
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 1c19d67ddab3..46aae69d259e 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 & MTRR_CONFIG_NUM_VAR_MASK;
>> }
>
> From previous review which you've missed to incorporate:
>
> "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."
>
Oh, sorry. Will do it.
Juergen
@@ -110,7 +110,7 @@ amd_validate_add_page(unsigned long base, unsigned long size, unsigned int type)
}
const struct mtrr_ops amd_mtrr_ops = {
- .vendor = X86_VENDOR_AMD,
+ .var_regs = 2,
.set = amd_set_mtrr,
.get = amd_get_mtrr,
.get_free_region = generic_get_free_region,
@@ -112,7 +112,7 @@ centaur_validate_add_page(unsigned long base, unsigned long size, unsigned int t
}
const struct mtrr_ops centaur_mtrr_ops = {
- .vendor = X86_VENDOR_CENTAUR,
+ .var_regs = 8,
.set = centaur_set_mcr,
.get = centaur_get_mcr,
.get_free_region = centaur_get_free_region,
@@ -689,7 +689,7 @@ int __init mtrr_cleanup(unsigned address_bits)
int index_good;
int i;
- if (!is_cpu(INTEL) || enable_mtrr_cleanup < 1)
+ if (!cpu_feature_enabled(X86_FEATURE_MTRR) || enable_mtrr_cleanup < 1)
return 0;
rdmsr(MSR_MTRRdefType, def, dummy);
@@ -886,7 +886,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)
* Make sure we only trim uncachable memory on machines that
* support the Intel MTRR architecture:
*/
- if (!is_cpu(INTEL) || disable_mtrr_trim)
+ if (!cpu_feature_enabled(X86_FEATURE_MTRR) || disable_mtrr_trim)
return 0;
rdmsr(MSR_MTRRdefType, def, dummy);
@@ -235,7 +235,7 @@ static void cyrix_set_arr(unsigned int reg, unsigned long base,
}
const struct mtrr_ops cyrix_mtrr_ops = {
- .vendor = X86_VENDOR_CYRIX,
+ .var_regs = 8,
.set = cyrix_set_arr,
.get = cyrix_get_arr,
.get_free_region = cyrix_get_free_region,
@@ -846,7 +846,7 @@ int generic_validate_add_page(unsigned long base, unsigned long size,
* For Intel PPro stepping <= 7
* must be 4 MiB aligned and not touch 0x70000000 -> 0x7003FFFF
*/
- if (is_cpu(INTEL) && boot_cpu_data.x86 == 6 &&
+ if (mtrr_if == &generic_mtrr_ops && boot_cpu_data.x86 == 6 &&
boot_cpu_data.x86_model == 1 &&
boot_cpu_data.x86_stepping <= 7) {
if (base & ((1 << (22 - PAGE_SHIFT)) - 1)) {
@@ -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 & MTRR_CONFIG_NUM_VAR_MASK;
}
@@ -13,7 +13,7 @@
extern unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
struct mtrr_ops {
- u32 vendor;
+ u32 var_regs;
void (*set)(unsigned int reg, unsigned long base,
unsigned long size, mtrr_type type);
void (*get)(unsigned int reg, unsigned long *base,
@@ -54,8 +54,6 @@ bool get_mtrr_state(void);
extern u64 size_or_mask, size_and_mask;
extern const struct mtrr_ops *mtrr_if;
-#define is_cpu(vnd) (mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
-
extern unsigned int num_var_ranges;
extern u64 mtrr_tom2;
extern struct mtrr_state_type mtrr_state;