Re: [PATCH v3 1/4] Grab the EFI System Resource Table and check it

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2022 17:40, Demi Marie Obenour wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -171,7 +171,7 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>          {
>          case EfiBootServicesCode:
>          case EfiBootServicesData:
> -            if ( map_bs )
> +            if ( map_bs || desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )

No need for the cast afaics, even more so that it casts away const-ness.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -567,6 +567,38 @@ static int __init efi_check_dt_boot(const 
> EFI_LOADED_IMAGE *loaded_image)
>  }
>  #endif
>  
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> +
> +static bool __init is_esrt_valid(
> +    const EFI_MEMORY_DESCRIPTOR *const desc)

As indicated elsewhere before, while we want to have const on pointed-to
types whenever possible, the 2nd const here is unusual in our code base
and hence would imo better be omitted.

> @@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE 
> ImageHandle, EFI_SYSTEM_TABLE *Syste
>      EFI_STATUS status;
>      UINTN info_size = 0, map_key;
>      bool retry;
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
>      unsigned int i;
> -#endif
>  
>      efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
>                           &efi_mdesc_size, &mdesc_ver);
> -    info_size += 8 * efi_mdesc_size;
> +    info_size += 8 * (efi_mdesc_size + 1);

What is this needed for? Does this perhaps belong into a later patch?

> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -10,6 +10,23 @@
>  #include <xen/spinlock.h>
>  #include <asm/page.h>
>  
> +typedef struct _ESRT_ENTRY {
> +    EFI_GUID FwClass;
> +    UINT32 FwType;
> +    UINT32 FwVersion;
> +    UINT32 FwLowestSupportedVersion;
> +    UINT32 FwCapsuleFlags;
> +    UINT32 FwLastAttemptVersion;
> +    UINT32 FwLastAttemptStatus;
> +} ESRT_ENTRY;
> +
> +typedef struct _ESRT {
> +    UINT32 Count;
> +    UINT32 Max;
> +    UINT64 Version;
> +    ESRT_ENTRY Entries[];
> +} ESRT;

The names in the spec, which (as said before) we're trying to follow along
with the gnu-efi package, where we would generally be taking things from,
are EFI_SYSTEM_RESOURCE_ENTRY and EFI_SYSTEM_RESOURCE_TABLE. The field
names of the former also don't all start with "Fw". The field names of the
latter are still quite far off of what the spec says.

Also, why did you move this here? There's no need to expose things in a
header which are used by a single CU.

> @@ -269,7 +270,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>      case XEN_FW_EFI_MEM_INFO:
>          for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>          {
> -            EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> +            const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;

While I don't mind this change, it also looks unrelated. Perhaps again
needed by (and then supposed to be in) a later patch?

Jan