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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 12/01/2022 08:45, Jan Beulich wrote:
> From: Sergey Temerkhanov <s.temerkhanov@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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