Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi Jan,

We will review and test the arm part (even though it is modifying some unused
 code at the moment) but I wanted to answer you on some questions you have ..

> On 30 Sep 2022, at 09:50, Jan Beulich <jbeulich@suse.com> wrote:
> 
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Partly RFC for Arm, for two reasons:
> 
> On Arm I question the conversion of EfiACPIReclaimMemory, in two ways:
> For one like on x86 such ranges would likely better be retained, as Dom0
> may (will?) have a need to look at tables placed there. Plus converting
> such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to
> me as well. I'd be inclined to make the latter adjustment right here
> (while the other change probably would better be separate, if there
> aren't actually reasons for the present behavior).
> 
> On Arm efi_init_memory() is compiled out, so adjusting Arm code here is
> perhaps more for consistency (not leaving a trap for someone to later
> fall into) than a strict requirement. I wonder though how Arm has
> managed to get away without at least some parts of efi_init_memory() for
> all the years that ACPI support has been present there. I guess this is
> connected to most of runtime.c also being compiled out, but that
> continuing to be the case is another aspect puzzling me.

On arm we only use the boot services in Xen and we do not provide
any efi services to dom0. The required info is passed through a simple device
tree.
There was a discussion on that subject some weeks ago and it is still an open
point to be solved.
Also APCI is officially unsupported on arm.

Cheers
Bertrand

> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem
> 
>     for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>     {
> -        if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
> -             (desc_ptr->Type == EfiConventionalMemory ||
> -              desc_ptr->Type == EfiLoaderCode ||
> -              desc_ptr->Type == EfiLoaderData ||
> -              (!map_bs &&
> -               (desc_ptr->Type == EfiBootServicesCode ||
> -                desc_ptr->Type == EfiBootServicesData))) )
> +        if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME )
> +            /* nothing */;
> +        else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) &&
> +                  (desc_ptr->Type == EfiConventionalMemory ||
> +                   desc_ptr->Type == EfiLoaderCode ||
> +                   desc_ptr->Type == EfiLoaderData ||
> +                   (!map_bs &&
> +                    (desc_ptr->Type == EfiBootServicesCode ||
> +                     desc_ptr->Type == EfiBootServicesData))) )
>         {
>             if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
>             {
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -185,7 +185,9 @@ static void __init efi_arch_process_memo
>             /* fall through */
>         case EfiLoaderCode:
>         case EfiLoaderData:
> -            if ( desc->Attribute & EFI_MEMORY_WB )
> +            if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +                type = E820_RESERVED;
> +            else if ( desc->Attribute & EFI_MEMORY_WB )
>                 type = E820_RAM;
>             else
>         case EfiUnusableMemory: