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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2020 23:11, Sergey Temerkhanov wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1100,7 +1100,9 @@ static void __init efi_exit_boot(EFI_HANDLE 
> ImageHandle, EFI_SYSTEM_TABLE *Syste
>      {
>          EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
>  
> -        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +        if ( (desc->Attribute & EFI_MEMORY_RUNTIME) ||
> +              desc->Type == EfiRuntimeServicesCode  ||

Nit: One too many blank at the start of the line and one too many
ahead of the ||. (Similar issues elsewhere.)

I think with the more involved logic this may also want a comment
indicating it needs keeping in sync with the other place (and a
similar comment should then go there as well).

> @@ -1510,6 +1512,21 @@ void __init efi_init_memory(void)
>                 desc->PhysicalStart, desc->PhysicalStart + len - 1,
>                 desc->Type, desc->Attribute);
>  
> +        if (efi_enabled(EFI_RS) &&
> +             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
> +                (desc->Type == EfiRuntimeServicesCode ||
> +                 desc->Type == EfiRuntimeServicesData))) {
> +            printk(XENLOG_WARNING "Fixing memory attributes for area %013"
> +                                   PRIx64 "-%013" PRIx64 "\n",
> +                   desc->PhysicalStart, desc->PhysicalStart + len - 1);

Please be more specific with the wording, e.g. "Adding runtime attribute
for area ...". Also please wrap the statement such that the format string
ends up all on one line, even if the line then exceeds the 80 cols limit.
For this you'll want to split between XENLOG_WARNING and the opening
quote.

> +            desc->Attribute |= EFI_MEMORY_RUNTIME;
> +            if ( !(desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) ) {
> +                desc->Attribute |= (desc->Type == EfiRuntimeServicesCode) &&
> +                                   (efi_bs_revision >= EFI_REVISION(2, 5)) ?
> +                                        EFI_MEMORY_WP : EFI_MEMORY_UC;

As said elsewhere already, please don't do this as a "proactive"
workaround.

Also please note that opening braces go on their own lines in our
coding style and there ought to be blanks immediately inside if()
and alike. See the surrounding code for reference.

> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -158,6 +158,9 @@ typedef enum {
>  #define EFI_MEMORY_UCE          0x0000000000000010  
>  #define EFI_MEMORY_WP           0x0000000000001000
>  
> +#define EFI_MEMORY_CACHEABILITY_MASK \
> +                                0x000000000000101F

Has such a #define appeared in the tree we've imported this from?
If not, it probably shouldn't go here. I'm also unconvinced
checking the bit EFI_MEMORY_WP occupies is, for the purpose here,
legitimate on versions prior to 2.5 - it's a reserved bit there,
and hence may have any [benign] meaning.

Finally, if this #define is to survive in the first place, please
don't use a literal number. Make its value an OR of the various
other constants instead, so it becomes self-documenting.

Jan