ESRT failures ... was Re: [PATCH 04/11] efi: Add efi_memmap_init_late() for permanent EFI memmap

DISCUSSION Prarit Bhargava 2 participants 11 messages Jul 13, 2016 – Jul 26, 2016 10 messages not in the archive EFI

Share & tools

Prarit Bhargava

author

2016-07-13 17:49 UTC #0 lore raw

[I am not subscribed to linux-efi. Sorry for the bad threading. Please include me on the cc on any replies.]

Matt, I grabbed your patches which are similar to a change that I wanted to make to the kernel. There is one issue though that I want to bring up.

With or without your changeset, I see the following issue on Dell system with a valid ESRT table.

[    0.000000] efi:  SMBIOS=0x8ce95000  ACPI=0x8d2fe000  ACPI 2.0=0x8d2fe014
ESRT=0x8d0f5000  MPS=0x8d0dd000
[    0.000000] efi: requested map not found.
[    0.000000] esrt: ESRT header is not in the memory map.
[    0.000000] SMBIOS 2.8 present.

This occurs AFAICT (with or without your changes) because of this check in efi_mem_desc_lookup():

                if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
                    md->type != EFI_BOOT_SERVICES_DATA &&
                    md->type != EFI_RUNTIME_SERVICES_DATA) {
                        continue;
                }

which then results in skipping the appropriate region. IIUC (and that's part of my question ;)) the above check means that the region has already been permanently mapped. That is,

rc = efi_mem_desc_lookup(efi.esrt, &md);
        if (rc < 0) {
                pr_err("ESRT header is not in the memory map.\n");
                return;
        }
...
        va = early_memremap(efi.esrt, size);
        if (!va) {

and rc is -ENOENT because a matching region couldn't be found. As can be seen the existing ESRT code will attempt to remap the region if not mapped.

I think that perhaps the function should be changed to do

        for_each_efi_memory_desc(md) {
                u64 size;
                u64 end;

                size = md->num_pages << EFI_PAGE_SHIFT;
                end = md->phys_addr + size;
                if (phys_addr >= md->phys_addr && phys_addr < end) {
                        memcpy(out_md, md, sizeof(*out_md));

                        if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
                            md->type != EFI_BOOT_SERVICES_DATA &&
                            md->type != EFI_RUNTIME_SERVICES_DATA) {
                                return UNMAPPED;
                        }

                        return MAPPED;
                }
        }

so that individual callers (like the ESRT code) can then make a decision on whether or not the area needs to be mapped.

Thoughts/concerns?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

2016-07-21 12:11 UTC #2 lore raw

Quoted 25 lines · Prarit Bhargava

[I am not subscribed to linux-efi. Sorry for the bad threading. Please include me on the cc on any replies.]
Matt, I grabbed your patches which are similar to a change that I wanted to make to the kernel. There is one issue though that I want to bring up.
With or without your changeset, I see the following issue on Dell system with a valid ESRT table.
[    0.000000] efi:  SMBIOS=0x8ce95000  ACPI=0x8d2fe000  ACPI 2.0=0x8d2fe014
ESRT=0x8d0f5000  MPS=0x8d0dd000
[    0.000000] efi: requested map not found.
[    0.000000] esrt: ESRT header is not in the memory map.
[    0.000000] SMBIOS 2.8 present.
This occurs AFAICT (with or without your changes) because of this check in efi_mem_desc_lookup():
                if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
                    md->type != EFI_BOOT_SERVICES_DATA &&
                    md->type != EFI_RUNTIME_SERVICES_DATA) {
                        continue;
                }

Quoted 3 lines

