[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 17 May 2022 15:16:55 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=v0UZ/OLygyxf38LEAdm98Ux14EuOFQE+8OC+NMGCUQI=; b=Qhrt6K0xjV6G8UcsZKsWQqaEagcRb9oKUxArllvu4EmKu2M7a3wIhnIz/H1EyO1DBp/LWNOMn06o0BvHjgWFJ5RkK2/Lk4UXTA95pTru7YqQWIgUeHvgS05C/OSKZnzsVqLuWxIXinAPxPWrSydQzdvFOApk43xKcG0tMTRtzE7u9CbKot7/69hkX78BFflqePsLbmQ6jwXVDG6EY/bY/FFMmw67MedrHz7fgs+Y72KdLoKRZItQebORO202zHCHxlvC3XUJgA1JLnDLUB2rDNyBTLI8/hI0qTC+niuO8mPA4aIfxdZPQEWux9diZ/plt0u1K3gmRBv7fCvRVf8dQw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=V5+BvwRAbB6ybPNfHiHWH2Ol+d6m1qBbDtG6MzPG7q4+xIZoFD8PiWlc6W1TkF1zqkcMvmwvWtl3KEjWPwOWUZRS+A2Ad5Bdf9niJ+17McaoUTcOa/5rSwN5H3uwcv5VLSmiVmh1IfJGCPp8el0iZz6XOVhtf/FSkeNzR99mt+ZlmqgLzks0vQPom0NxBObu7eC5UQJ4oduYSj7S4idhTbV+16bZ6X8iIfMNj58w4hQeF44O/yur7ARGaPajHJEGdbbFkCEstzt7O/Bm8+JCwUGaCPF7lsl0KQX+fWDlNzgv7OhpTRhaxFoj6+982PZJQS21hQtBfKUDzk6IuuhzWw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 17 May 2022 13:16:54 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 07.05.2022 06:26, Demi Marie Obenour wrote:
> On Fri, May 06, 2022 at 12:59:05PM +0200, Jan Beulich wrote:
>> On 05.05.2022 07:38, Demi Marie Obenour wrote:
>>> @@ -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.

Hmm, I guess I don't see (taking into account also my own reply to that
comment of mine) why it would need to become unconditional then.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.