All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stefano Stabellini <sstabellini@kernel.org>
To: Luca Fancellu <luca.fancellu@arm.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	wei.chen@arm.com, "Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Jan Beulich" <jbeulich@suse.com>, "Wei Liu" <wl@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot
Date: Fri, 24 Sep 2021 13:45:10 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2109241334430.17979@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <6A929631-AA44-4ECF-AACB-16C0110393F6@arm.com>

[-- Attachment #1: Type: text/plain, Size: 8155 bytes --]

On Fri, 24 Sep 2021, Luca Fancellu wrote:
> > On 23 Sep 2021, at 17:59, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 23 Sep 2021, Luca Fancellu wrote:
> >>>> +/*
> >>>> + * Binaries will be translated into bootmodules, the maximum number for them is
> >>>> + * MAX_MODULES where we should remove a unit for Xen and one for Xen DTB
> >>>> + */
> >>>> +#define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> >>>> +static struct file __initdata dom0less_file;
> >>>> +static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> >>>> +static unsigned int __initdata dom0less_modules_available =
> >>>> +                               MAX_DOM0LESS_MODULES;
> >>>> +static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> This is a lot better!
> >>> 
> >>> We don't need both dom0less_modules_idx and dom0less_modules_available.
> >>> You can just do:
> >>> 
> >>> #define dom0less_modules_available (MAX_DOM0LESS_MODULES - dom0less_modules_idx)
> >>> static unsigned int __initdata dom0less_modules_idx;
> >>> 
> >>> But maybe we can even get rid of dom0less_modules_available entirely?
> >>> 
> >>> We can change the check at the beginning of allocate_dom0less_file to:
> >>> 
> >>> if ( dom0less_modules_idx == dom0less_modules_available )
> >>>   blexit
> >>> 
> >>> Would that work?
> >> 
> >> I thought about it but I think they need to stay, because dom0less_modules_available is the
> >> upper bound for the additional dom0less modules (it is decremented each time a dom0 module
> >> Is added), instead dom0less_modules_idx is the typical index for the array of dom0less modules.
> > 
> > [...]
> > 
> > 
> >>>> +    /*
> >>>> +     * 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");
> >>> 
> >>> This is the check that could be based on dom0less_modules_idx
> >>> 
> >> 
> >> The only way I see to have it based on dom0less_modules_idx will be to compare it
> >> to the amount of modules still available, that is not constant because it is dependent
> >> on how many dom0 modules are loaded, so still two variables needed.
> >> Don’t know if I’m missing something.
> > 
> > I think I understand where the confusion comes from. I am appending a
> > small patch to show what I had in mind. We are already accounting for
> > Xen and the DTB when declaring MAX_DOM0LESS_MODULES (MAX_MODULES - 2).
> > The other binaries are the Dom0 kernel and ramdisk, however, in my setup
> > they don't trigger a call to handle_dom0less_module_node because they
> > are compatible xen,linux-zimage and xen,linux-initrd.
> > 
> > However, the Dom0 kernel and ramdisk can be also compatible
> > multiboot,kernel and multiboot,ramdisk. If that is the case, then they
> > would indeed trigger a call to handle_dom0less_module_node.
> > 
> > I think that is not a good idea: a function called
> > handle_dom0less_module_node should only be called for dom0less modules
> > (domUs) and not dom0.

I can see that I misread the code yesterday: Dom0 modules don't go
through handle_dom0less_module_node thanks to the xen,domain check in
efi_arch_check_dom0less_boot.


> > But from the memory consumption point of view, it would be better
> > actually to catch dom0 modules too as you intended. In that case we need to:
> > 
> > - add a check for xen,linux-zimage and xen,linux-initrd in
> >  handle_dom0less_module_node also
> > 
> > - rename handle_dom0less_domain_node, handle_dom0less_module_node,
> >  dom0less_file, dom0less_modules, dom0less_modules_idx to something
> >  else more generic
> > 
> > 
> > For instance they could be called:
> > 
> > handle_domain_node
> > handle_module_node
> > module_file
> > modules
> > modules_idx
> > 
> > 
> > 
> > 
> > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > index e2b007ece0..812d0bd607 100644
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -22,8 +22,6 @@ typedef struct {
> > #define MAX_DOM0LESS_MODULES (MAX_MODULES - 2)
> > static struct file __initdata dom0less_file;
> > static dom0less_module_name __initdata dom0less_modules[MAX_DOM0LESS_MODULES];
> > -static unsigned int __initdata dom0less_modules_available =
> > -                               MAX_DOM0LESS_MODULES;
> > static unsigned int __initdata dom0less_modules_idx;
> > 
> > #define ERROR_DOM0LESS_FILE_NOT_FOUND (-1)
> > @@ -592,14 +590,6 @@ static void __init efi_arch_handle_module(const struct file *file,
> >          * stop here.
> >          */
> >         blexit(L"Unknown module type");
> > -
> > -    /*
> > -     * dom0less_modules_available is decremented here because for each dom0
> > -     * file added, there will be an additional bootmodule, so the number
> > -     * of dom0less module files will be decremented because there is
> > -     * a maximum amount of bootmodules that can be loaded.
> > -     */
> > -    dom0less_modules_available--;
> > }
> > 
> > /*
> > @@ -643,7 +633,7 @@ static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> >      * dom0less_modules_available is updated each time we use read_file(...)
> >      * successfully.
> >      */
> > -    if ( !dom0less_modules_available )
> > +    if ( dom0less_modules_idx == MAX_DOM0LESS_MODULES )
> >         blexit(L"No space left for domU modules");
> > 
> >     module_name.s = (char*) name;
> 
> Hi Stefano,
> 
> Yes I understand what you mean, unfortunately it can’t be done, I will explain why in details
> because the UEFI code is very tricky in the way it was written.
> 
> Dom0 modules and xsm-policy are handled in boot.c by read_section(…) or read_file(…) and
> both call handle_file_info(…) that inside calls efi_arch_handle_module(…).
> Dom0less modules (xen,domain child modules) are handled in efi-boot.h by handle_dom0less_module_node(…)
> that may call allocate_dom0less_file(…) and (to reuse code) calls read_file(…).
> 
> So there can be maximum (MAX_MODULES-2) boot modules because at start in start_xen(…)
> we add Xen and the DTB as boot modules.
> 
> This amount is to be shared among dom0 modules (kernel, ramdisk), xsm-policy and domU
> modules (kernel, ramdisk, dtb).
> 
> The thing is that we don’t know how many dom0 modules will be specified and also for the xsm-policy.
> 
> In your code example, let’s say I have MAX_DOM0LESS_MODULES=(32-2) so 30 modules available,
> If I declare a Dom0 kernel and 30 DomU kernels, it will pass the check, but on boot I think it will ignore
> the exceeding modules.
> 
> For that reason we need to keep track of how many “slots” are available, so in my code every time the
> read_file(…)/read_section(…) is called, the dom0less_modules_available is decremented.
> 
> If we want to get rid of one variable, we can just assume the dom0 modules and xsm-policy will be always
> loaded and we can set MAX_DOM0LESS_MODULES=(MAX_MODULES - 2 - 3) where 3 is dom0 kernel,
> dom0 ramdisk and xsm-policy. If the user doesn’t load any of them, we just lost 3 slots.
> 
> Another solution can be keeping just the dom0less_modules_available and every time loop backward in the array
> starting from that position and stopping when we have a dom0less_module_name.name == NULL so we
> know we have an available slot or we reach below zero and we know there is no space. But I think it’s not
> worthy.
> 
> So if you really want to have only one variable, I will remove space from MAX_DOM0LESS_MODULES and
> I will push it in v3.
 
