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

Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Fri, 24 Sep 2021 16:28:47 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.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; bh=p4xggSuehu3Z4kQU35gQC/BEPRVfobQILqP/luDi4qg=; b=dxKk/RaP7r/jkgvzTFRoZD+wCbxCWxxloXv2CdkjBVgXw2n8PmVHoUVJ6OQ4w1q9BO8FVTdDiCKJQxPC+5Opw9gBpX3NYS1BgTH4em1Dgb2hUJSLj0KR66MYqkr3YVKep5c3j5foH+GS1O746IDdnBG+CNHNfaxGozczlQkiJ4tJRgPVKIjf0sDkMVzruesQFHkpmxeiEZ7mdeA+WlB44UtySE9AzLtQQnIPIgqpg0/gBpd2O+eocvXEanlDHpeVL46qCOmiNVMMVhJXO2HkMcGQoCwQGwKg1BZgAFgAiS2kKzG5r+jgjMT03TLT+6EJ4VmaupyMD+xOLoiCNun4pA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lKmjoxHnhfJyt6UwfWRF1hTPHmEgGJLQ8mFuFJtGa95lV1aY3+YpbeMsTK6LfrpruRjKF7kKw43czoBSmt537rZjqGv6CEx0w3tdVjk1QL10oFDau7tFOFXFf/sCLZePcP+TNzDstcy28hxbCV19xCO/DKGiT+3fFDJJlbbazlWBnTouC4INOjM3kClXHtlAjIfMj/2JUzKx6gOsitO3YYkSOAQcqJkLQbPoROJE0Zn9x9m+ISBTgkB/t+rhWcgpY372bLH9V+rTViCGDoBY8WjYZZALDArMvnPhw1tXEUf05+0+PRM6p01l/Sel/3YeQIl3YwMLRDGiBhYzyUnF7g==
  • Authentication-results-original: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;
  • Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>, wei.chen@xxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Fri, 24 Sep 2021 15:29:22 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=arm.com;


> On 24 Sep 2021, at 15:02, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 22.09.2021 16:13, Luca Fancellu wrote:
>> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE 
>> dir_handle,
>> +                                                  const char *name,
>> +                                                  unsigned int name_len)
>> +{
>> +    dom0less_module_name* file_name;
>> +    union string module_name;
>> +    unsigned int ret_idx;
>> +
>> +    /*
>> +     * Check if there is any space left for a domU module, the variable
>> +     * dom0less_modules_available is updated each time we use read_file(...)
>> +     * successfully.
>> +     */
>> +    if ( !dom0less_modules_available )
>> +        blexit(L"No space left for domU modules");
>> +
>> +    module_name.s = (char*) name;
> 
> Unfortunately there are too many style issues in these Arm additions to
> really enumerate; I'd like to ask that you go through yourself with
> ./CODING_STYLE, surrounding code, and review comments on earlier patches
> of yours in mind. This cast stands out, though: I'm pretty sure you were
> told before that casts are often dangerous and hence should be avoided
> whenever (easily) possible. There was a prior case where union string
> was used in a similar way, not all that long ago. Hence why it now has
> a "const char *" member. (That's still somewhat risky, but imo way
> better than a cast.)

Hi Jan,

Yes I will use the .cs member, I will have also a better look on the patch to
find the style issues.

> 
>> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>>         cfg.addr = 0;
>> 
>> -        dir_handle->Close(dir_handle);
>> -
>>         if ( gop && !base_video )
>>             gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>>     }
>> 
>> +    /*
>> +     * Check if a proper configuration is provided to start Xen:
>> +     *  - Dom0 specified (minimum required)
>> +     *  - Dom0 and DomU(s) specified
>> +     *  - DomU(s) specified
>> +     */
>> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
>> +        blexit(L"No Dom0 kernel image specified.");
>> +
>> +    dir_handle->Close(dir_handle);
> 
> So far I was under the impression that handles and alike need closing
> before calling Exit(), to prevent resource leaks. While I will admit
> that likely there are more (pre-existing) affected paths, I think that
> - if that understanding of mine is correct - it would be nice to avoid
> adding yet more instances.

Ok sure, I will close the handle before the blexit.

Cheers,
Luca

> 
> Jan
> 




 


Rackspace

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