From: Cliff Wickman <[email protected]>
Create a new e820 memory type (E820_VENDOR_RESERVED) for areas
of memory reserved by the BIOS in the EFI table.
(An example use of this functionality is the UV system, which
will access extremely large areas of memory with a memory engine
that allows a user to address beyond the processor's range. Such
areas are reserved in the EFI table by the BIOS.)
Without this patch the EFI_RESERVED_TYPE memory reservations will be
marked usable in the e820 table. There will be a collision between
kernel use and reserver's use of this memory.
This patch causes the EFI_RESERVED_TYPE memory reservations to be recorded
in the e820 table as new type E820_VENDOR_RESERVED.
This patch makes sanitize_e820_map() preserve E820_VENDOR_RESERVED types
as separate entries.
[The elilo loader may combine regions of like type as it builds the e820
table in boot_params (regular RAM and vendor reserved areas are combined).
But this patch makes do_add_efi_memmap() separate the RESERVED regions
into separate e820 entries.
Some loaders have a restricted number of entries possible in the e820 table,
hence the need to record the reservations in the unrestricted EFI table.]
The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
on the kernel command line.
Diffed against 2.6.30-rc8
Signed-off-by: Cliff Wickman <[email protected]>
---
arch/x86/include/asm/e820.h | 1 +
arch/x86/kernel/e820.c | 7 ++++++-
arch/x86/kernel/efi.c | 4 +++-
3 files changed, 10 insertions(+), 2 deletions(-)
Index: linux/arch/x86/kernel/e820.c
===================================================================
--- linux.orig/arch/x86/kernel/e820.c
+++ linux/arch/x86/kernel/e820.c
@@ -141,6 +141,9 @@ static void __init e820_print_type(u32 t
case E820_RESERVED:
printk(KERN_CONT "(reserved)");
break;
+ case E820_VENDOR_RESERVED:
+ printk(KERN_CONT "(vendor reserved)");
+ break;
case E820_ACPI:
printk(KERN_CONT "(ACPI data)");
break;
@@ -355,8 +358,10 @@ int __init sanitize_e820_map(struct e820
/*
* continue building up new bios map based on this
* information
+ * (do not consolidate the E820_VENDOR_RESERVED ranges)
*/
- if (current_type != last_type) {
+ if ((current_type != last_type) ||
+ (current_type == E820_VENDOR_RESERVED)) {
if (last_type != 0) {
new_bios[new_bios_entry].size =
change_point[chgidx]->addr - last_addr;
Index: linux/arch/x86/kernel/efi.c
===================================================================
--- linux.orig/arch/x86/kernel/efi.c
+++ linux/arch/x86/kernel/efi.c
@@ -240,7 +240,9 @@ static void __init do_add_efi_memmap(voi
unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
int e820_type;
- if (md->attribute & EFI_MEMORY_WB)
+ if (md->type == EFI_RESERVED_TYPE)
+ e820_type = E820_VENDOR_RESERVED;
+ else if (md->attribute & EFI_MEMORY_WB)
e820_type = E820_RAM;
else
e820_type = E820_RESERVED;
Index: linux/arch/x86/include/asm/e820.h
===================================================================
--- linux.orig/arch/x86/include/asm/e820.h
+++ linux/arch/x86/include/asm/e820.h
@@ -44,6 +44,7 @@
#define E820_ACPI 3
#define E820_NVS 4
#define E820_UNUSABLE 5
+#define E820_VENDOR_RESERVED 6
/* reserved RAM used by kernel itself */
#define E820_RESERVED_KERN 128
Cliff Wickman wrote:
> From: Cliff Wickman <[email protected]>
>
> Create a new e820 memory type (E820_VENDOR_RESERVED) for areas
> of memory reserved by the BIOS in the EFI table.
>
> (An example use of this functionality is the UV system, which
> will access extremely large areas of memory with a memory engine
> that allows a user to address beyond the processor's range. Such
> areas are reserved in the EFI table by the BIOS.)
>
There is no difference between that and E820_RESERVED, so there is no
reason to distinguish them. The semantics are exactly the same.
> Without this patch the EFI_RESERVED_TYPE memory reservations will be
> marked usable in the e820 table. There will be a collision between
> kernel use and reserver's use of this memory.
>
> This patch causes the EFI_RESERVED_TYPE memory reservations to be recorded
> in the e820 table as new type E820_VENDOR_RESERVED.
> This patch makes sanitize_e820_map() preserve E820_VENDOR_RESERVED types
> as separate entries.
>
> [The elilo loader may combine regions of like type as it builds the e820
> table in boot_params (regular RAM and vendor reserved areas are combined).
> But this patch makes do_add_efi_memmap() separate the RESERVED regions
> into separate e820 entries.
> Some loaders have a restricted number of entries possible in the e820 table,
> hence the need to record the reservations in the unrestricted EFI table.]
> The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
> on the kernel command line.
This patch fixes a real problem in a wrong way.
The real problem is that this condition is too lenient:
if (md->attribute & EFI_MEMORY_WB)
e820_type = E820_RAM;
else
e820_type = E820_RESERVED;
It really should be something like:
switch (md->type) {
case EFI_LOADER_CODE:
case EFI_LOADER_DATA:
case EFI_BOOT_SERVICES_CODE:
case EFI_BOOT_SERVICES_DATA:
case EFI_CONVENTIONAL_MEMORY:
if (md->attribute & EFI_MEMORY_WB)
e820_type = E820_RAM;
else
e820_type = E820_RESERVED;
break;
case EFI_ACPI_RECLAIM_MEMORY:
e820_type = E820_ACPI;
break;
case EFI_ACPI_MEMORY_NVS:
e820_type = E820_NVS;
break;
case EFI_UNUSABLE_MEMORY:
e820_type = E820_UNUSUABLE;
break;
default:
e820_type = E820_RESERVED;
break;
}
Personally, it's not clear to me if this should do add any non-memory
ranges, as the boot loader should have done that, but I guess in this
particular case we have already horked out.
Another problem is that the comment is wrong. sanitize_e820_map() will
coalesce adjacent entries, as it should.
Finally, randomly definiting a standard value in E820 with new semantics
isn't going to fly; it's likely to conflict with official allocations.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Thu, Jun 11, 2009 at 08:34:23AM -0700, H. Peter Anvin wrote:
> Cliff Wickman wrote:
> > From: Cliff Wickman <[email protected]>
> >
> > Create a new e820 memory type (E820_VENDOR_RESERVED) for areas
> > of memory reserved by the BIOS in the EFI table.
> >
> > (An example use of this functionality is the UV system, which
> > will access extremely large areas of memory with a memory engine
> > that allows a user to address beyond the processor's range. Such
> > areas are reserved in the EFI table by the BIOS.)
> >
>
> There is no difference between that and E820_RESERVED, so there is no
> reason to distinguish them. The semantics are exactly the same.
I thought a new type would be clearer, but if it would break an e820
standard I withdraw the idea. All is good as long as the memory gets reserved.
>
> > Without this patch the EFI_RESERVED_TYPE memory reservations will be
> > marked usable in the e820 table. There will be a collision between
> > kernel use and reserver's use of this memory.
> >
> > This patch causes the EFI_RESERVED_TYPE memory reservations to be recorded
> > in the e820 table as new type E820_VENDOR_RESERVED.
> > This patch makes sanitize_e820_map() preserve E820_VENDOR_RESERVED types
> > as separate entries.
> >
> > [The elilo loader may combine regions of like type as it builds the e820
> > table in boot_params (regular RAM and vendor reserved areas are combined).
> > But this patch makes do_add_efi_memmap() separate the RESERVED regions
> > into separate e820 entries.
> > Some loaders have a restricted number of entries possible in the e820 table,
> > hence the need to record the reservations in the unrestricted EFI table.]
> > The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
> > on the kernel command line.
>
> This patch fixes a real problem in a wrong way.
>
> The real problem is that this condition is too lenient:
>
> if (md->attribute & EFI_MEMORY_WB)
> e820_type = E820_RAM;
> else
> e820_type = E820_RESERVED;
>
> It really should be something like:
>
> switch (md->type) {
> case EFI_LOADER_CODE:
> case EFI_LOADER_DATA:
> case EFI_BOOT_SERVICES_CODE:
> case EFI_BOOT_SERVICES_DATA:
> case EFI_CONVENTIONAL_MEMORY:
> if (md->attribute & EFI_MEMORY_WB)
> e820_type = E820_RAM;
> else
> e820_type = E820_RESERVED;
> break;
> case EFI_ACPI_RECLAIM_MEMORY:
> e820_type = E820_ACPI;
> break;
> case EFI_ACPI_MEMORY_NVS:
> e820_type = E820_NVS;
> break;
> case EFI_UNUSABLE_MEMORY:
> e820_type = E820_UNUSUABLE;
> break;
> default:
> e820_type = E820_RESERVED;
> break;
> }
Okay. I buy that as more straightforward.
> Personally, it's not clear to me if this should do add any non-memory
> ranges, as the boot loader should have done that, but I guess in this
> particular case we have already horked out.
>
> Another problem is that the comment is wrong. sanitize_e820_map() will
> coalesce adjacent entries, as it should.
>
> Finally, randomly definiting a standard value in E820 with new semantics
> isn't going to fly; it's likely to conflict with official allocations.
>
> -hpa
I propose to submit your code (basically) in the form of the below patch.
It works for me. Does it look okay to you?
Subject: [PATCH] x86: efi/e820 table merge fix
This patch causes all the EFI_RESERVED_TYPE memory reservations to be recorded
in the e820 table as type E820_RESERVED.
Without this patch EFI_RESERVED_TYPE memory reservations may be
marked usable in the e820 table. There may be a collision between
kernel use and some reserver's use of this memory.
(An example use of this functionality is the UV system, which
will access extremely large areas of memory with a memory engine
that allows a user to address beyond the processor's range. Such
areas are reserved in the EFI table by the BIOS.
Some loaders have a restricted number of entries possible in the e820 table,
hence the need to record the reservations in the unrestricted EFI table.)
The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
on the kernel command line.
Diffed against 2.6.30-rc8
Signed-off-by: Cliff Wickman <[email protected]>
---
arch/x86/kernel/efi.c | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
Index: linux/arch/x86/kernel/efi.c
===================================================================
--- linux.orig/arch/x86/kernel/efi.c
+++ linux/arch/x86/kernel/efi.c
@@ -240,10 +240,35 @@ static void __init do_add_efi_memmap(voi
unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
int e820_type;
- if (md->attribute & EFI_MEMORY_WB)
- e820_type = E820_RAM;
- else
+ switch (md->type) {
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_CONVENTIONAL_MEMORY:
+ if (md->attribute & EFI_MEMORY_WB)
+ e820_type = E820_RAM;
+ else
+ e820_type = E820_RESERVED;
+ break;
+ case EFI_ACPI_RECLAIM_MEMORY:
+ e820_type = E820_ACPI;
+ break;
+ case EFI_ACPI_MEMORY_NVS:
+ e820_type = E820_NVS;
+ break;
+ case EFI_UNUSABLE_MEMORY:
+ e820_type = E820_UNUSABLE;
+ break;
+ default:
+ /*
+ * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
+ * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
+ * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
+ */
e820_type = E820_RESERVED;
+ break;
+ }
e820_add_region(start, size, e820_type);
}
sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
--
Cliff Wickman
SGI
[email protected]
(651) 683-3824
Yinghai, Huang, Paul: looks good to you [see patch at end]? Anyone else
we should have look at this?
-hpa
Cliff Wickman wrote:
>> There is no difference between that and E820_RESERVED, so there is no
>> reason to distinguish them. The semantics are exactly the same.
>
> I thought a new type would be clearer, but if it would break an e820
> standard I withdraw the idea. All is good as long as the memory gets reserved.
We *could* add private types with negative numbers if we had to, but
that means adding some infrastructure, and this doesn't seem justified
for this case. There is also a cost involved, since different types
can't be range-merged.
>>
>> The real problem is that this condition is too lenient:
>>
>> if (md->attribute & EFI_MEMORY_WB)
>> e820_type = E820_RAM;
>> else
>> e820_type = E820_RESERVED;
>>
>> It really should be something like:
>>
>> switch (md->type) {
>> case EFI_LOADER_CODE:
>> case EFI_LOADER_DATA:
>> case EFI_BOOT_SERVICES_CODE:
>> case EFI_BOOT_SERVICES_DATA:
>> case EFI_CONVENTIONAL_MEMORY:
>> if (md->attribute & EFI_MEMORY_WB)
>> e820_type = E820_RAM;
>> else
>> e820_type = E820_RESERVED;
>> break;
>> case EFI_ACPI_RECLAIM_MEMORY:
>> e820_type = E820_ACPI;
>> break;
>> case EFI_ACPI_MEMORY_NVS:
>> e820_type = E820_NVS;
>> break;
>> case EFI_UNUSABLE_MEMORY:
>> e820_type = E820_UNUSUABLE;
>> break;
>> default:
>> e820_type = E820_RESERVED;
>> break;
>> }
>
> Okay. I buy that as more straightforward.
>
>> Personally, it's not clear to me if this should do add any non-memory
>> ranges, as the boot loader should have done that, but I guess in this
>> particular case we have already horked out.
>>
>> Another problem is that the comment is wrong. sanitize_e820_map() will
>> coalesce adjacent entries, as it should.
>>
>> Finally, randomly definiting a standard value in E820 with new semantics
>> isn't going to fly; it's likely to conflict with official allocations.
>>
>> -hpa
>
> I propose to submit your code (basically) in the form of the below patch.
> It works for me. Does it look okay to you?
>
>
>
> Subject: [PATCH] x86: efi/e820 table merge fix
>
> This patch causes all the EFI_RESERVED_TYPE memory reservations to be recorded
> in the e820 table as type E820_RESERVED.
>
> Without this patch EFI_RESERVED_TYPE memory reservations may be
> marked usable in the e820 table. There may be a collision between
> kernel use and some reserver's use of this memory.
>
> (An example use of this functionality is the UV system, which
> will access extremely large areas of memory with a memory engine
> that allows a user to address beyond the processor's range. Such
> areas are reserved in the EFI table by the BIOS.
> Some loaders have a restricted number of entries possible in the e820 table,
> hence the need to record the reservations in the unrestricted EFI table.)
>
> The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
> on the kernel command line.
>
> Diffed against 2.6.30-rc8
>
> Signed-off-by: Cliff Wickman <[email protected]>
> ---
> arch/x86/kernel/efi.c | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> Index: linux/arch/x86/kernel/efi.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/efi.c
> +++ linux/arch/x86/kernel/efi.c
> @@ -240,10 +240,35 @@ static void __init do_add_efi_memmap(voi
> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
> int e820_type;
>
> - if (md->attribute & EFI_MEMORY_WB)
> - e820_type = E820_RAM;
> - else
> + switch (md->type) {
> + case EFI_LOADER_CODE:
> + case EFI_LOADER_DATA:
> + case EFI_BOOT_SERVICES_CODE:
> + case EFI_BOOT_SERVICES_DATA:
> + case EFI_CONVENTIONAL_MEMORY:
> + if (md->attribute & EFI_MEMORY_WB)
> + e820_type = E820_RAM;
> + else
> + e820_type = E820_RESERVED;
> + break;
> + case EFI_ACPI_RECLAIM_MEMORY:
> + e820_type = E820_ACPI;
> + break;
> + case EFI_ACPI_MEMORY_NVS:
> + e820_type = E820_NVS;
> + break;
> + case EFI_UNUSABLE_MEMORY:
> + e820_type = E820_UNUSABLE;
> + break;
> + default:
> + /*
> + * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
> + * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
> + * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
> + */
> e820_type = E820_RESERVED;
> + break;
> + }
> e820_add_region(start, size, e820_type);
> }
> sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
>
H. Peter Anvin wrote:
> Yinghai, Huang, Paul: looks good to you [see patch at end]? Anyone else
> we should have look at this?
>
> -hpa
>
>
> Cliff Wickman wrote:
>>> There is no difference between that and E820_RESERVED, so there is no
>>> reason to distinguish them. The semantics are exactly the same.
>> I thought a new type would be clearer, but if it would break an e820
>> standard I withdraw the idea. All is good as long as the memory gets reserved.
>
> We *could* add private types with negative numbers if we had to, but
> that means adding some infrastructure, and this doesn't seem justified
> for this case. There is also a cost involved, since different types
> can't be range-merged.
>
>>> The real problem is that this condition is too lenient:
>>>
>>> if (md->attribute & EFI_MEMORY_WB)
>>> e820_type = E820_RAM;
>>> else
>>> e820_type = E820_RESERVED;
>>>
>>> It really should be something like:
>>>
>>> switch (md->type) {
>>> case EFI_LOADER_CODE:
>>> case EFI_LOADER_DATA:
>>> case EFI_BOOT_SERVICES_CODE:
>>> case EFI_BOOT_SERVICES_DATA:
>>> case EFI_CONVENTIONAL_MEMORY:
>>> if (md->attribute & EFI_MEMORY_WB)
>>> e820_type = E820_RAM;
>>> else
>>> e820_type = E820_RESERVED;
>>> break;
>>> case EFI_ACPI_RECLAIM_MEMORY:
>>> e820_type = E820_ACPI;
>>> break;
>>> case EFI_ACPI_MEMORY_NVS:
>>> e820_type = E820_NVS;
>>> break;
>>> case EFI_UNUSABLE_MEMORY:
>>> e820_type = E820_UNUSUABLE;
>>> break;
>>> default:
>>> e820_type = E820_RESERVED;
>>> break;
>>> }
>> Okay. I buy that as more straightforward.
>>
>>> Personally, it's not clear to me if this should do add any non-memory
>>> ranges, as the boot loader should have done that, but I guess in this
>>> particular case we have already horked out.
>>>
>>> Another problem is that the comment is wrong. sanitize_e820_map() will
>>> coalesce adjacent entries, as it should.
>>>
>>> Finally, randomly definiting a standard value in E820 with new semantics
>>> isn't going to fly; it's likely to conflict with official allocations.
>>>
>>> -hpa
>> I propose to submit your code (basically) in the form of the below patch.
>> It works for me. Does it look okay to you?
>>
>>
>>
>> Subject: [PATCH] x86: efi/e820 table merge fix
>>
>> This patch causes all the EFI_RESERVED_TYPE memory reservations to be recorded
>> in the e820 table as type E820_RESERVED.
>>
>> Without this patch EFI_RESERVED_TYPE memory reservations may be
>> marked usable in the e820 table. There may be a collision between
>> kernel use and some reserver's use of this memory.
>>
>> (An example use of this functionality is the UV system, which
>> will access extremely large areas of memory with a memory engine
>> that allows a user to address beyond the processor's range. Such
>> areas are reserved in the EFI table by the BIOS.
>> Some loaders have a restricted number of entries possible in the e820 table,
>> hence the need to record the reservations in the unrestricted EFI table.)
>>
>> The call to do_add_efi_memmap() is only made if "add_efi_memmap" is specified
>> on the kernel command line.
>>
>> Diffed against 2.6.30-rc8
>>
>> Signed-off-by: Cliff Wickman <[email protected]>
>> ---
>> arch/x86/kernel/efi.c | 31 ++++++++++++++++++++++++++++---
>> 1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> Index: linux/arch/x86/kernel/efi.c
>> ===================================================================
>> --- linux.orig/arch/x86/kernel/efi.c
>> +++ linux/arch/x86/kernel/efi.c
>> @@ -240,10 +240,35 @@ static void __init do_add_efi_memmap(voi
>> unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
>> int e820_type;
>>
>> - if (md->attribute & EFI_MEMORY_WB)
>> - e820_type = E820_RAM;
>> - else
>> + switch (md->type) {
>> + case EFI_LOADER_CODE:
>> + case EFI_LOADER_DATA:
>> + case EFI_BOOT_SERVICES_CODE:
>> + case EFI_BOOT_SERVICES_DATA:
>> + case EFI_CONVENTIONAL_MEMORY:
>> + if (md->attribute & EFI_MEMORY_WB)
>> + e820_type = E820_RAM;
>> + else
>> + e820_type = E820_RESERVED;
>> + break;
>> + case EFI_ACPI_RECLAIM_MEMORY:
>> + e820_type = E820_ACPI;
>> + break;
>> + case EFI_ACPI_MEMORY_NVS:
>> + e820_type = E820_NVS;
>> + break;
>> + case EFI_UNUSABLE_MEMORY:
>> + e820_type = E820_UNUSABLE;
>> + break;
>> + default:
>> + /*
>> + * EFI_RESERVED_TYPE EFI_RUNTIME_SERVICES_CODE
>> + * EFI_RUNTIME_SERVICES_DATA EFI_MEMORY_MAPPED_IO
>> + * EFI_MEMORY_MAPPED_IO_PORT_SPACE EFI_PAL_CODE
>> + */
>> e820_type = E820_RESERVED;
then, if those entries are near TOML, and it is E820_RESERVED now, and it could not be directly mapped at first point.
but later efi_remap will direct map it if the size is too big for runtime service.
not sure others.
YH
>> + break;
>> + }
>> e820_add_region(start, size, e820_type);
>> }
>> sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
>>
Yinghai Lu wrote:
>
> then, if those entries are near TOML, and it is E820_RESERVED now, and it could not be directly mapped at first point.
> but later efi_remap will direct map it if the size is too big for runtime service.
> not sure others.
>
I'm not sure I understand the comment?
-hpa