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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 30 Sep 2022, at 09:50, Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> #arm

Cheers
Bertrand

> ---
> 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.
> 
> --- 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: