Re: [PATCH] x86: vendor reserved memory type

["H. Peter Anvin" <hpa@zytor.com>]

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;
> +			break;
> +		}
>  		e820_add_region(start, size, e820_type);
>  	}
>  	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
>