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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Thu, 16 Sep 2021 16:07:40 +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=OuDY7kva6TB+DFTqavYUDWtKCiVsaC/k4SIMhEkr47k=; b=Qf/eRcgLoBxvAkydXg+uKcxScqHaVkz16Bfrmwn93as0gQEDuJfY3exDKlmOJlHpWsttjNXLV3Z3Upep/G2kcVBcdN1jRy8f2FhLPaT6HOLxn2Wmic5ktYCvzaiS3yNgKmcHwXufhwAhgIyRBwdUvIUr/zykAxuAySyR2LhXiKT8UQ5RYBWPi4dEFxOuZQV/mfVYScEwveBk5qvUh+4SE+73JmxkLVd99nIHGniF0wvt8QqIN9MmnD72UFV1eLtZgAqu92JXiKhyEk/v06CxSYnA8N4ehIxR0URcn2fVE+U78e+hOPntABTmZFMzzaQDgNhD6tCCl9VWnLUciY99Sw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Vy7ojjr1QGzINLrWdtFc45jCxnNzbY8hpNzTzXH4ihyetVIcvwj0MCwgRPWjUj5fsI7QPskbcNy38KVUCEAGFx8vE0cfIFdQmoXSDDkxomaLwewJlo2HVnAJ5QNeIkybbqVX0hPuW3cYH8kfd7w3cGZFFz+KHvbD/Wj0jIsAq+9uCt4OzdwjfWC/AjMKlZ2LKYv1b42tOJMYHWXvw+jIyZElXe7FsBDd6AzdMsUOztNTYpemBP/YHXZT0eluw+BOqaA2qaYfiG1woNSnyRZk6qaGGHGPtWy1DYYRsnKztj4stBpoYyc696kOJbR0eMi7V161m3EzbBNxs0AOb8Bctw==
  • 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, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 16 Sep 2021 15:08:14 +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 16 Sep 2021, at 13:15, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.09.2021 13:28, Luca Fancellu wrote:
>>> On 16 Sep 2021, at 09:46, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 15.09.2021 16:26, Luca Fancellu wrote:
>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>> @@ -8,9 +8,39 @@
>>>> #include <asm/setup.h>
>>>> #include <asm/smp.h>
>>>> 
>>>> +typedef struct {
>>>> +    char* name;
>>> 
>>> Misplaced *.
>> 
>> I was looking in the CODING_STYLE and I didn’t found anything that mandates
>> char *name; instead of char* name; but if you prefer I can change it since I 
>> have
>> to do some modification to the patch.
> 
> I don't think it's reasonable to spell out there every little detail.
> You should also take adjacent code into consideration, making yours
> match. Issues only arise when there's bad code that you happen to
> look at.
> 
>>>> @@ -1285,14 +1286,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>>>> *SystemTable)
>>>>            efi_bs->FreePool(name.w);
>>>>        }
>>>> 
>>>> -        if ( !name.s )
>>>> -            blexit(L"No Dom0 kernel image specified.");
>>>> -
>>>>        efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>>>> 
>>>> -        option_str = split_string(name.s);
>>>> +#ifdef CONFIG_ARM
>>>> +        /* dom0less feature is supported only on ARM */
>>>> +        dom0less_found = check_dom0less_efi_boot(dir_handle);
>>>> +#endif
>>>> +
>>>> +        if ( !name.s && !dom0less_found )
>>> 
>>> Here you (properly ) use !name.s,
>> 
>> This is not my code, I just added && !dom0less
> 
> Correct, which is why this is fine.
> 
>>>> +            blexit(L"No Dom0 kernel image specified.");
>>>> +
>>>> +        if ( name.s != NULL )
>>> 
>>> Here you then mean to omit the "!= NULL" for consistency and brevity.
>> 
>> I usually check explicitely pointers with NULL, is it something to be 
>> avoided in Xen?
>> There are some industrial coding standards that says to avoid the use of ! 
>> operator
>> with pointers. Is it important here to do !name.s instead of the solution 
>> above?
> 
> As you can see from neighboring code, we prefer the shorter forms,
> for being easier/shorter to read.

Ok

> 
>>>> +            option_str = split_string(name.s);
>>>> 
>>>> -        if ( !read_section(loaded_image, L"kernel", &kernel, option_str) )
>>>> +        if ( (!read_section(loaded_image, L"kernel", &kernel, 
>>>> option_str)) &&
>>> 
>>> Stray parentheses.
>> 
>> Will fix
>> 
>>> 
>>>> +             (name.s != NULL) )
>>> 
>>> See above.
>> 
>> Will fix
>> 
>>> 
>>> I also don't think this logic is quite right: If you're dom0less,
>>> why would you want to look for an embedded Dom0 kernel image?
>> 
>> This is common code, that check is not from my patch.
> 
> It is you who is adding the name.s != NULL part, isn't it?

Ok I think the logic needs to be explained because also the name dom0less
Is a little bit misleading, I will explain below.

Short answer for now: if there is no dom0 kernel embedded in Xen and name.s is 
NULL,
do not try to load something or check dom0 command line because there is no 
dom0.

> 
>> Before this serie, EFI boot requires that a dom0 module was passed, otherwise
>> the boot was stopped.
>> 
>> This serie instead removes this requirement, letting the boot continue if 
>> there is no dom0
>> kernel.
>> 
>> However (as in the old code) if the user embed the dom0 kernel in the image, 
>> then it is
>> legit to use it and if there are also other domUs specified by DT, then the 
>> system will
>> start the dom0 kernel and the domUs kernel as well.
> 
> This doesn't match what I would expect - if configuration tells
> to boot dom0less, why would an embedded Dom0 kernel matter? I can
> see that views might differ here; you will want to write down
> somewhere what the intended behavior in such a case is.

I explain here my understanding on dom0less, this feature is used to start 
domUs at
Xen boot in parallel, the name is misleading but it doesn’t require dom0 to be 
absent.

So if you have a dom0 kernel embed in the image, it's completely fine to start 
it and it 
doesn’t have to be skipped.

Here the possible user cases:
1) start only dom0 [dom0 modules on xen.cfg or embedded in Xen image]
2) start only domUs, true dom0less [no dom0 modules on xen.cfg or embedded in 
Xen image, domUs on DT]
3) start dom0 and domUs [(dom0 modules on xen.cfg or embedded in Xen image) and 
domUs on DT]

Let me know your thoughts about that.

Cheers,

Luca


> 
> Jan
> 
> 




 


Rackspace

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