which then results in skipping the appropriate region. IIUC (and that's part of my question ;)) the above check means that the region has already been permanently mapped. That is,

No, the above check is intended to skip any regions that are not required at runtime and so won't be found in the EFI memmap.

The question is, what does the EFI memmap descriptor look like that describes your ESRT table?

Quoted 33 lines

rc = efi_mem_desc_lookup(efi.esrt, &md);
        if (rc < 0) {
                pr_err("ESRT header is not in the memory map.\n");
                return;
        }
...
        va = early_memremap(efi.esrt, size);
        if (!va) {

and rc is -ENOENT because a matching region couldn't be found. As can be seen the existing ESRT code will attempt to remap the region if not mapped.

I think that perhaps the function should be changed to do

        for_each_efi_memory_desc(md) {
                u64 size;
                u64 end;

                size = md->num_pages << EFI_PAGE_SHIFT;
                end = md->phys_addr + size;
                if (phys_addr >= md->phys_addr && phys_addr < end) {
                        memcpy(out_md, md, sizeof(*out_md));

                        if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
                            md->type != EFI_BOOT_SERVICES_DATA &&
                            md->type != EFI_RUNTIME_SERVICES_DATA) {
                                return UNMAPPED;
                        }

                        return MAPPED;
                }
        }

Quoted 4 lines

so that individual callers (like the ESRT code) can then make a decision on whether or not the area needs to be mapped.
Thoughts/concerns?

efi_mem_desc_lookup() should just return the correct EFI memory descriptor now that we have efi_mem_reserve() coming in v4.9.

What does the EFI memmap (as printed with efi=debug) look like on your machine?

A message that is not in the archive (sent off-list or not yet received): 20160721121136.GF26504-mF/unelCI9GS6iBeEJttW/[email protected]

Prarit Bhargava

author

2016-07-21 14:44 UTC #4 lore raw

Quoted 7 lines · Matt Fleming

efi_mem_desc_lookup() should just return the correct EFI memory descriptor now that we have efi_mem_reserve() coming in v4.9.
What does the EFI memmap (as printed with efi=debug) look like on your machine?

[    0.000000] efi: EFI v2.40 by Dell Inc.
[    0.000000] efi:  ESRT=0x8eb1c000  ACPI=0x8efe7000  ACPI 2.0=0x8efe7014

SMBIOS=0x8ef69000

and

[    0.000000] efi: mem249: [Reserved           |   |  |  |  |  |  |  |

|WB|WT|WC|UC] range=[0x000000008d6df000-0x000000008ef69fff] (24MB)

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

2016-07-22 13:11 UTC #6 lore raw

Quoted 11 lines · Prarit Bhargava

Quoted 7 lines · Matt Fleming
efi_mem_desc_lookup() should just return the correct EFI memory descriptor now that we have efi_mem_reserve() coming in v4.9.
What does the EFI memmap (as printed with efi=debug) look like on your machine?

Quoted 8 lines

[    0.000000] efi: EFI v2.40 by Dell Inc.
[    0.000000] efi:  ESRT=0x8eb1c000  ACPI=0x8efe7000  ACPI 2.0=0x8efe7014
SMBIOS=0x8ef69000
and
[    0.000000] efi: mem249: [Reserved           |   |  |  |  |  |  |  |
|WB|WT|WC|UC] range=[0x000000008d6df000-0x000000008ef69fff] (24MB)

Yeah, that's a Dell firmware bug. This should be EFI boot services data. Quoting the spec Section 22.3 EFI System Resource Table,

"See Section 4.6 for description of how to publish ESRT using
EFI_CONFIGURATION_TABLE. The ESRT shall be stored in memory of type EfiBootServicesData."

We do not use regions mark EFI_RESERVED_TYPE because they're basically supposed to be unusable by the OS, AFAIK. I guess we'll have to update efi_mem_desc_lookup() to return reserved regions and make sure they're always contained within the memmap.

A message that is not in the archive (sent off-list or not yet received): 20160722131101.GJ26504-mF/unelCI9GS6iBeEJttW/[email protected]

Prarit Bhargava

author

2016-07-22 13:31 UTC #8 lore raw

Quoted 24 lines · Matt Fleming

Quoted 11 lines · Prarit Bhargava
Quoted 7 lines · Matt Fleming
efi_mem_desc_lookup() should just return the correct EFI memory descriptor now that we have efi_mem_reserve() coming in v4.9.
What does the EFI memmap (as printed with efi=debug) look like on your machine?
Quoted 8 lines
[    0.000000] efi: EFI v2.40 by Dell Inc.
[    0.000000] efi:  ESRT=0x8eb1c000  ACPI=0x8efe7000  ACPI 2.0=0x8efe7014
SMBIOS=0x8ef69000
and
[    0.000000] efi: mem249: [Reserved           |   |  |  |  |  |  |  |
|WB|WT|WC|UC] range=[0x000000008d6df000-0x000000008ef69fff] (24MB)
Yeah, that's a Dell firmware bug. This should be EFI boot services data. Quoting the spec Section 22.3 EFI System Resource Table,

Quoted 9 lines

"See Section 4.6 for description of how to publish ESRT using
EFI_CONFIGURATION_TABLE. The ESRT shall be stored in memory of type EfiBootServicesData."
We do not use regions mark EFI_RESERVED_TYPE because they're basically supposed to be unusable by the OS, AFAIK. I guess we'll have to update efi_mem_desc_lookup() to return reserved regions and make sure they're always contained within the memmap.

Hmm ... maybe just a Dell specific quirk?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

2016-07-25 14:44 UTC #10 lore raw

Quoted 2 lines · Prarit Bhargava

Hmm ... maybe just a Dell specific quirk?

Quirks are best avoided at all costs, platform-specific quirks doubly so because there are just so many in the EFI arena that maintaining them all would be a nightmare.

Where possible we adopt a "most buggy implementation" approach to compatibility, as a least common denominator.

Could you try this patch out? It's not a final version, but I'd just like to see if anything else explodes when we start returning reserved regions.

---

drivers/firmware/efi/efi.c +2 -1

index 05509f3aaee8..9857796c4cd4 100644
@@ -299,7 +299,8 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
 		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
 		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
+		    md->type != EFI_RUNTIME_SERVICES_DATA &&
+		    md->type != EFI_RESERVED_TYPE) {
 			early_memunmap(md, sizeof (*md));
 			continue;
 		}

