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

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

[Luca Fancellu]

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:

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?

Thank you.

Cheers,
Luca

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

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:
> 
> 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, it is not described in
arm/device-tree/booting.txt, and it is only rarely used.

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.

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'd just update the wikipage.

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

[Julien Grall]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5710 bytes --]

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

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

[Julien Grall]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8765 bytes --]

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