Messages in this thread | | | Date | Fri, 12 Jun 2009 11:17:49 -0700 | From | Yinghai Lu <> | Subject | Re: [PATCH] x86: vendor reserved memory type |
| |
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 <cpw@sgi.com> >> --- >> 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); >>
| |