This series tries to fix the rather special case of PAT being available
without having MTRRs (either due to CONFIG_MTRR being not set, or
because the feature has been disabled e.g. by a hypervisor).
The main use cases are Xen PV guests and SEV-SNP guests running under
Hyper-V.
Patch 2 seems to be a little hacky, as it special cases only
memtype_reserve() and memtype_free(), but OTOH this doesn't seem to
be worse than in previous days, where PAT was disabled when MTRRs
haven't been available.
My tests with Xen didn't show any problems, but I'm rather sure I
couldn't cover all corner cases.
The only cleaner solution I could think of would be to introduce MTRR
read-only access. It would theoretically be possible to get the actual
MTRR contents for the variable MTRRs from Xen, but I'm not sure this
is really the way to go.
For the SEV-SNP case with Hyper-V I guess such a read-only mode could
be rather simple, but I'm really not sure this would cover all needed
corner cases (I'd basically say always "WB" in that case).
I have added more cleanup which has been discussed when looking into
the most recent failures.
Juergen Gross (6):
x86/mtrr: make mtrr_enabled() non-static
x86/pat: check for MTRRs enabled in memtype_reserve()
x86/mtrr: revert commit 90b926e68f50
x86/mtrr: don't let mtrr_type_lookup() return MTRR_TYPE_INVALID
x86/mm: only check uniform after calling mtrr_type_lookup()
x86/mtrr: drop sanity check in mtrr_type_lookup_fixed()
arch/x86/include/asm/mtrr.h | 13 +++++++++++--
arch/x86/include/uapi/asm/mtrr.h | 6 +++---
arch/x86/kernel/cpu/mtrr/generic.c | 10 +++-------
arch/x86/kernel/cpu/mtrr/mtrr.c | 2 +-
arch/x86/mm/pat/memtype.c | 13 ++++++++-----
arch/x86/mm/pgtable.c | 6 ++----
6 files changed, 28 insertions(+), 22 deletions(-)
--
2.35.3
In order to be able to use mtrr_enabled() outside of MTRR code, make
it non-static and add a sub for the !CONFIG_MTRR case.
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/include/asm/mtrr.h | 6 ++++++
arch/x86/kernel/cpu/mtrr/mtrr.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f0eeaf6e5f5f..29ec2d6f0537 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -31,6 +31,7 @@
*/
# ifdef CONFIG_MTRR
void mtrr_bp_init(void);
+bool mtrr_enabled(void);
extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
extern void mtrr_save_fixed_ranges(void *);
extern void mtrr_save_state(void);
@@ -48,6 +49,11 @@ void mtrr_disable(void);
void mtrr_enable(void);
void mtrr_generic_set_state(void);
# else
+static inline bool mtrr_enabled(void)
+{
+ return false;
+}
+
static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
{
/*
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 783f3210d582..814cc13fd6eb 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -59,7 +59,7 @@
#define MTRR_TO_PHYS_WC_OFFSET 1000
u32 num_var_ranges;
-static bool mtrr_enabled(void)
+bool mtrr_enabled(void)
{
return !!mtrr_if;
}
--
2.35.3
Today memtype_reserve() bails out early if pat_enabled() returns false.
The same can be done in case MTRRs aren't enabled.
This will reinstate the behavior of memtype_reserve() before commit
72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
running on Xen"). There have been reports about that commit breaking
SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
but that again resulted in problems with Xen PV guests.
Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/mm/pat/memtype.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index fb4b1b5e0dea..18f612b43763 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
return -EINVAL;
}
- if (!pat_enabled()) {
- /* This is identical to page table setting without PAT */
+ /*
+ * PAT disabled or MTRRs disabled don't require any memory type
+ * tracking or type adjustments, as there can't be any conflicts
+ * between PAT and MTRRs with at least one of both being disabled.
+ */
+ if (!pat_enabled() || !mtrr_enabled()) {
if (new_type)
*new_type = req_type;
return 0;
@@ -627,7 +631,7 @@ int memtype_free(u64 start, u64 end)
int is_range_ram;
struct memtype *entry_old;
- if (!pat_enabled())
+ if (!pat_enabled() || !mtrr_enabled())
return 0;
start = sanitize_phys(start);
--
2.35.3
Commit 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled
case") has introduced a regression with Xen.
Revert the patch.
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/mm/pat/memtype.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 18f612b43763..2c6d95f95b49 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -387,8 +387,7 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end,
u8 mtrr_type, uniform;
mtrr_type = mtrr_type_lookup(start, end, &uniform);
- if (mtrr_type != MTRR_TYPE_WRBACK &&
- mtrr_type != MTRR_TYPE_INVALID)
+ if (mtrr_type != MTRR_TYPE_WRBACK)
return _PAGE_CACHE_MODE_UC_MINUS;
return _PAGE_CACHE_MODE_WB;
--
2.35.3
mtrr_type_lookup() should always return a valid memory type. In case
there is no information available, it should return the default UC.
At the same time the mtrr_type_lookup() stub for the !CONFIG_MTRR
case should set uniform to 1, as if the memory range would be
covered by no MTRR at all.
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/include/asm/mtrr.h | 7 +++++--
arch/x86/kernel/cpu/mtrr/generic.c | 4 ++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 29ec2d6f0537..c12c73d48f08 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -57,9 +57,12 @@ static inline bool mtrr_enabled(void)
static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
{
/*
- * Return no-MTRRs:
+ * Return the default MTRR type, without any known other types in
+ * that range.
*/
- return MTRR_TYPE_INVALID;
+ *uniform = 1;
+
+ return MTRR_TYPE_UNCACHABLE;
}
#define mtrr_save_fixed_ranges(arg) do {} while (0)
#define mtrr_save_state() do {} while (0)
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index ee09d359e08f..c749ec4436a1 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -262,10 +262,10 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
end--;
if (!mtrr_state_set)
- return MTRR_TYPE_INVALID;
+ return MTRR_TYPE_UNCACHABLE;
if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
- return MTRR_TYPE_INVALID;
+ return MTRR_TYPE_UNCACHABLE;
/*
* Look up the fixed ranges first, which take priority over
--
2.35.3
Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
WB or INVALID after calling mtrr_type_lookup(). Those tests can be
dropped, as the only reason to not use a large mapping would be
uniform being 0. Any MTRR type can be accepted as long as it applies
to the whole memory range covered by the mapping, as the alternative
would only be to map the same region with smaller pages instead using
the same PAT type as for the large mapping.
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/mm/pgtable.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index e4f499eb0f29..7b9c5443d176 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
u8 mtrr, uniform;
mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
- if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
- (mtrr != MTRR_TYPE_WRBACK))
+ if (!uniform)
return 0;
/* Bail out if we are we on a populated non-leaf entry: */
@@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
u8 mtrr, uniform;
mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
- if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
- (mtrr != MTRR_TYPE_WRBACK)) {
+ if (!uniform) {
pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
__func__, addr, addr + PMD_SIZE);
return 0;
--
2.35.3
mtrr_type_lookup_fixed() contains a sanity check for the case it is
being called with a start address above 1 MB. As it is static and it
is called only iff the start address is below 1MB, this sanity check
can be dropped.
This will remove the last case where mtrr_type_lookup() can return
MTRR_TYPE_INVALID, so adjust the comment in include/uapi/asm/mtrr.h.
Note that removing the MTRR_TYPE_INVALID #define from that header
could break user code, so it has to stay.
Signed-off-by: Juergen Gross <[email protected]>
---
arch/x86/include/uapi/asm/mtrr.h | 6 +++---
arch/x86/kernel/cpu/mtrr/generic.c | 6 +-----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
index 376563f2bac1..4aa05c2ffa78 100644
--- a/arch/x86/include/uapi/asm/mtrr.h
+++ b/arch/x86/include/uapi/asm/mtrr.h
@@ -115,9 +115,9 @@ struct mtrr_state_type {
#define MTRR_NUM_TYPES 7
/*
- * Invalid MTRR memory type. mtrr_type_lookup() returns this value when
- * MTRRs are disabled. Note, this value is allocated from the reserved
- * values (0x7-0xff) of the MTRR memory types.
+ * Invalid MTRR memory type. No longer used outside of MTRR code.
+ * Note, this value is allocated from the reserved values (0x7-0xff) of
+ * the MTRR memory types.
*/
#define MTRR_TYPE_INVALID 0xff
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index c749ec4436a1..c17cb00aac6a 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -114,16 +114,12 @@ static int check_type_overlap(u8 *prev, u8 *curr)
* 0xC0000 - 0xFFFFF : This range is divided into sixty-four 4KB sub-ranges
*
* Return Values:
- * MTRR_TYPE_(type) - Matched memory type
- * MTRR_TYPE_INVALID - Unmatched
+ * MTRR_TYPE_(type) - Memory type
*/
static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
{
int idx;
- if (start >= 0x100000)
- return MTRR_TYPE_INVALID;
-
/* 0x0 - 0x7FFFF */
if (start < 0x80000) {
idx = 0;
--
2.35.3
* Juergen Gross <[email protected]> wrote:
> Today memtype_reserve() bails out early if pat_enabled() returns false.
> The same can be done in case MTRRs aren't enabled.
>
> This will reinstate the behavior of memtype_reserve() before commit
> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
> running on Xen"). There have been reports about that commit breaking
> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
> but that again resulted in problems with Xen PV guests.
>
> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> arch/x86/mm/pat/memtype.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index fb4b1b5e0dea..18f612b43763 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
> return -EINVAL;
> }
>
> - if (!pat_enabled()) {
> - /* This is identical to page table setting without PAT */
> + /*
> + * PAT disabled or MTRRs disabled don't require any memory type
> + * tracking or type adjustments, as there can't be any conflicts
> + * between PAT and MTRRs with at least one of both being disabled.
> + */
> + if (!pat_enabled() || !mtrr_enabled()) {
> if (new_type)
> *new_type = req_type;
Doesn't memtype_reserve() also check for overlapping ranges & type
compatibility in memtype_check_conflict(), etc., which can occur even in a
pure PAT setup? Ie. are we 100% sure that in the !MTRR case it would be a
NOP?
But even if it's a functional NOP as you claim, we'd still be better off if
the memtype tree was still intact - instead of just turning off the API.
Also, speling nit:
s/one of both
/one or both
Thanks,
Ingo
On 07.02.23 09:49, Ingo Molnar wrote:
>
> * Juergen Gross <[email protected]> wrote:
>
>> Today memtype_reserve() bails out early if pat_enabled() returns false.
>> The same can be done in case MTRRs aren't enabled.
>>
>> This will reinstate the behavior of memtype_reserve() before commit
>> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
>> running on Xen"). There have been reports about that commit breaking
>> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
>> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
>> but that again resulted in problems with Xen PV guests.
>>
>> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
>> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
>> Signed-off-by: Juergen Gross <[email protected]>
>> ---
>> arch/x86/mm/pat/memtype.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
>> index fb4b1b5e0dea..18f612b43763 100644
>> --- a/arch/x86/mm/pat/memtype.c
>> +++ b/arch/x86/mm/pat/memtype.c
>> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
>> return -EINVAL;
>> }
>>
>> - if (!pat_enabled()) {
>> - /* This is identical to page table setting without PAT */
>> + /*
>> + * PAT disabled or MTRRs disabled don't require any memory type
>> + * tracking or type adjustments, as there can't be any conflicts
>> + * between PAT and MTRRs with at least one of both being disabled.
>> + */
>> + if (!pat_enabled() || !mtrr_enabled()) {
>> if (new_type)
>> *new_type = req_type;
>
> Doesn't memtype_reserve() also check for overlapping ranges & type
> compatibility in memtype_check_conflict(), etc., which can occur even in a
> pure PAT setup? Ie. are we 100% sure that in the !MTRR case it would be a
> NOP?
>
> But even if it's a functional NOP as you claim, we'd still be better off if
> the memtype tree was still intact - instead of just turning off the API.
Yes, that's basically the issue discussed in [patch 0/6].
It should still be better than the original case (PAT and MTRR off, but
the ability to use PAT nevertheless), though.
>
> Also, speling nit:
>
> s/one of both
> /one or both
Hmm, but only if I drop the "at least". I don't really mind either way.
Juergen
On Tue, Feb 07, 2023 at 08:28:58AM +0100, Juergen Gross wrote:
> Today memtype_reserve() bails out early if pat_enabled() returns false.
> The same can be done in case MTRRs aren't enabled.
>
> This will reinstate the behavior of memtype_reserve() before commit
> 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when
> running on Xen"). There have been reports about that commit breaking
> SEV-SNP guests under Hyper-V, which was tried to be resolved by commit
> 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case"),
> but that again resulted in problems with Xen PV guests.
No, no commit message text with references to other commits.
Considering how this is one of those "let's upset the universe" thing of
decoupling MTRRs from PAT and how it breaks in weird ways, if we ever
end up doing that, then we need to explain *exactly* why we're doing it.
And in detail.
Because otherwise, in the future, we'll end up scratching heads just
like we're doing now as to why the large page thing allowed those
certain types, and so on and so on.
> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
> Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> arch/x86/mm/pat/memtype.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index fb4b1b5e0dea..18f612b43763 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -557,8 +557,12 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type,
> return -EINVAL;
> }
>
> - if (!pat_enabled()) {
> - /* This is identical to page table setting without PAT */
> + /*
> + * PAT disabled or MTRRs disabled don't require any memory type
> + * tracking or type adjustments, as there can't be any conflicts
> + * between PAT and MTRRs with at least one of both being disabled.
> + */
> + if (!pat_enabled() || !mtrr_enabled()) {
Yah, looks straight-forward to me but I have said this before. And we
have broken shit so if anything, this needs to be tested on everything
before we go with it.
IMNSVHO.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
> Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
> WB or INVALID after calling mtrr_type_lookup(). Those tests can be
> dropped, as the only reason to not use a large mapping would be
> uniform being 0. Any MTRR type can be accepted as long as it applies
> to the whole memory range covered by the mapping, as the alternative
> would only be to map the same region with smaller pages instead using
> the same PAT type as for the large mapping.
>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> arch/x86/mm/pgtable.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index e4f499eb0f29..7b9c5443d176 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> u8 mtrr, uniform;
>
> mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
> - if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
> - (mtrr != MTRR_TYPE_WRBACK))
> + if (!uniform)
> return 0;
>
> /* Bail out if we are we on a populated non-leaf entry: */
> @@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> u8 mtrr, uniform;
>
> mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> - if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
> - (mtrr != MTRR_TYPE_WRBACK)) {
> + if (!uniform) {
> pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
> __func__, addr, addr + PMD_SIZE);
> return 0;
> --
See my reply here:
https://lore.kernel.org/all/[email protected]
I understand it as WB is ok, for example, even if not uniform. That
thing in mtrr_type_lookup():
/*
* Look up the fixed ranges first, which take priority over
* the variable ranges.
*/
if ((start < 0x100000) &&
(mtrr_state.have_fixed) &&
(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
is_uniform = 0;
type = mtrr_type_lookup_fixed(start, end);
goto out;
}
If that can return WB, then I guess that says it is still ok. Can the
fixed ranges even cover a, at least a PMD? I guess I need to stare at
this more.
Lemme add Toshi who authored that code - he might have a comment or two.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 07.02.23 12:42, Borislav Petkov wrote:
> On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
>> Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
>> WB or INVALID after calling mtrr_type_lookup(). Those tests can be
>> dropped, as the only reason to not use a large mapping would be
>> uniform being 0. Any MTRR type can be accepted as long as it applies
>> to the whole memory range covered by the mapping, as the alternative
>> would only be to map the same region with smaller pages instead using
>> the same PAT type as for the large mapping.
>>
>> Suggested-by: Linus Torvalds <[email protected]>
>> Signed-off-by: Juergen Gross <[email protected]>
>> ---
>> arch/x86/mm/pgtable.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>> index e4f499eb0f29..7b9c5443d176 100644
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -721,8 +721,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>> u8 mtrr, uniform;
>>
>> mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
>> - if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
>> - (mtrr != MTRR_TYPE_WRBACK))
>> + if (!uniform)
>> return 0;
>>
>> /* Bail out if we are we on a populated non-leaf entry: */
>> @@ -748,8 +747,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>> u8 mtrr, uniform;
>>
>> mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
>> - if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
>> - (mtrr != MTRR_TYPE_WRBACK)) {
>> + if (!uniform) {
>> pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
>> __func__, addr, addr + PMD_SIZE);
>> return 0;
>> --
>
> See my reply here:
>
> https://lore.kernel.org/all/[email protected]
>
> I understand it as WB is ok, for example, even if not uniform. That
> thing in mtrr_type_lookup():
>
> /*
> * Look up the fixed ranges first, which take priority over
> * the variable ranges.
> */
> if ((start < 0x100000) &&
> (mtrr_state.have_fixed) &&
> (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
> is_uniform = 0;
> type = mtrr_type_lookup_fixed(start, end);
> goto out;
> }
>
> If that can return WB, then I guess that says it is still ok. Can the
> fixed ranges even cover a, at least a PMD? I guess I need to stare at
> this more.
Fixed MTRRs are all below 1MB. So no, they can't cover a PMD.
Juergen
On Tue, Feb 07, 2023 at 12:54:53PM +0100, Juergen Gross wrote:
> Fixed MTRRs are all below 1MB. So no, they can't cover a PMD.
Good, belongs in the commit message.
And we need more code staring like this to make sure nothing else
breaks. As said, upsetting the universe is not easy.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 6, 2023 at 11:29 PM Juergen Gross <[email protected]> wrote:
>
> mtrr_type_lookup() should always return a valid memory type. In case
> there is no information available, it should return the default UC.
Why isn't the second case (ie MTRR_STATE_MTRR_ENABLED not being set)
returning 'mtrr_state.def_type'. That's what the hw does, no?
> At the same time the mtrr_type_lookup() stub for the !CONFIG_MTRR
> case should set uniform to 1, as if the memory range would be
> covered by no MTRR at all.
.. but you didn't do that for the CONFIG_MTRR, so now they return
MTRR_TYPE_UNCACHABLE, but don't set uniform=1.
Linus
On 07.02.23 12:42, Borislav Petkov wrote:
> On Tue, Feb 07, 2023 at 08:29:01AM +0100, Juergen Gross wrote:
> > Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
> > WB or INVALID after calling mtrr_type_lookup(). Those tests can be
> > dropped, as the only reason to not use a large mapping would be
> > uniform being 0. Any MTRR type can be accepted as long as it applies
> > to the whole memory range covered by the mapping, as the alternative
> > would only be to map the same region with smaller pages instead using
> > the same PAT type as for the large mapping.
:
> Lemme add Toshi who authored that code - he might have a comment or
> two.
The current mtrr_type_look() does not set 'uniform' for MTRR_TYPE_INVALID.
Please also update it to set 'uniform', if not done in other patches.
Toshi
On 07.02.23 17:20, Linus Torvalds wrote:
> On Mon, Feb 6, 2023 at 11:29 PM Juergen Gross <[email protected]> wrote:
>>
>> mtrr_type_lookup() should always return a valid memory type. In case
>> there is no information available, it should return the default UC.
>
> Why isn't the second case (ie MTRR_STATE_MTRR_ENABLED not being set)
> returning 'mtrr_state.def_type'. That's what the hw does, no?
Are you sure? In the SDM I'm reading:
* E (MTRRs enabled) flag, bit 11 — MTRRs are enabled when set; all MTRRs are
disabled when clear, and the UC memory type is applied to all of physical
memory.
So UC it should be IMHO.
>> At the same time the mtrr_type_lookup() stub for the !CONFIG_MTRR
>> case should set uniform to 1, as if the memory range would be
>> covered by no MTRR at all.
>
> .. but you didn't do that for the CONFIG_MTRR, so now they return
> MTRR_TYPE_UNCACHABLE, but don't set uniform=1.
I agree that setting uniform should be done at least for the case of not
enabled MTRRs.
The case of !mtrr_state_set is different, as the data regarding setting
uniform isn't known yet. OTOH there is no caller of mtrr_type_lookup()
interested in the uniform setting so early in boot, so I guess not
setting it is fine (setting it would be okay for the same reason, but
it would be technically wrong IMO).
Juergen
From: Juergen Gross <[email protected]> Sent: Monday, February 6, 2023 11:29 PM
>
> This series tries to fix the rather special case of PAT being available
> without having MTRRs (either due to CONFIG_MTRR being not set, or
> because the feature has been disabled e.g. by a hypervisor).
>
> The main use cases are Xen PV guests and SEV-SNP guests running under
> Hyper-V.
>
> Patch 2 seems to be a little hacky, as it special cases only
> memtype_reserve() and memtype_free(), but OTOH this doesn't seem to
> be worse than in previous days, where PAT was disabled when MTRRs
> haven't been available.
>
> My tests with Xen didn't show any problems, but I'm rather sure I
> couldn't cover all corner cases.
I tested this patch set with Hyper-V SEV-SNP guests, and ioremap_cache()
is correctly mapping as WB.
As an observation, with commit 90b926e68f50 it was nice to have
the memtype entries created. I could check for any unexpected
mappings in /sys/kernel/debug/x86/pat_memtype_list. With this patch
set, we're back to not creating those entries.
Michael
>
> The only cleaner solution I could think of would be to introduce MTRR
> read-only access. It would theoretically be possible to get the actual
> MTRR contents for the variable MTRRs from Xen, but I'm not sure this
> is really the way to go.
>
> For the SEV-SNP case with Hyper-V I guess such a read-only mode could
> be rather simple, but I'm really not sure this would cover all needed
> corner cases (I'd basically say always "WB" in that case).
>
> I have added more cleanup which has been discussed when looking into
> the most recent failures.
>
> Juergen Gross (6):
> x86/mtrr: make mtrr_enabled() non-static
> x86/pat: check for MTRRs enabled in memtype_reserve()
> x86/mtrr: revert commit 90b926e68f50
> x86/mtrr: don't let mtrr_type_lookup() return MTRR_TYPE_INVALID
> x86/mm: only check uniform after calling mtrr_type_lookup()
> x86/mtrr: drop sanity check in mtrr_type_lookup_fixed()
>
> arch/x86/include/asm/mtrr.h | 13 +++++++++++--
> arch/x86/include/uapi/asm/mtrr.h | 6 +++---
> arch/x86/kernel/cpu/mtrr/generic.c | 10 +++-------
> arch/x86/kernel/cpu/mtrr/mtrr.c | 2 +-
> arch/x86/mm/pat/memtype.c | 13 ++++++++-----
> arch/x86/mm/pgtable.c | 6 ++----
> 6 files changed, 28 insertions(+), 22 deletions(-)
>
> --
> 2.35.3
On 08.02.23 16:08, Michael Kelley (LINUX) wrote:
> From: Juergen Gross <[email protected]> Sent: Monday, February 6, 2023 11:29 PM
>>
>> This series tries to fix the rather special case of PAT being available
>> without having MTRRs (either due to CONFIG_MTRR being not set, or
>> because the feature has been disabled e.g. by a hypervisor).
>>
>> The main use cases are Xen PV guests and SEV-SNP guests running under
>> Hyper-V.
>>
>> Patch 2 seems to be a little hacky, as it special cases only
>> memtype_reserve() and memtype_free(), but OTOH this doesn't seem to
>> be worse than in previous days, where PAT was disabled when MTRRs
>> haven't been available.
>>
>> My tests with Xen didn't show any problems, but I'm rather sure I
>> couldn't cover all corner cases.
>
> I tested this patch set with Hyper-V SEV-SNP guests, and ioremap_cache()
> is correctly mapping as WB.
>
> As an observation, with commit 90b926e68f50 it was nice to have
> the memtype entries created. I could check for any unexpected
> mappings in /sys/kernel/debug/x86/pat_memtype_list. With this patch
> set, we're back to not creating those entries.
I'm currently looking into the solution mentioned below. This might turn
out to be much cleaner.
>
> Michael
>
>>
>> The only cleaner solution I could think of would be to introduce MTRR
>> read-only access. It would theoretically be possible to get the actual
>> MTRR contents for the variable MTRRs from Xen, but I'm not sure this
>> is really the way to go.
>>
>> For the SEV-SNP case with Hyper-V I guess such a read-only mode could
>> be rather simple, but I'm really not sure this would cover all needed
>> corner cases (I'd basically say always "WB" in that case).
Juergen
On Tue, Feb 7, 2023 at 10:20 PM Juergen Gross <[email protected]> wrote:
>
> Are you sure? In the SDM I'm reading:
>
> * E (MTRRs enabled) flag, bit 11 — MTRRs are enabled when set; all MTRRs are
> disabled when clear, and the UC memory type is applied to all of physical
> memory.
>
> So UC it should be IMHO.
Right you are. I clearly misread that section when I did my original patch.
Linus