[PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}

Jan Beulich posted 1 patch 2 years, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/89d182f6-95e8-674a-2297-6e98435385f8@suse.com
[PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
Posted by Jan Beulich 2 years, 2 months ago
From: Sergey Temerkhanov <s.temerkhanov@gmail.com>

This helps overcome problems observed with some UEFI implementations
which don't set the Attributes field in memery descriptors properly.

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
    switch()). Style.
---
I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
yet another patch then I suppose.

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
     {
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
 
-        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+        /*
+         * Runtime services regions are always mapped here.
+         * Attributes may be adjusted in efi_init_memory().
+         */
+        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
+             desc->Type == EfiRuntimeServicesCode ||
+             desc->Type == EfiRuntimeServicesData )
             desc->VirtualStart = desc->PhysicalStart;
         else
             desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
@@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
                     ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
         }
 
-        if ( !efi_enabled(EFI_RS) ||
-             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
-              (!map_bs ||
-               (desc->Type != EfiBootServicesCode &&
-                desc->Type != EfiBootServicesData))) )
+        if ( !efi_enabled(EFI_RS) )
             continue;
 
+        if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+        {
+            switch ( desc->Type )
+            {
+            default:
+                continue;
+
+            /*
+             * Adjust runtime services regions. Keep in sync with
+             * efi_exit_boot().
+             */
+            case EfiRuntimeServicesCode:
+            case EfiRuntimeServicesData:
+                printk(XENLOG_WARNING
+                       "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
+                       desc->PhysicalStart, desc->PhysicalStart + len - 1);
+                desc->Attribute |= EFI_MEMORY_RUNTIME;
+                break;
+
+            case EfiBootServicesCode:
+            case EfiBootServicesData:
+                if ( !map_bs )
+                    continue;
+                break;
+            }
+        }
+
         desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
 
         smfn = PFN_DOWN(desc->PhysicalStart);


Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
Posted by Luca Fancellu 2 years, 2 months ago

> On 12 Jan 2022, at 08:45, Jan Beulich <jbeulich@suse.com> wrote:
> 
> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> 
> This helps overcome problems observed with some UEFI implementations
> which don't set the Attributes field in memery descriptors properly.
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Hi,

I’ve tested this patch on an arm machine with UEFI boot and it works fine.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

> ---
> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>    switch()). Style.
> ---
> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
> yet another patch then I suppose.
> 
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>     {
>         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> 
> -        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +        /*
> +         * Runtime services regions are always mapped here.
> +         * Attributes may be adjusted in efi_init_memory().
> +         */
> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +             desc->Type == EfiRuntimeServicesCode ||
> +             desc->Type == EfiRuntimeServicesData )
>             desc->VirtualStart = desc->PhysicalStart;
>         else
>             desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> @@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
>                     ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
>         }
> 
> -        if ( !efi_enabled(EFI_RS) ||
> -             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> -              (!map_bs ||
> -               (desc->Type != EfiBootServicesCode &&
> -                desc->Type != EfiBootServicesData))) )
> +        if ( !efi_enabled(EFI_RS) )
>             continue;
> 
> +        if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> +        {
> +            switch ( desc->Type )
> +            {
> +            default:
> +                continue;
> +
> +            /*
> +             * Adjust runtime services regions. Keep in sync with
> +             * efi_exit_boot().
> +             */
> +            case EfiRuntimeServicesCode:
> +            case EfiRuntimeServicesData:
> +                printk(XENLOG_WARNING
> +                       "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
> +                       desc->PhysicalStart, desc->PhysicalStart + len - 1);
> +                desc->Attribute |= EFI_MEMORY_RUNTIME;
> +                break;
> +
> +            case EfiBootServicesCode:
> +            case EfiBootServicesData:
> +                if ( !map_bs )
> +                    continue;
> +                break;
> +            }
> +        }
> +
>         desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> 
>         smfn = PFN_DOWN(desc->PhysicalStart);
> 
> 


Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
Posted by Julien Grall 2 years, 2 months ago
Hi Jan,

On 12/01/2022 08:45, Jan Beulich wrote:
> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> 
> This helps overcome problems observed with some UEFI implementations

Would it be possible to provide some details about the platform? This is 
helpful to track why a workaround is present.

> which don't set the Attributes field in memery descriptors properly.

typo: s/memery/memory/

> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>      switch()). Style.
> ---
> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
> yet another patch then I suppose.

IIUC, we would need to invalidate the mapping when map_bs was used. Is 
it correct? If so, then I agree this sounds separate to the issue you 
are describing here.

> 
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>       {
>           EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>   
> -        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +        /*
> +         * Runtime services regions are always mapped here.
> +         * Attributes may be adjusted in efi_init_memory().
> +         */
> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +             desc->Type == EfiRuntimeServicesCode ||
> +             desc->Type == EfiRuntimeServicesData )

My knowledge with the stub is limited. Would you be able to explain why 
we need to map the runtime services even with !efi_enabled(EFI_RS)?

The reason I am asking is if we don't need it, then it would be 
sufficient to rely on desc->Attribute because you updated it in 
efi_init_memory().

