Hi Luca and Stefano,
On 02/11/2021 23:36, Stefano Stabellini wrote:
On Tue, 2 Nov 2021, Luca Fancellu wrote:
Hi all,
We recently discovered that there is a way to list Dom0 modules that is not
supported by the EFI boot,
It’s happened browsing some Wiki pages like this one:
https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Lager
In that page the Dom0 modules are listed inside a subnode of the /chosen node:
chosen {
modules {
#address-cells = <1>;
#size-cells = <1>;
module@0x72000000 {
compatible = "multiboot,kernel", "multiboot,module";
reg = <0x72000000 0x2fd158>;
};
module@0x74000000 {
compatible = "xen,xsm-policy", "multiboot,module";
reg = <0x74000000 0x2559>;
};
};
};
Instead for how it is implemented now in the EFI code and described in:
1) https://xenbits.xen.org/docs/unstable/misc/arm/device-tree/booting.txt
2) https://xenbits.xen.org/docs/unstable/misc/efi.html
Only the following approach is supported, so Dom0 modules must be a direct
child of /chosen:
Do you mean this is not supported after your changes or this was never
supported? (see more below).
chosen {
#address-cells = <1>;
#size-cells = <1>;
module@0x72000000 {
compatible = "multiboot,kernel", "multiboot,module";
reg = <0x72000000 0x2fd158>;
};
module@0x74000000 {
compatible = "xen,xsm-policy", "multiboot,module";
reg = <0x74000000 0x2559>;
};
};
Is this a problem that needs a fix?
Let me start by saying that I don't feel strongly either way, so I am
happy to go with other people's opinion on this one.
In this kind of situations I usually look at two things:
- what the specification says
- what the existing code actually does
In general, I try to follow the specification unless obviously
production code relies on something that contradicts the spec, in which
case I'd say to update the spec.
In this case, although it is true that "modules" could be nice to have,
it is missing a compatible string,
There are a few nodes in the DT without compatible (e.g. cpus, memory,
chosen, soc). So I am a bit confused why this is a problem.
it is not described in arm/device-tree/booting.txt,
Up until Xen 4.4, we had the following sentence:
"
Each node has the form /chosen/modules/module@<N> and contains the
following properties:
"
This was removed by commit af82a77f3abc "xen: arm: remove innaccurate
statement about multiboot module path". But, IMHO, the new wording still
doesn't explicit says the module should be directly in /chosen.
and it is only rarely used.
Hmmm... We have quite a few examples on the wiki that create 'module'
under 'modules'. In fact, we have provided U-boot script [2] that can be
easily re-used. So I would not call it rare.
For these reasons, I don't think it is a problem that we need to fix.
Especially considering that the EFI case is the only case not working
and it was never supported until now.
Hmmm... Looking at the implementation of efi_arch_use_config_file() in
4.12, we are looking for the compatible "mutiboot,module". So I would
say this is supported.
If we want to add support for "modules", that could be fine, but I think
we should describe it in arm/device-tree/booting.txt and also add a
compatible string for it. For 4.16
I think the first question we need to resolved is whether this has ever
been supported in EFI. I think it was and therefore this is technically
a regression.
That said, outside of the dom0less case, I don't expect any UEFI users
will bother to create the nodes manually and instead rely on GRUB to
create them. So I think breaking it would be OK.
I am less convinced about breaking it for non-UEFI case. That said, I
think the documentation should be updated either way for 4.16 (the more
if this has been broken as part of recent changes).
I'd just update the wikipage.
There are quite a few places to update on the wiki page. AFAICT, they
are all related to U-boot, so I don't think there is an action needed here.
Cheers,
[1]
[2] https://xenbits.xen.org/people/julieng/load-xen-tftp.scr.txt
--
Julien Grall
