Re: [PATCH] x86: efi/e820 table merge fix

[Cliff Wickman <cpw@sgi.com>]

On Wed, Jun 17, 2009 at 01:08:22PM +0800, Huang Ying wrote:
> On Wed, 2009-06-17 at 12:03 +0800, H. Peter Anvin wrote:
> > Huang Ying wrote:
> > >>> Why does BIOS mark memory region without EFI_MEMORY_WB as these types?
> > >>> Any example?
> > >>>
> > >> Probably not, but if it does, it's broken, and the memory should be
> > >> ignored. The original code had the EFI_MEMORY_WB check already, so it
> > >> seems prudent to keep it.
> > > 
> > > Maybe we need a real life example for that "fix". And attribute that to
> > > the vendor in comments.
> > > 
> > > Best Regards,
> > > Huang Ying
> > 
> > I think you're reading the patch backwards.
> > 
> > Before the patch, the EFI code didn't look at the type *AT ALL*, it only
> > looked at the EFI_MEMORY_WB attribute.  This broke for SGI when they
> > were -- correctly -- reserving real memory (and hence still
> > EFI_MEMORY_WB) with the type set to EFI_RESERVED_TYPE.  This is correct
> > behavior, but the old code saw that it was EFI_MEMORY_WB and therefore
> > considered it usable RAM.  This is obviously broken.
> > 
> > Now why, you're asking, do we still look at md->attribute at all?
> > That's where caution dictates that it is prudent to diverge from the
> > previous behavior, but it is not *this* patch that should be the source
> > of that question, but from the author of the existing code, which
> > appears to be Paul Jackson of SGI.  Unfortunately, his email now bounces
> > and noone has that information.
> 
> Yes. You are right. Thank you for your patient.
> 
> > If you think about it, though, we don't want to consider it as usable
> > RAM if it isn't EFI_MEMORY_WB, and it would in fact be a bug (or
> > workaround for a broken system) to ignore it.  In fact, we go through
> > great pains elsewhere in the kernel to remove memory which isn't WB from
> > the usable pool.
> 
> Because it appears that checking EFI_MEMORY_WB is not necessary, maybe
> it is necessary to add some comments about why it is checked to prevent
> it to be deleted later.

Paul Jackson retired from SGI a while back.  I haven't seen him
participating in the LKML.  But he must have been trying to assure 
that, as Peter says, memory that isn't WB doesn't get into the usable
pool.

I think we are in agreement.  I propose the below, with the comment about WB.


---
 arch/x86/kernel/efi.c |   35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 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,39 @@ 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:
+			/*
+			 * make sure that memory that is not write-back does
+			 * not enter the usable memory pool
+			 */
+			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
cpw@sgi.com
(651) 683-3824