You are right, let's just keep dom0less_modules_available, thanks for
the explanation.

One suggestion for future improvement (this patch series is OK as is)
would be to extend get_dom0less_file_index to also be able to search for
the dom0 kernel and ramdisk modules so that if a dom0less kernel or
ramdisk has the same filename as the dom0 kernel or ramdisk, the file
doesn't need to be loaded twice. It could be a follow-up patch after
this series gets committed.

  reply	other threads:[~2021-09-24 20:45 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 14:13 [PATCH v2 0/2] arm/efi: Add dom0less support to UEFI boot Luca Fancellu
2021-09-22 14:13 ` [PATCH v2 1/2] arm/efi: Introduce uefi,cfg-load DT property Luca Fancellu
2021-09-22 21:19   ` Stefano Stabellini
2021-09-23 10:42     ` Luca Fancellu
2021-09-22 14:13 ` [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot Luca Fancellu
2021-09-22 21:51   ` Stefano Stabellini
2021-09-23 14:08     ` Luca Fancellu
2021-09-23 16:59       ` Stefano Stabellini
2021-09-24 10:51         ` Luca Fancellu
2021-09-24 20:45           ` Stefano Stabellini [this message]
2021-09-24 14:02   ` Jan Beulich
2021-09-24 15:28     ` Luca Fancellu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2109241334430.17979@sstabellini-ThinkPad-T480s \
    --to=sstabellini@kernel.org \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --cc=luca.fancellu@arm.com \
    --cc=roger.pau@citrix.com \
    --cc=wei.chen@arm.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.