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

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

[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;
                }

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?

P.

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

[Matt Fleming]

On Wed, 13 Jul, at 01:49:38PM, Prarit Bhargava wrote:
> [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,
 
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?

> 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?

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?

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

[Prarit Bhargava]

On 07/21/2016 08:11 AM, Matt Fleming wrote:
> 
> 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)

P.

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

[Matt Fleming]

On Thu, 21 Jul, at 10:44:32AM, Prarit Bhargava wrote:
> 
> 
> On 07/21/2016 08:11 AM, Matt Fleming wrote:
> > 
> > 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)

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.

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

[Prarit Bhargava]

On 07/22/2016 09:11 AM, Matt Fleming wrote:
> On Thu, 21 Jul, at 10:44:32AM, Prarit Bhargava wrote:
>>
>>
>> On 07/21/2016 08:11 AM, Matt Fleming wrote:
>>>
>>> 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)
> 
> 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.
> 

Hmm ... maybe just a Dell specific quirk?

P.

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

[Matt Fleming]

On Fri, 22 Jul, at 09:31:20AM, Prarit Bhargava wrote:
> 
> 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.

---

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 05509f3aaee8..9857796c4cd4 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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;
 		}

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

[Prarit Bhargava]

On 07/25/2016 10:44 AM, Matt Fleming wrote:
> On Fri, 22 Jul, at 09:31:20AM, Prarit Bhargava wrote:
>>
>> 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.
> 
> ---
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 05509f3aaee8..9857796c4cd4 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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.

P.

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

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

[Matt Fleming]

On Mon, 25 Jul, at 11:11:31AM, Prarit Bhargava wrote:
> 
> 
> On 07/25/2016 10:44 AM, Matt Fleming wrote:
> > On Fri, 22 Jul, at 09:31:20AM, Prarit Bhargava wrote:
> >>
> >> 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.
> > 
> > ---
> > 
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 05509f3aaee8..9857796c4cd4 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -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.

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

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

[Prarit Bhargava]

On 07/25/2016 11:13 AM, Matt Fleming wrote:
> On Mon, 25 Jul, at 11:11:31AM, Prarit Bhargava wrote:
>>
>>
>> On 07/25/2016 10:44 AM, Matt Fleming wrote:
>>> On Fri, 22 Jul, at 09:31:20AM, Prarit Bhargava wrote:
>>>>
>>>> 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.
>>>
>>> ---
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index 05509f3aaee8..9857796c4cd4 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -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.
> 
> 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).

P.

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

[Prarit Bhargava]

On 07/25/2016 11:38 AM, Prarit Bhargava wrote:
> 
> 
> On 07/25/2016 11:13 AM, Matt Fleming wrote:
>> On Mon, 25 Jul, at 11:11:31AM, Prarit Bhargava wrote:
>>>
>>>
>>> On 07/25/2016 10:44 AM, Matt Fleming wrote:
>>>> On Fri, 22 Jul, at 09:31:20AM, Prarit Bhargava wrote:
>>>>>
>>>>> 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.
>>>>
>>>> ---
>>>>
>>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>>> index 05509f3aaee8..9857796c4cd4 100644
>>>> --- a/drivers/firmware/efi/efi.c
>>>> +++ b/drivers/firmware/efi/efi.c
>>>> @@ -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.
>>
>> 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 :)

P.

> 
> P.
> 

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

[Matt Fleming]

On Tue, 26 Jul, at 07:33:56AM, Prarit Bhargava wrote:
> 
> 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.