Re: [PATCH] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2020 01:24, Sergey Temerkhanov wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void)

Looking at the line numbers - is this patch against the master
or staging branch? I ask because about as far away from the line
number above as the chunk of cose you mean to change there's a
very similar conditional, which has caused some slight confusion
over here.

>          }
>  
>          if ( !efi_enabled(EFI_RS) ||
> -             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +             ((!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +                (desc->Type != EfiRuntimeServicesCode &&
> +                 desc->Type != EfiRuntimeServicesData)) &&
>                (!map_bs ||
>                 (desc->Type != EfiBootServicesCode &&
>                  desc->Type != EfiBootServicesData))) )

I'm in principle okay with a workaround like this, but I don't
think it should go silently. I'd therefore like to suggest you
add a new if() ahead of this one and then set
EFI_MEMORY_RUNTIME in affected descriptors (to keep things
consistent with other consumers of the memory map without
having to update every one of those checking for the flag)
alongside issuing a log message.

There's nevertheless another piece of code you need to adjust,
inside a CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP conditional in
efi_exit_boot(). But you shouldn't adjust the descriptor
there, yet - this should happen only after its logging in
efi_init_memory().

Additionally I'd like it to be at least considered to also
check that EFI_MEMORY_WB (or at the very least one of the
cachability flags) is set, so that we won't run into the
path further down complaining about a lack thereof in this
case.

Jan