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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 03/11/2021 21:57, Stefano Stabellini wrote:
> On Wed, 3 Nov 2021, Julien Grall wrote:
> > 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.
> They tend to be "exceptions". Node names are usually not meaningful
> except for few top-level nodes without a compatible string. 
I think you misundertood my point here. I wasn't necessarily saying that 
the name "modules" was meaningful. Instead, I was pointing out there was 
various nodes without compatible property. I can see how this is useful 
to group nodes.
In fact, I couldn't find a section in the Device-Tree suggesting that 
the compatible property was mandatory. Do you have one pointer in hand?
> Cpus, memory
> and chosen are all top level nodes. I don't know about "soc", that one
> should probably be compatible = "simple-bus". If you have a pointer to
> an "soc" node without a compatible I'd be interested in taking a look.
See above, I wasn't suggesting that the name "soc" is meaningful.

> No worries if you don't have it handy, I was just curious.
Nothing in hand sorry. I vaguely recall we had that discussion when 
introducing the partial device-tree a few years ago.
> > > 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).
> It would be good to clarify. If we decide to go with making it clear
> that "modules" is not supported then from a device tree point of view I
> think we should say that "multiboot,module" nodes for Dom0 and Xen (xsm)
> are children of /chosen. I prefer this option because I think that if
> we wanted to group the dom0 and/or Xen modules together (which could be
> good) we could come up with something better than "modules", more
> aligned with dom0less.
To expand what I wrote above, I viewed "modules" as just a way to group 
nodes rather than a meaningful name.
In the current implementation in Xen doesn't care of the name. It just 
looks for any node in chosen up to depth 3. So anyone could create a 
node "bar" to group everything together.
> Otherwise we could try to add a "modules" node to the description with a
> compatible string and a comment saying certain legacy versions might not
> have a compatible string.
I am not really in favor of introducing a new compatible because it will 
never be used by Xen (or anyone else).
So if the compatible is mandatory, then I would prefer to deprecate the 
use in the next release (we could add a warning).
Cheers,

--
Julien Grall