>               desc->VirtualStart = desc->PhysicalStart;
>           else
>               desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
> @@ -1545,13 +1551,36 @@ void __init efi_init_memory(void)
>                       ROUNDUP(desc->PhysicalStart + len, PAGE_SIZE));
>           }
>   
> -        if ( !efi_enabled(EFI_RS) ||
> -             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> -              (!map_bs ||
> -               (desc->Type != EfiBootServicesCode &&
> -                desc->Type != EfiBootServicesData))) )
> +        if ( !efi_enabled(EFI_RS) )
>               continue;
>   
> +        if ( !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> +        {
> +            switch ( desc->Type )
> +            {
> +            default:
> +                continue;
> +
> +            /*
> +             * Adjust runtime services regions. Keep in sync with
> +             * efi_exit_boot().
> +             */
> +            case EfiRuntimeServicesCode:
> +            case EfiRuntimeServicesData:
> +                printk(XENLOG_WARNING
> +                       "Setting RUNTIME attribute for %013" PRIx64 "-%013" PRIx64 "\n",
> +                       desc->PhysicalStart, desc->PhysicalStart + len - 1);
> +                desc->Attribute |= EFI_MEMORY_RUNTIME;
> +                break;
> +
> +            case EfiBootServicesCode:
> +            case EfiBootServicesData:
> +                if ( !map_bs )
> +                    continue;
> +                break;
> +            }
> +        }

The new code is much easier to read :).


> +
>           desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
>   
>           smfn = PFN_DOWN(desc->PhysicalStart);
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
Posted by Jan Beulich 2 years, 2 months ago
On 27.01.2022 11:45, Julien Grall wrote:
> On 12/01/2022 08:45, Jan Beulich wrote:
>> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>
>> This helps overcome problems observed with some UEFI implementations
> 
> Would it be possible to provide some details about the platform? This is 
> helpful to track why a workaround is present.

I cannot provide any details. I took over the patch after pinging
Sergey more than once, without any reaction. It was him to actually
run into the problem in the wild.

>> which don't set the Attributes field in memery descriptors properly.
> 
> typo: s/memery/memory/
> 
>>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>>      switch()). Style.
>> ---
>> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
>> yet another patch then I suppose.
> 
> IIUC, we would need to invalidate the mapping when map_bs was used. Is 
> it correct? If so, then I agree this sounds separate to the issue you 
> are describing here.

No, the other way around: We'd also need to put in a valid virtual
address there for EfiBootServices{Code,Data}. I expect this wasn't
done when map_bs was introduced because iirc at that time the code
fragment in efi_exit_boot() was entirely dead, present merely for
documentation purposes.

>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>>       {
>>           EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>   
>> -        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>> +        /*
>> +         * Runtime services regions are always mapped here.
>> +         * Attributes may be adjusted in efi_init_memory().
>> +         */
>> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
>> +             desc->Type == EfiRuntimeServicesCode ||
>> +             desc->Type == EfiRuntimeServicesData )
> 
> My knowledge with the stub is limited. Would you be able to explain why 
> we need to map the runtime services even with !efi_enabled(EFI_RS)?

In principle we wouldn't need to, but the final setting of this bit
isn't known yet at this point - it'll be known only after parsing
the command line (see parse_efi_param()).

Jan


Re: [PATCH v4] EFI: always map EfiRuntimeServices{Code,Data}
Posted by Julien Grall 2 years, 1 month ago
Hi Jan,

On 27/01/2022 12:25, Jan Beulich wrote:
> On 27.01.2022 11:45, Julien Grall wrote:
>> On 12/01/2022 08:45, Jan Beulich wrote:
>>> From: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>>
>>> This helps overcome problems observed with some UEFI implementations
>>
>> Would it be possible to provide some details about the platform? This is
>> helpful to track why a workaround is present.
> 
> I cannot provide any details. I took over the patch after pinging
> Sergey more than once, without any reaction. It was him to actually
> run into the problem in the wild.

This is a quite unfortunate.

> 
>>> which don't set the Attributes field in memery descriptors properly.
>>
>> typo: s/memery/memory/
>>
>>>
>>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v4: Drop EFI_MEMORY_CACHEABILITY_MASK. Fold with pre-existing if() (into
>>>       switch()). Style.
>>> ---
>>> I guess "map_bs" would also want honoring in efi_exit_boot(), but that's
>>> yet another patch then I suppose.
>>
>> IIUC, we would need to invalidate the mapping when map_bs was used. Is
>> it correct? If so, then I agree this sounds separate to the issue you
>> are describing here.
> 
> No, the other way around: We'd also need to put in a valid virtual
> address there for EfiBootServices{Code,Data}. I expect this wasn't
> done when map_bs was introduced because iirc at that time the code
> fragment in efi_exit_boot() was entirely dead, present merely for
> documentation purposes.
> 
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -1094,7 +1094,13 @@ static void __init efi_exit_boot(EFI_HAN
>>>        {
>>>            EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>>>    
>>> -        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>>> +        /*
>>> +         * Runtime services regions are always mapped here.
>>> +         * Attributes may be adjusted in efi_init_memory().
>>> +         */
>>> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
>>> +             desc->Type == EfiRuntimeServicesCode ||
>>> +             desc->Type == EfiRuntimeServicesData )
>>
>> My knowledge with the stub is limited. Would you be able to explain why
>> we need to map the runtime services even with !efi_enabled(EFI_RS)?
> 
> In principle we wouldn't need to, but the final setting of this bit
> isn't known yet at this point - it'll be known only after parsing
> the command line (see parse_efi_param()).

Oh. I thought efi_exit_boot() was called *after* efi_init_memory(). Your 
answer about 'map_bs' also make more sense.

Thanks for the explanation!

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall