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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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. 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.
No worries if you don't have it handy, I was just curious.


> > 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.

Nice work of archaeology there!


> > 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.

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'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.

OK