LinuxLists.cc - x86.git: mtrr trimming removes all memory under kvm

[permalink] [raw]

Subject: Re: x86.git: mtrr trimming removes all memory under kvm

Jeremy Fitzhardinge wrote:
> When booting a current x86.git kernel under kvm, I get this:
>
> (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> early_ioremap_init()
> BIOS-provided physical RAM map:
> BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> console [earlyser0] enabled
> 0MB HIGHMEM available.
> 511MB LOWMEM available.
> Scan SMP from c0000000 for 1024 bytes.
> Scan SMP from c009fc00 for 1024 bytes.
> Scan SMP from c00f0000 for 65536 bytes.
> Scan SMP from c009fc00 for 1024 bytes.
> ***************
> **** WARNING: likely BIOS bug
> **** MTRRs don't cover all of memory, trimmed 131056 pages
> ***************

Looks like the code doesn't check that the CPU *has* MTRRs...

-hpa

2008-01-25 02:05:34

[permalink] [raw]

Subject: Re: x86.git: mtrr trimming removes all memory under kvm

2008-01-25 02:26:32

[permalink] [raw]

Subject: Re: x86.git: mtrr trimming removes all memory under kvm

On Thursday 24 January 2008 05:49:00 pm H. Peter Anvin wrote:
> Jeremy Fitzhardinge wrote:
> > When booting a current x86.git kernel under kvm, I get this:
> >
> > (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> > (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> > early_ioremap_init()
> > BIOS-provided physical RAM map:
> > BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> > BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> > BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> > BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> > BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> > BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> > console [earlyser0] enabled
> > 0MB HIGHMEM available.
> > 511MB LOWMEM available.
> > Scan SMP from c0000000 for 1024 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > Scan SMP from c00f0000 for 65536 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > ***************
> > **** WARNING: likely BIOS bug
> > **** MTRRs don't cover all of memory, trimmed 131056 pages
> > ***************
>
> Looks like the code doesn't check that the CPU *has* MTRRs...

please try this

[PATCH] x86: trim RAM need to check if mtrr is there

Signed-off-by: Yinghai Lu <[email protected]>

diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index a1551d0..a0b6f55 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -646,9 +646,6 @@ static __init int amd_special_default_mtrr(unsigned long end_pfn)
{
u32 l, h;

- /* Doesn't apply to memory < 4GB */
- if (end_pfn <= (0xffffffff >> PAGE_SHIFT))
- return 0;
if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
return 0;
if (boot_cpu_data.x86 < 0xf || boot_cpu_data.x86 > 0x11)
@@ -682,6 +679,12 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)
mtrr_type type;
u64 trim_start, trim_size;

+ /* Doesn't apply to memory < 4GB */
+ if (end_pfn <= (0xffffffffUL >> PAGE_SHIFT))
+ return 0;
+
+ if (!cpu_has_mtrr)
+ return 0;
/*
* Make sure we only trim uncachable memory on machines that
* support the Intel MTRR architecture:

2008-01-25 03:41:32

[permalink] [raw]

Subject: [PATCH] x86: trim ram need to check if mtrr is there v2

[PATCH] x86: trim ram need to check if mtrr is there v2

> >Jeremy Fitzhardinge wrote:
> > When booting a current x86.git kernel under kvm, I get this:
> >
> > (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> > (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> > early_ioremap_init()
> > BIOS-provided physical RAM map:
> > BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> > BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> > BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> > BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> > BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> > BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> > console [earlyser0] enabled
> > 0MB HIGHMEM available.
> > 511MB LOWMEM available.
> > Scan SMP from c0000000 for 1024 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > Scan SMP from c00f0000 for 65536 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > ***************
> > **** WARNING: likely BIOS bug
> > **** MTRRs don't cover all of memory, trimmed 131056 pages
> > ***************
>
> H. Peter Anvin wrote:
> Looks like the code doesn't check that the CPU *has* MTRRs...

so check it mtrr is there, also check if mem less 4G and is AMD as early

Signed-off-by: Yinghai Lu <[email protected]>

Index: linux-2.6/arch/x86/kernel/cpu/mtrr/main.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/mtrr/main.c
+++ linux-2.6/arch/x86/kernel/cpu/mtrr/main.c
@@ -642,13 +642,10 @@ early_param("disable_mtrr_trim", disable
#define Tom2Enabled (1U << 21)
#define Tom2ForceMemTypeWB (1U << 22)

-static __init int amd_special_default_mtrr(unsigned long end_pfn)
+static __init int amd_special_default_mtrr(void)
{
u32 l, h;

- /* Doesn't apply to memory < 4GB */
- if (end_pfn <= (0xffffffff >> PAGE_SHIFT))
- return 0;
if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
return 0;
if (boot_cpu_data.x86 < 0xf || boot_cpu_data.x86 > 0x11)
@@ -682,6 +679,13 @@ int __init mtrr_trim_uncached_memory(uns
mtrr_type type;
u64 trim_start, trim_size;

+ if (!cpu_has_mtrr)
+ return 0;
+
+ /* Doesn't apply to memory < 4GB */
+ if (end_pfn <= (0xffffffffUL >> PAGE_SHIFT))
+ return 0;
+
/*
* Make sure we only trim uncachable memory on machines that
* support the Intel MTRR architecture:
@@ -691,6 +695,9 @@ int __init mtrr_trim_uncached_memory(uns
if (!is_cpu(INTEL) || disable_mtrr_trim || def != MTRR_TYPE_UNCACHABLE)
return 0;

+ if (amd_special_default_mtrr())
+ return 0;
+
/* Find highest cached pfn */
for (i = 0; i < num_var_ranges; i++) {
mtrr_if->get(i, &base, &size, &type);
@@ -702,9 +709,6 @@ int __init mtrr_trim_uncached_memory(uns
highest_addr = base + size;
}

- if (amd_special_default_mtrr(end_pfn))
- return 0;
-
if ((highest_addr >> PAGE_SHIFT) < end_pfn) {
printk(KERN_WARNING "***************\n");
printk(KERN_WARNING "**** WARNING: likely BIOS bug\n");

2008-01-25 05:53:34

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

Yinghai Lu wrote:
>> H. Peter Anvin wrote:
>> Looks like the code doesn't check that the CPU *has* MTRRs...
>
> so check it mtrr is there, also check if mem less 4G and is AMD as early

Why the check for < 4 GB? The same thing applies to memory below the 4
GB limit -- in fact, we've had a number of that kind of systems in the past.

-hpa

2008-01-25 07:59:57

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

Yinghai Lu wrote:
> [PATCH] x86: trim ram need to check if mtrr is there v2
>
>
>>> Jeremy Fitzhardinge wrote:
>>> When booting a current x86.git kernel under kvm, I get this:
>>>
>>> (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
>>> (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
>>> early_ioremap_init()
>>> BIOS-provided physical RAM map:
>>> BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
>>> BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
>>> BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
>>> BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
>>> BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
>>> BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
>>> console [earlyser0] enabled
>>> 0MB HIGHMEM available.
>>> 511MB LOWMEM available.
>>> Scan SMP from c0000000 for 1024 bytes.
>>> Scan SMP from c009fc00 for 1024 bytes.
>>> Scan SMP from c00f0000 for 65536 bytes.
>>> Scan SMP from c009fc00 for 1024 bytes.
>>> ***************
>>> **** WARNING: likely BIOS bug
>>> **** MTRRs don't cover all of memory, trimmed 131056 pages
>>> ***************
>>>
>> H. Peter Anvin wrote:
>> Looks like the code doesn't check that the CPU *has* MTRRs...
>>
>
> so check it mtrr is there, also check if mem less 4G and is AMD as early
>

Thanks, this gets me to usermode under kvm.

J

2008-01-25 08:01:56

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

On Jan 24, 2008 9:44 PM, H. Peter Anvin <[email protected]> wrote:
> Yinghai Lu wrote:
> >> H. Peter Anvin wrote:
> >> Looks like the code doesn't check that the CPU *has* MTRRs...
> >
> > so check it mtrr is there, also check if mem less 4G and is AMD as early
>
> Why the check for < 4 GB? The same thing applies to memory below the 4
> GB limit -- in fact, we've had a number of that kind of systems in the past.

then we could remove that.

YH

2008-01-25 08:44:36

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

On Jan 24, 2008 11:43 PM, Jeremy Fitzhardinge <[email protected]> wrote:
>
> Yinghai Lu wrote:
> > [PATCH] x86: trim ram need to check if mtrr is there v2
> >
> >
> >>> Jeremy Fitzhardinge wrote:
> >>> When booting a current x86.git kernel under kvm, I get this:
> >>>
> >>> (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> >>> (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> >>> early_ioremap_init()
> >>> BIOS-provided physical RAM map:
> >>> BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> >>> BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> >>> BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> >>> BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> >>> BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> >>> BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> >>> console [earlyser0] enabled
> >>> 0MB HIGHMEM available.
> >>> 511MB LOWMEM available.
> >>> Scan SMP from c0000000 for 1024 bytes.
> >>> Scan SMP from c009fc00 for 1024 bytes.
> >>> Scan SMP from c00f0000 for 65536 bytes.
> >>> Scan SMP from c009fc00 for 1024 bytes.
> >>> ***************
> >>> **** WARNING: likely BIOS bug
> >>> **** MTRRs don't cover all of memory, trimmed 131056 pages
> >>> ***************
> >>>
> >> H. Peter Anvin wrote:
> >> Looks like the code doesn't check that the CPU *has* MTRRs...
> >>
> >
> > so check it mtrr is there, also check if mem less 4G and is AMD as early
> >
>
> Thanks, this gets me to usermode under kvm.

i think the is 0xffffffff to 0xffffffffUL make the difference.

the cpu mode in KVM/qemu may cpu_has_mtrr, but doesn't really have
return mtrr msr with correct value.

is_cpu(INTEL) already make sure we have mtrr_if got assigned and cpu_has_mtrr.

may need to fix qemu instead...

Can you try post /proc/mtrr for your guest in kvm/qemu?

Thanks

YH

2008-01-25 08:48:13

[permalink] [raw]

Subject: [PATCH] x86: trim ram need to check if mtrr is there v3

[PATCH] x86: trim ram need to check if mtrr is there v3

> >Jeremy Fitzhardinge wrote:
> > When booting a current x86.git kernel under kvm, I get this:
> >
> > (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> > (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> > early_ioremap_init()
> > BIOS-provided physical RAM map:
> > BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> > BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> > BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> > BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> > BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> > BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> > console [earlyser0] enabled
> > 0MB HIGHMEM available.
> > 511MB LOWMEM available.
> > Scan SMP from c0000000 for 1024 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > Scan SMP from c00f0000 for 65536 bytes.
> > Scan SMP from c009fc00 for 1024 bytes.
> > ***************
> > **** WARNING: likely BIOS bug
> > **** MTRRs don't cover all of memory, trimmed 131056 pages
> > ***************
>
> H. Peter Anvin wrote:
> Looks like the code doesn't check that the CPU *has* MTRRs...

so more strict check if mtrr is there really.
bail out if mtrr all blank when qemu cpu model is used

and check if is AMD as early
also remove 4G less check, according to hpa.

Signed-off-by: Yinghai Lu <[email protected]>

Index: linux-2.6/arch/x86/kernel/cpu/mtrr/main.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/mtrr/main.c
+++ linux-2.6/arch/x86/kernel/cpu/mtrr/main.c
@@ -642,13 +642,10 @@ early_param("disable_mtrr_trim", disable
#define Tom2Enabled (1U << 21)
#define Tom2ForceMemTypeWB (1U << 22)

-static __init int amd_special_default_mtrr(unsigned long end_pfn)
+static __init int amd_special_default_mtrr(void)
{
u32 l, h;

- /* Doesn't apply to memory < 4GB */
- if (end_pfn <= (0xffffffff >> PAGE_SHIFT))
- return 0;
if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
return 0;
if (boot_cpu_data.x86 < 0xf || boot_cpu_data.x86 > 0x11)
@@ -686,9 +683,14 @@ int __init mtrr_trim_uncached_memory(uns
* Make sure we only trim uncachable memory on machines that
* support the Intel MTRR architecture:
*/
+ if (!is_cpu(INTEL) || disable_mtrr_trim)
+ return 0;
rdmsr(MTRRdefType_MSR, def, dummy);
def &= 0xff;
- if (!is_cpu(INTEL) || disable_mtrr_trim || def != MTRR_TYPE_UNCACHABLE)
+ if (def != MTRR_TYPE_UNCACHABLE)
+ return 0;
+
+ if (amd_special_default_mtrr())
return 0;

/* Find highest cached pfn */
@@ -702,8 +704,14 @@ int __init mtrr_trim_uncached_memory(uns
highest_addr = base + size;
}

- if (amd_special_default_mtrr(end_pfn))
+ /* kvm/qemu doesn't have mtrr set right, don't trim them all */
+ if (!highest_addr) {
+ printk(KERN_WARNING "***************\n");
+ printk(KERN_WARNING "**** WARNING: likely strange cpu\n");
+ printk(KERN_WARNING "**** MTRRs all blank, cpu in qemu?\n");
+ printk(KERN_WARNING "***************\n");
return 0;
+ }

if ((highest_addr >> PAGE_SHIFT) < end_pfn) {
printk(KERN_WARNING "***************\n");

2008-01-25 08:48:59

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

On Jan 25, 2008 12:13 AM, Yinghai Lu <[email protected]> wrote:
>
> On Jan 24, 2008 11:43 PM, Jeremy Fitzhardinge <[email protected]> wrote:
> >
> > Yinghai Lu wrote:
> > > [PATCH] x86: trim ram need to check if mtrr is there v2
> > >
> > >
> > >>> Jeremy Fitzhardinge wrote:
> > >>> When booting a current x86.git kernel under kvm, I get this:
> > >>>
> > >>> (qemu) Linux version 2.6.24-rc8 (jeremy@ezr) (gcc version 4.1.2 20070925
> > >>> (Red Hat 4.1.2-33)) #1928 SMP PREEMPT Thu Jan 24 17:09:04 PST 2008
> > >>> early_ioremap_init()
> > >>> BIOS-provided physical RAM map:
> > >>> BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> > >>> BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> > >>> BIOS-e820: 00000000000e8000 - 0000000000100000 (reserved)
> > >>> BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
> > >>> BIOS-e820: 000000001fff0000 - 0000000020000000 (ACPI data)
> > >>> BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
> > >>> console [earlyser0] enabled
> > >>> 0MB HIGHMEM available.
> > >>> 511MB LOWMEM available.
> > >>> Scan SMP from c0000000 for 1024 bytes.
> > >>> Scan SMP from c009fc00 for 1024 bytes.
> > >>> Scan SMP from c00f0000 for 65536 bytes.
> > >>> Scan SMP from c009fc00 for 1024 bytes.
> > >>> ***************
> > >>> **** WARNING: likely BIOS bug
> > >>> **** MTRRs don't cover all of memory, trimmed 131056 pages
> > >>> ***************
> > >>>
> > >> H. Peter Anvin wrote:
> > >> Looks like the code doesn't check that the CPU *has* MTRRs...
> > >>
> > >
> > > so check it mtrr is there, also check if mem less 4G and is AMD as early
> > >
> >
> > Thanks, this gets me to usermode under kvm.
>
> i think the is 0xffffffff to 0xffffffffUL make the difference.
>
> the cpu mode in KVM/qemu may cpu_has_mtrr, but doesn't really have
> return mtrr msr with correct value.
>
> is_cpu(INTEL) already make sure we have mtrr_if got assigned and cpu_has_mtrr.
>
> may need to fix qemu instead...
>
> Can you try post /proc/mtrr for your guest in kvm/qemu?

jeremy,

can you try v3 patch?

Thanks

YH

2008-01-25 10:52:45

by Andi Kleen

[permalink] [raw]

Subject: Re: x86.git: mtrr trimming removes all memory under kvm

Yinghai Lu <[email protected]> writes:
>
> + /* Doesn't apply to memory < 4GB */
> + if (end_pfn <= (0xffffffffUL >> PAGE_SHIFT))
> + return 0;

I don't think this makes sense to move outside the AMD workaround
because the normal MTRRs don't have an implicit hidden 4GB semantics.

-Andi

2008-01-25 11:09:59

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Yinghai Lu <[email protected]> wrote:

> [PATCH] x86: trim ram need to check if mtrr is there v3

> > H. Peter Anvin wrote:
> > Looks like the code doesn't check that the CPU *has* MTRRs...
>
> so more strict check if mtrr is there really.
> bail out if mtrr all blank when qemu cpu model is used
>
> and check if is AMD as early
> also remove 4G less check, according to hpa.

thanks, applied. Shouldnt we put in an exception for when there is MTRR
support, but they dont cover anything. Still emit a warning - but
booting up real slow is still better than losing all of RAM and crashing
...

i also updated the messages, they now go like this:

WARNING: strange, CPU MTRRs all blank?

and:

WARNING: BIOS bug: CPU MTRRs don't cover all of memory, losing 45MB of RAM.

Ingo

2008-01-25 11:12:40

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

* Yinghai Lu <[email protected]> wrote:

> is_cpu(INTEL) already make sure we have mtrr_if got assigned and
> cpu_has_mtrr.
>
> may need to fix qemu instead...

no ...

lets not do non-sensical trimming of RAM, ok? Emit a warning but never
trim all of RAM and make the system unbootable. Trimmed RAM is something
that users can pester board/BIOS vendors with. Non-booting kernels is
something _we_ get pestered with ;-)

Ingo

2008-01-25 14:22:19

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Yinghai Lu <[email protected]> wrote:

> -static __init int amd_special_default_mtrr(unsigned long end_pfn)
> +static __init int amd_special_default_mtrr(void)
> {
> u32 l, h;
>
> - /* Doesn't apply to memory < 4GB */
> - if (end_pfn <= (0xffffffff >> PAGE_SHIFT))
> - return 0;
> if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> return 0;

The logic we ideally would like to have is something like this:

find _any_ RAM that is not mapped via any MTRRs (be that special MTRR
extensions like Tom2) and clear that from the e820 maps.

not just 'end of RAM'.

And in that sense the amd_special_default_mtrr() check is wrong, because
it just checks that Tom2 is set and then does no other checking. And the
original MTRR check is wrong too because it just finds the highest
cacheable MTRR-covered address and compares that to the kernel-known end
of RAM.

what we should probably do instead is to have a filter function:

new_end = trim_range_to_mtrr_cached(start, end);

and then we could iterate through every e820 map entry that is marked as
usable RAM, and send it through this filter. If the filter returns the
same value that got passed in, we keep the e820 entry unchanged. If the
filter returns a new "end" value, we use that in the e820 map.

that way, the current Tom2 hack is just a natural extension to the
filter function: it would (on AMD CPUs) recognize (within
trim_range_to_mtrr_cached filter) that all memory addresses above 4GB
are marked as cacheable via Tom2.

Or something like this. Hm?

Ingo

2008-01-25 14:57:46

by Andi Kleen

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar <[email protected]> writes:
>
> what we should probably do instead is to have a filter function:
>
> new_end = trim_range_to_mtrr_cached(start, end);
>
> and then we could iterate through every e820 map entry that is marked as
> usable RAM, and send it through this filter. If the filter returns the
> same value that got passed in, we keep the e820 entry unchanged. If the
> filter returns a new "end" value, we use that in the e820 map.

To be fully generic you would need to allow it to adjust start too.

> that way, the current Tom2 hack is just a natural extension to the
> filter function: it would (on AMD CPUs) recognize (within
> trim_range_to_mtrr_cached filter) that all memory addresses above 4GB
> are marked as cacheable via Tom2.
>
> Or something like this. Hm?

I agree that would be the correct way to do it.

Later on with PAT that filter could also do PAT related checks
and something like this will likely be needed anyways.

-Andi

2008-01-25 15:10:40

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Andi Kleen <[email protected]> wrote:

> Ingo Molnar <[email protected]> writes:
> >
> > what we should probably do instead is to have a filter function:
> >
> > new_end = trim_range_to_mtrr_cached(start, end);
> >
> > and then we could iterate through every e820 map entry that is
> > marked as usable RAM, and send it through this filter. If the filter
> > returns the same value that got passed in, we keep the e820 entry
> > unchanged. If the filter returns a new "end" value, we use that in
> > the e820 map.
>
> To be fully generic you would need to allow it to adjust start too.

no, to be fully generic it would have to be able to 'split' e820 entries
up and punch holes into them - but we dont want to go that far i think.
The most common problem is mismatch at the end of a range.

but what matters more is to have full, generic _detection_ of the
problem - and that's what we dont do right now. (and that's what my
reply outlines)

The _fixup_ which we base on this information can then be anything from
"trivially trim the end" up to a complex "punch holes" solution or the
simplest "print nasty warning message and do nothing else" solution.

> > that way, the current Tom2 hack is just a natural extension to the
> > filter function: it would (on AMD CPUs) recognize (within
> > trim_range_to_mtrr_cached filter) that all memory addresses above
> > 4GB are marked as cacheable via Tom2.
> >
> > Or something like this. Hm?
>
> I agree that would be the correct way to do it.
>
> Later on with PAT that filter could also do PAT related checks and
> something like this will likely be needed anyways.

a "what is the effective MTRR caching attribute of this physical
address" type of function would benefit PAT too, yes.

Ingo

2008-01-25 15:51:36

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v2

Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> is_cpu(INTEL) already make sure we have mtrr_if got assigned and
>> cpu_has_mtrr.
>>
>> may need to fix qemu instead...
>
> no ...
>
> lets not do non-sensical trimming of RAM, ok? Emit a warning but never
> trim all of RAM and make the system unbootable. Trimmed RAM is something
> that users can pester board/BIOS vendors with. Non-booting kernels is
> something _we_ get pestered with ;-)
>

*And* let's push a fix to Qemu/KVM as appropriate.

Last I checked Qemu never even turns caching on in %cr0, never mind gets
the MTRRs right. If it's advertising MTRRs, this is a problem.

-hpa

2008-01-25 16:00:49

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar wrote:
>
> no, to be fully generic it would have to be able to 'split' e820 entries
> up and punch holes into them - but we dont want to go that far i think.
> The most common problem is mismatch at the end of a range.
>

For what it's worth, I have a set of code to do this, written in order
to canonicalize and modify e820 data structures for memdisk.

The key to it is the observation that the e820-delivered (address, len,
type) tuples isn't actually the data structure you want -- what you want
is an ordered list of (address, type) tuples, where type may includes
undefined (the e820 default type - i.e. unused, available address space).

In addition to the attached code, to do this right, we probably want a
function to do filtering (only clobber entries of a specfic type, in
this case type 1) as opposed to blind clobber; with an ordered array
this is quite trivial.

-hpa

Attachments:

e820.h (0.98 kB)
e820func.c (2.44 kB)
Download all attachments

2008-01-25 18:55:41

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>
>> [PATCH] x86: trim ram need to check if mtrr is there v3
>>
>
>
>>> H. Peter Anvin wrote:
>>> Looks like the code doesn't check that the CPU *has* MTRRs...
>>>
>> so more strict check if mtrr is there really.
>> bail out if mtrr all blank when qemu cpu model is used
>>
>> and check if is AMD as early
>> also remove 4G less check, according to hpa.
>>
>
> thanks, applied. Shouldnt we put in an exception for when there is MTRR
> support, but they dont cover anything. Still emit a warning - but
> booting up real slow is still better than losing all of RAM and crashing
>

The problem is re-occuring for me with current x86.git. Looks like v2
did the trick, and v3 is broken...

J

2008-01-25 18:59:21

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>
>> [PATCH] x86: trim ram need to check if mtrr is there v3
>>
>
>
>>> H. Peter Anvin wrote:
>>> Looks like the code doesn't check that the CPU *has* MTRRs...
>>>
>> so more strict check if mtrr is there really.
>> bail out if mtrr all blank when qemu cpu model is used
>>
>> and check if is AMD as early
>> also remove 4G less check, according to hpa.
>>
>
> thanks, applied. Shouldnt we put in an exception for when there is MTRR
> support, but they dont cover anything. Still emit a warning - but
> booting up real slow is still better than losing all of RAM and crashing
> ...
>
> i also updated the messages, they now go like this:
>
> WARNING: strange, CPU MTRRs all blank?
>
> and:
>
> WARNING: BIOS bug: CPU MTRRs don't cover all of memory, losing 45MB of RAM.
>

Though I don't see this form of message; have you pushed your changes
out to the public x86.git#mm tree?

J

2008-01-25 19:00:24

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Jeremy Fitzhardinge <[email protected]> wrote:

>> thanks, applied. Shouldnt we put in an exception for when there is
>> MTRR support, but they dont cover anything. Still emit a warning -
>> but booting up real slow is still better than losing all of RAM and
>> crashing
>
> The problem is re-occuring for me with current x86.git. Looks like v2
> did the trick, and v3 is broken...

which git head is that? I'm pushing out the queue with v3 included this
very minute, so i doubt you can have tested that already! :-)

Ingo

2008-01-25 19:02:13

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar wrote:
> * Jeremy Fitzhardinge <[email protected]> wrote:
>
>
>>> thanks, applied. Shouldnt we put in an exception for when there is
>>> MTRR support, but they dont cover anything. Still emit a warning -
>>> but booting up real slow is still better than losing all of RAM and
>>> crashing
>>>
>> The problem is re-occuring for me with current x86.git. Looks like v2
>> did the trick, and v3 is broken...
>>
>
> which git head is that? I'm pushing out the queue with v3 included this
> very minute, so i doubt you can have tested that already! :-)
>

I was referring to:

commit 6a4544a9c8b54b82893044cb53695502cc386f00
Author: Yinghai Lu <[email protected]>
Date: Fri Jan 25 00:15:29 2008 +0100

x86_32: trim memory by updating e820 v3

but you've answered the question...

J

2008-01-25 19:02:47

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

On Friday 25 January 2008 10:55:31 am Jeremy Fitzhardinge wrote:
> Ingo Molnar wrote:
> > * Yinghai Lu <[email protected]> wrote:
> >
> >
> >> [PATCH] x86: trim ram need to check if mtrr is there v3
> >>
> >
> >
> >>> H. Peter Anvin wrote:
> >>> Looks like the code doesn't check that the CPU *has* MTRRs...
> >>>
> >> so more strict check if mtrr is there really.
> >> bail out if mtrr all blank when qemu cpu model is used
> >>
> >> and check if is AMD as early
> >> also remove 4G less check, according to hpa.
> >>
> >
> > thanks, applied. Shouldnt we put in an exception for when there is MTRR
> > support, but they dont cover anything. Still emit a warning - but
> > booting up real slow is still better than losing all of RAM and crashing
> >
>
> The problem is re-occuring for me with current x86.git. Looks like v2
> did the trick, and v3 is broken...

so the ram size less 4g is way out for your case.

again, can you post /proc/mtrrs with v2 patch?

Thanks

YH

2008-01-25 19:03:23

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Jeremy Fitzhardinge <[email protected]> wrote:

>> and:
>>
>> WARNING: BIOS bug: CPU MTRRs don't cover all of memory, losing 45MB
>> of RAM.
>
> Though I don't see this form of message; have you pushed your changes
> out to the public x86.git#mm tree?

it's available ... 3 .. 2 ... 1 ... NOW! ;-)

Ingo

2008-01-25 19:04:33

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Yinghai Lu wrote:
> On Friday 25 January 2008 10:55:31 am Jeremy Fitzhardinge wrote:
>
>> Ingo Molnar wrote:
>>
>>> * Yinghai Lu <[email protected]> wrote:
>>>
>>>
>>>
>>>> [PATCH] x86: trim ram need to check if mtrr is there v3
>>>>
>>>>
>>>
>>>
>>>>> H. Peter Anvin wrote:
>>>>> Looks like the code doesn't check that the CPU *has* MTRRs...
>>>>>
>>>>>
>>>> so more strict check if mtrr is there really.
>>>> bail out if mtrr all blank when qemu cpu model is used
>>>>
>>>> and check if is AMD as early
>>>> also remove 4G less check, according to hpa.
>>>>
>>>>
>>> thanks, applied. Shouldnt we put in an exception for when there is MTRR
>>> support, but they dont cover anything. Still emit a warning - but
>>> booting up real slow is still better than losing all of RAM and crashing
>>>
>>>
>> The problem is re-occuring for me with current x86.git. Looks like v2
>> did the trick, and v3 is broken...
>>
>
> so the ram size less 4g is way out for your case.
>
> again, can you post /proc/mtrrs with v2 patch?
>

It was empty. But ignore this report; it wasn't against the right
version of x86.git.

J

2008-01-25 19:04:47

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Jeremy Fitzhardinge <[email protected]> wrote:

> I was referring to:
>
> commit 6a4544a9c8b54b82893044cb53695502cc386f00
> Author: Yinghai Lu <[email protected]>
> Date: Fri Jan 25 00:15:29 2008 +0100
>
> x86_32: trim memory by updating e820 v3
>
> but you've answered the question...

yeah, i guess what i've pushed out just now is v3++. Does it work for
you?

Ingo

2008-01-25 19:27:41

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

Ingo Molnar wrote:
> * Jeremy Fitzhardinge <[email protected]> wrote:
>
>
>> I was referring to:
>>
>> commit 6a4544a9c8b54b82893044cb53695502cc386f00
>> Author: Yinghai Lu <[email protected]>
>> Date: Fri Jan 25 00:15:29 2008 +0100
>>
>> x86_32: trim memory by updating e820 v3
>>
>> but you've answered the question...
>>
>
> yeah, i guess what i've pushed out just now is v3++. Does it work for
> you?
>

Yes, it does. And for the record:

[root@qemu ~]# cat /proc/mtrr
[root@qemu ~]#

J

2008-01-25 19:30:29

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

* Jeremy Fitzhardinge <[email protected]> wrote:

>> yeah, i guess what i've pushed out just now is v3++. Does it work for
>> you?
>
> Yes, it does. And for the record:
>
> [root@qemu ~]# cat /proc/mtrr [root@qemu ~]#
> J

great, thanks for checking.

Ingo

2008-01-25 19:32:22

[permalink] [raw]

Subject: Re: [PATCH] x86: trim ram need to check if mtrr is there v3

On Jan 25, 2008 11:27 AM, Jeremy Fitzhardinge <[email protected]> wrote:
> Ingo Molnar wrote:
> > * Jeremy Fitzhardinge <[email protected]> wrote:
> >
> >
> >> I was referring to:
> >>
> >> commit 6a4544a9c8b54b82893044cb53695502cc386f00
> >> Author: Yinghai Lu <[email protected]>
> >> Date: Fri Jan 25 00:15:29 2008 +0100
> >>
> >> x86_32: trim memory by updating e820 v3
> >>
> >> but you've answered the question...
> >>
> >
> > yeah, i guess what i've pushed out just now is v3++. Does it work for
> > you?
> >
>
> Yes, it does. And for the record:
>
> [root@qemu ~]# cat /proc/mtrr
> [root@qemu ~]#

so the boot msg say:
WARNING: strange, CPU MTRRs all blank?

YH

2008-01-25 19:39:43