Re: [PATCH v2 09/21] xen/arm: move cmdline out of boot_modules

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Stefano Stabellini <stefanos@xilinx.com>,
	nd@arm.com, andrii_anisov@epam.com, xen-devel@lists.xen.org
Subject: Re: [PATCH v2 09/21] xen/arm: move cmdline out of boot_modules
Date: Tue, 10 Jul 2018 22:11:11 +0100	[thread overview]
Message-ID: <7dcbcd52-2178-e9a8-a99d-b5650e13d37a@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1807091612430.8023@sstabellini-ThinkPad-X260>

Hi Stefano,

On 10/07/2018 01:00, Stefano Stabellini wrote:
> On Mon, 9 Jul 2018, Julien Grall wrote:
>> Hi,
>>
>> On 07/07/18 00:12, Stefano Stabellini wrote:
>>> Remove the cmdline field from struct boot_module, cmdline is stored
>>> independently out of the boot_modules array as dom0_cmdline.
>>
>> I am not entirely convince of this patch, this does not seem to go towards a
>> better code base because dom0_cmdline is only set if "bootargs". This may
>> raise some confusing to the developer.
> 
> I'll add a comment on top of dom0_cmdline to clarify its purpose:
> 
> /*
>   * Dom0 command line as passed via Device Tree as "bootargs" for the
>   * Dom0 kernel module.
>   */
> 
> 
>> I would still prefer to keep the command-line in the boot module structure and
>> find a way to associated the bootmodule with a node.
> 
> How do you suggest we find a good way to associate a boot module with a
> node?
> 
> I have thought about this problem quite a bit. Although I admit this
> patch is not super nice, it is the best option I found. I have actually
> developed 2 other completely different implementations of this fix and
> they were all worse than this. This is the third incarnation. It is
> actually surprisingly easy to do worse than this patch.

I actually have an idea. Looking at the Device-Tree specification [1], a 
path should always be unique. This means that for a given path a/b/c/d, 
d will always be unique.

Reading the binding you introduced, the multiboot node will be a child 
of a node with the compatible "xen,domain". As the name of the parent 
node will be unique, you can tag the boot-module with that name. No need 
for the full path as all the configuration node should follow the 
pattern /chosen/<domain-name>.

When the guest is built, you have the Device-Tree path of the 
configuration in hand. From that you can deduce the name that you could 
re-use to find the correct boot-module.

You may still require a separate handling similar to your next patch of 
Dom0 as with the current wording they could be a level deeper. But we 
can tight the wording for the new bindings (though, I think it is tight 
enough).

What do you think?

Now regarding the current implementation, it does not follow the defined 
specification. The multiboot nodes are looked everywhere in the DT 
rather than only /chosen. This is an implementation bug and should be fixed.

Cheers,

[1] https://www.devicetree.org/specifications/

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel