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

Re: Arm EFI boot issue for Dom0 module listed inside subnode of chosen



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



 


Rackspace

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