A message that is not in the archive (sent off-list or not yet received): 20160725144431.GA31759-mF/unelCI9GS6iBeEJttW/[email protected]

Prarit Bhargava

author

2016-07-25 15:11 UTC #12 lore raw

Quoted 22 lines · Matt Fleming

Quoted 2 lines · Prarit Bhargava
Hmm ... maybe just a Dell specific quirk?
Quirks are best avoided at all costs, platform-specific quirks doubly so because there are just so many in the EFI arena that maintaining them all would be a nightmare.
Where possible we adopt a "most buggy implementation" approach to compatibility, as a least common denominator.
Could you try this patch out? It's not a final version, but I'd just like to see if anything else explodes when we start returning reserved regions.
---
drivers/firmware/efi/efi.c +0 -0
index 05509f3aaee8..9857796c4cd4 100644

drivers/firmware/efi/efi.c int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) · line 303

Quoted 6 lines

drivers/firmware/efi/efi.c +2 -1

@@ -299,7 +299,8 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
 		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
 		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
+		    md->type != EFI_RUNTIME_SERVICES_DATA &&
+		    md->type != EFI_RESERVED_TYPE) {

I had tried this fix earlier FWIW and it causes the boot to hang.

Quoted 4 lines

drivers/firmware/efi/efi.c +0 -0

 			early_memunmap(md, sizeof (*md));
 			continue;
 		}

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

2016-07-25 15:13 UTC #14 lore raw

Quoted 26 lines · Prarit Bhargava

Quoted 23 lines · Matt Fleming
Quoted 2 lines · Prarit Bhargava
Hmm ... maybe just a Dell specific quirk?
Quirks are best avoided at all costs, platform-specific quirks doubly so because there are just so many in the EFI arena that maintaining them all would be a nightmare.
Where possible we adopt a "most buggy implementation" approach to compatibility, as a least common denominator.
Could you try this patch out? It's not a final version, but I'd just like to see if anything else explodes when we start returning reserved regions.
---
drivers/firmware/efi/efi.c +0 -0
index 05509f3aaee8..9857796c4cd4 100644
@@ -299,7 +299,8 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 

Quoted 7 lines

drivers/firmware/efi/efi.c int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) · line 303
Quoted 5 lines
drivers/firmware/efi/efi.c +2 -1
 		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
 		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
