Split generic_set_all() into multiple parts, while moving the main
function body into cacheinfo.c.
Prepare the support of PAT without needing MTRR support by
moving the main function body of generic_set_all() into cacheinfo.c
while renaming it to cache_cpu_init(). The MTRR specific parts are
moved into a dedicated small function called by cache_cpu_init() in
order to make cache_cpu_init() as MTRR agnostic as possible.
The setting of smp_changes_mask is merged into the (new) function
mtrr_generic_set_state() used to call set_mtrr_state(). It was
probably split in ancient times, as atomic operations while running
uncached might be quite expensive, but OTOH only systems with a
broken BIOS should ever require to set any bit in smp_changes_mask,
so just hurting those devices with a penalty of a few microseconds
during boot shouldn't be a real issue.
Signed-off-by: Juergen Gross <[email protected]>
---
V2:
- new patch
V4:
- remove some comments (Borislav Petkov)
---
arch/x86/include/asm/cacheinfo.h | 1 +
arch/x86/include/asm/mtrr.h | 2 ++
arch/x86/kernel/cpu/cacheinfo.c | 17 +++++++++++++++++
arch/x86/kernel/cpu/mtrr/generic.c | 15 ++-------------
4 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 6159874b4183..978bac70fd49 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -12,5 +12,6 @@ void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
void cache_disable(void);
void cache_enable(void);
+void cache_cpu_init(void);
#endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 12a16caed395..986249a2b9b6 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -50,6 +50,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
extern int amd_special_default_mtrr(void);
void mtrr_disable(void);
void mtrr_enable(void);
+void mtrr_generic_set_state(void);
# else
static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
{
@@ -91,6 +92,7 @@ static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
#define mtrr_bp_restore() do {} while (0)
#define mtrr_disable() do {} while (0)
#define mtrr_enable() do {} while (0)
+#define mtrr_generic_set_state() do {} while (0)
# endif
#ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index ff32b2b1ca23..49b60a427fc9 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -1120,3 +1120,20 @@ void cache_enable(void) __releases(cache_disable_lock)
raw_spin_unlock(&cache_disable_lock);
}
+
+void cache_cpu_init(void)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ cache_disable();
+
+ if (memory_caching_control & CACHE_MTRR)
+ mtrr_generic_set_state();
+
+ if (memory_caching_control & CACHE_PAT)
+ pat_init();
+
+ cache_enable();
+ local_irq_restore(flags);
+}
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index bfe13eedaca8..32aebed25e3f 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -731,30 +731,19 @@ void mtrr_enable(void)
mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
}
-static void generic_set_all(void)
+void mtrr_generic_set_state(void)
{
unsigned long mask, count;
- unsigned long flags;
-
- local_irq_save(flags);
- cache_disable();
/* Actually set the state */
mask = set_mtrr_state();
- /* also set PAT */
- pat_init();
-
- cache_enable();
- local_irq_restore(flags);
-
/* Use the atomic bitops to update the global mask */
for (count = 0; count < sizeof(mask) * 8; ++count) {
if (mask & 0x01)
set_bit(count, &smp_changes_mask);
mask >>= 1;
}
-
}
/**
@@ -854,7 +843,7 @@ int positive_have_wrcomb(void)
* Generic structure...
*/
const struct mtrr_ops generic_mtrr_ops = {
- .set_all = generic_set_all,
+ .set_all = cache_cpu_init,
.get = generic_get_mtrr,
.get_free_region = generic_get_free_region,
.set = generic_set_mtrr,
--
2.35.3
On Tue, Oct 04, 2022 at 10:10:14AM +0200, Juergen Gross wrote:
> Split generic_set_all() into multiple parts, while moving the main
> function body into cacheinfo.c.
>
> Prepare the support of PAT without needing MTRR support by
> moving the main function body of generic_set_all() into cacheinfo.c
> while renaming it to cache_cpu_init(). The MTRR specific parts are
> moved into a dedicated small function called by cache_cpu_init() in
> order to make cache_cpu_init() as MTRR agnostic as possible.
>
> The setting of smp_changes_mask is merged into the (new) function
> mtrr_generic_set_state() used to call set_mtrr_state(). It was
> probably split in ancient times, as atomic operations while running
> uncached might be quite expensive, but OTOH only systems with a
> broken BIOS should ever require to set any bit in smp_changes_mask,
> so just hurting those devices with a penalty of a few microseconds
> during boot shouldn't be a real issue.
This still needs addressing
"So the commit message should not say what you're doing - that should
be visible from the diff itself. It should talk more about the *why*
you're doing it."
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 26.10.22 12:37, Borislav Petkov wrote:
> On Tue, Oct 04, 2022 at 10:10:14AM +0200, Juergen Gross wrote:
>> Split generic_set_all() into multiple parts, while moving the main
>> function body into cacheinfo.c.
>>
>> Prepare the support of PAT without needing MTRR support by
>> moving the main function body of generic_set_all() into cacheinfo.c
>> while renaming it to cache_cpu_init(). The MTRR specific parts are
>> moved into a dedicated small function called by cache_cpu_init() in
>> order to make cache_cpu_init() as MTRR agnostic as possible.
>>
>> The setting of smp_changes_mask is merged into the (new) function
>> mtrr_generic_set_state() used to call set_mtrr_state(). It was
>> probably split in ancient times, as atomic operations while running
>> uncached might be quite expensive, but OTOH only systems with a
>> broken BIOS should ever require to set any bit in smp_changes_mask,
>> so just hurting those devices with a penalty of a few microseconds
>> during boot shouldn't be a real issue.
>
> This still needs addressing
>
> "So the commit message should not say what you're doing - that should
> be visible from the diff itself. It should talk more about the *why*
> you're doing it."
>
The reason is "Prepare the support of PAT without needing MTRR support".
DYM I should just remove the rest of the commit message?
Juergen
On Wed, Oct 26, 2022 at 01:43:52PM +0200, Juergen Gross wrote:
> The reason is "Prepare the support of PAT without needing MTRR support".
>
> DYM I should just remove the rest of the commit message?
I mean something like this:
"Disentangle MTRR init from PAT init.
Add a main cache_cpu_init() init routine which initializes MTRR and/or
PAT support depending on what has been detected on the system.
Leave the MTRR-specific initialization in a MTRR-specific init function
where the smp_changes_mask setting happens now with caches disabled.
This global mask update was done with caches enabled before probably
because atomic operations while running uncached might have been quite
expensive.
But since only systems with a broken BIOS should ever require to set any
bit in smp_changes_mask, hurting those devices with a penalty of a few
microseconds during boot shouldn't be a real issue."
A commit message should talk about why the change is done - not, what is
being done. The what is clear from the diff.
It is way more important to state why and the direction this is
taking and what the author's concerns were while doing it. Like the
bit about the smp_changes_mask - that is important.
The fact that generic_set_all() is gutted out and renamed to
mtrr_generic_set_state() - not so much and also visible.
:-)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette