Re: [PATCH v4] Preserve the EFI System Resource Table for dom0

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, May 06, 2022 at 12:59:05PM +0200, Jan Beulich wrote:
> On 05.05.2022 07:38, Demi Marie Obenour wrote:
> > @@ -1056,13 +1091,11 @@ 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);
> 
> I think I did ask on an earlier version already why you're making this
> change. It continues to look to me like a leftover which was needed by
> an early version only.

Will revert in v5.

> > @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE 
> > ImageHandle, EFI_SYSTEM_TABLE *Syste
> >          if ( EFI_ERROR(status) )
> >              PrintErrMesg(L"Cannot obtain memory map", status);
> >  
> > +        for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > +        {
> > +            if ( !is_esrt_valid(efi_memmap + i) )
> > +                continue;
> 
> Instead of repeating the size calculation below, could you make the
> function (with an altered name) simply return the size (and zero if
> not [valid] ESRT), simplifying things below?

Will fix in v5.

> > +            if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
> > +                 EfiRuntimeServicesData )
> > +            {
> > +                /* ESRT needs to be moved to memory of type 
> > EfiRuntimeServicesData
> > +                 * so that the memory it is in will not be used for other 
> > purposes */
> 
> Nit: Comment style.

Will fix in v5.

> > +                size_t esrt_size = offsetof(ESRT, Entries) +
> > +                    ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
> > +                void *new_esrt = NULL;
> > +                status = efi_bs->AllocatePool(EfiRuntimeServicesData, 
> > esrt_size, &new_esrt);
> 
> Nit: Please have a blank line between declaration(s) and statement(s).

Will fix in v5.

> > +                if ( status != EFI_SUCCESS )
> > +                {
> > +                    PrintErrMesg(L"Cannot allocate memory for ESRT", 
> > status);
> 
> Neither this nor ...
> 
> > +                    break;
> > +                }
> > +                memcpy(new_esrt, (void *)esrt, esrt_size);
> > +                status = efi_bs->InstallConfigurationTable(&esrt_guid, 
> > new_esrt);
> > +                if ( status != EFI_SUCCESS )
> > +                {
> > +                    PrintErrMesg(L"Cannot install new ESRT", status);
> > +                    efi_bs->FreePool(new_esrt);
> 
> ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
> ends in blexit().

Whoops!  I did not realized PrintErrMsg() was fatal.

> > +                }
> > +            }
> > +            break;
> > +        }
> > +
> >          efi_arch_process_memory_map(SystemTable, efi_memmap, 
> > efi_memmap_size,
> >                                      efi_mdesc_size, mdesc_ver);
> 
> The allocation may have altered the memory map and hence invalidated what
> was retrieved just before. You'd need to "continue;" without setting
> "retry" to true, but then the question is why you make this allocation
> after retrieving the memory map in the first place. It's not entirely
> clear to me if it can be done _much_ earlier (if it can, doing it earlier
> would of course be better), but since you need to do it before
> ExitBootServices() anyway, and since you will need to call GetMemoryMap()
> afterwards again, you could as well do it before calling GetMemoryMap().

This would mean the allocation would need to be unconditional.  Right
now, I avoid the allocation if it is not necessary.

> > --- 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;
> 
> I'm pretty sure I did indicate before that types used in just a single
> source file should be put in that source file, unless we obtain them
> by importing a header (e.g. the ones in include/efi/) from elsewhere.

Will fix in v5.

> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -269,7 +269,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;
> >              u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> >  
> >              if ( info->mem.addr >= desc->PhysicalStart &&
> 
> With the restructured approach I don't think this stray change should
> be left in here anymore. Or am I overlooking anything requiring this
> adjustment?

It’s a trivial cleanup but I can get rid of it.

> > --- a/xen/include/efi/efiapi.h
> > +++ b/xen/include/efi/efiapi.h
> > @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
> >  #define SAL_SYSTEM_TABLE_GUID    \
> >      { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
> > 0xc1, 0x4d} }
> >  
> > +#define ESRT_GUID    \
> > +    { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 
> > 0x21, 0x80} }
> 
> Like above I'm pretty sure I did ask that you do not alter this
> imported header. If gnu-efi now has these definitions, we should
> import them all in one go (i.e. then the two struct declarations
> would also want to go into their appropriate place under include/efi/.
> Otherwise this wants putting next to the other GUIDs defined in
> boot.c.

Will fix in v5.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature