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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 5 Nov 2021, Julien Grall wrote:
> 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?

I'll answer below to make the conversation easier to follow


> > 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.
 
First let me clarify that the specification is not mandating that the
module nodes are at a specific depth, from that point of view it is OK
to have "intermediate" nodes. The issue is that "modules" is not
described and doesn't have a compatible string, so we don't know how we
should deal with it.

Although in the device tree specification [1] is not written in clear
letters that a compatible string is mandatory, it written that the
compatible string is necessary for two things:

- identify the driver to use for a device
- specify the "Device Bindings" [2]

The device bindings are the "requirements for how specific types and
classes of devices are represented in the devicetree". Basically, it is
what tells us how to represent and what to do with a given node.

Let's say that we want to group together a bunch of device nodes. We use
a bus node like "amba" to do it. If we don't specify compatible =
"simple-bus" we don't know how to map the addresses of the children
nodes into the parent address space, so the device nodes under "amba"
become actually unusable.

Chosen is for configurations, not for the description of devices.
However, similar rules apply: if we encounter a "modules" node, what are
we going to do with it? Is it OK to proceed and parse the children nodes
as normal? We don't know for sure. Imagine that instead of "modules" one
calls it "disabled" instead. What do we do in that case?

In short, we need a description of "modules" to know what to do with it.
To describe "modules", we need a binding. To have a binding, we need a
compatible string.

[1] https://github.com/devicetree-org/devicetree-specification
[2] 
https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter4-device-bindings.rst
 

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

That's fine by me