From: Luca Fancellu <luca.fancellu@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: 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 11:51:51 +0100 [thread overview]
Message-ID: <6A929631-AA44-4ECF-AACB-16C0110393F6@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2109230943510.17979@sstabellini-ThinkPad-T480s>
> 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.
>
> 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.
Cheers,
Luca
next prev parent reply other threads:[~2021-09-24 10:52 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 [this message]
2021-09-24 20:45 ` Stefano Stabellini
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=6A929631-AA44-4ECF-AACB-16C0110393F6@arm.com \
--to=luca.fancellu@arm.com \
--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=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).