+		    md->type != EFI_RUNTIME_SERVICES_DATA &&
+		    md->type != EFI_RESERVED_TYPE) {
I had tried this fix earlier FWIW and it causes the boot to hang.

Do you know where it hangs? earlyprintk=efi might shed some light on that if you don't have a serial console setup.

A message that is not in the archive (sent off-list or not yet received): 20160725151344.GB31759-mF/unelCI9GS6iBeEJttW/[email protected]

Prarit Bhargava

author

2016-07-25 15:38 UTC #16 lore raw

Quoted 35 lines · Matt Fleming

Quoted 26 lines · Prarit Bhargava
Quoted 23 lines · Matt Fleming
Quoted 2 lines · Prarit Bhargava
Hmm ... maybe just a Dell specific quirk?
Quirks are best avoided at all costs, platform-specific quirks doubly so because there are just so many in the EFI arena that maintaining them all would be a nightmare.
Where possible we adopt a "most buggy implementation" approach to compatibility, as a least common denominator.
Could you try this patch out? It's not a final version, but I'd just like to see if anything else explodes when we start returning reserved regions.
---
drivers/firmware/efi/efi.c +0 -0
index 05509f3aaee8..9857796c4cd4 100644
@@ -299,7 +299,8 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
Quoted 7 lines
drivers/firmware/efi/efi.c int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) · line 303
Quoted 5 lines
drivers/firmware/efi/efi.c +2 -1
 		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
 		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
+		    md->type != EFI_RUNTIME_SERVICES_DATA &&
+		    md->type != EFI_RESERVED_TYPE) {
I had tried this fix earlier FWIW and it causes the boot to hang.

Quoted 3 lines

Do you know where it hangs? earlyprintk=efi might shed some light on that if you don't have a serial console setup.

Nope, nothing else is displayed with "earlyprintk=efi" as a kernel parameter (I already had that as a kernel parameter).

A message that is not in the archive (sent off-list or not yet received): [email protected]

Prarit Bhargava

author

2016-07-26 11:33 UTC #18 lore raw

Quoted 38 lines · Prarit Bhargava

Quoted 35 lines · Matt Fleming
Quoted 33 lines · Prarit Bhargava
Quoted 22 lines · Matt Fleming
Quoted 2 lines · Prarit Bhargava
Hmm ... maybe just a Dell specific quirk?
Quirks are best avoided at all costs, platform-specific quirks doubly so because there are just so many in the EFI arena that maintaining them all would be a nightmare.
Where possible we adopt a "most buggy implementation" approach to compatibility, as a least common denominator.
Could you try this patch out? It's not a final version, but I'd just like to see if anything else explodes when we start returning reserved regions.
---
drivers/firmware/efi/efi.c +0 -0
index 05509f3aaee8..9857796c4cd4 100644
drivers/firmware/efi/efi.c int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md) · line 303
Quoted 6 lines
drivers/firmware/efi/efi.c +2 -1
@@ -299,7 +299,8 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
 		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
 		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
+		    md->type != EFI_RUNTIME_SERVICES_DATA &&
+		    md->type != EFI_RESERVED_TYPE) {
I had tried this fix earlier FWIW and it causes the boot to hang.

Quoted 6 lines

Quoted 3 lines
Do you know where it hangs? earlyprintk=efi might shed some light on that if you don't have a serial console setup.
Nope, nothing else is displayed with "earlyprintk=efi" as a kernel parameter (I already had that as a kernel parameter).

I was ping'd by a Dell engineer who says that Dell is taking a look at this. Most likely a BIOS fix will be released.

... I think that means I should drop the issue :)

Quoted 3 lines

P.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matt Fleming

2016-07-26 14:26 UTC #20 lore raw

Quoted 5 lines · Prarit Bhargava

I was ping'd by a Dell engineer who says that Dell is taking a look at this. Most likely a BIOS fix will be released.
... I think that means I should drop the issue :)

It would be valuable if they could still provide details on the issue though. Not everyone will upgrade their BIOS and it would be nice to have things work out of the box for those machines currently sat on shelves in warehouses, i.e. where a fixed BIOS cannot be installed before the user receives it.

Feel free to pull whoever pinged you into this thread.