Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Alejandro Vallejo <agarciav@amd.com>]

On Wed May 14, 2025 at 12:23 AM CEST, Daniel P. Smith wrote:
> On 5/13/25 04:05, Jan Beulich wrote:
>> On 06.05.2025 21:29, Daniel P. Smith wrote:
>>> On 5/2/25 03:21, Jan Beulich wrote:
>>>> On 30.04.2025 20:56, Daniel P. Smith wrote:
>>>>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>>>>> --- a/xen/common/Makefile
>>>>>> +++ b/xen/common/Makefile
>>>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>>>>     obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>>>>     obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>>>>     obj-y += domain.o
>>>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>>>>
>>>>> Please don't do this, use IF_ENABLED in core.c and then hide the
>>>>> unnecessary units in domain-builder/Makefile as I originally had it.
>>>>> This allows for a much easier time incrementally converting the dom0
>>>>> construction path into a generalized domain construction path.
>>>>
>>>> That is, are you viewing this as a transitional thing only? If the end
>>>> goal is to have it as Alejandro has it above, that may be acceptable
>>>> (even if not nice).
>>>
>>> There is/will be shared domain construction code between dom0-only and
>>> multidomain construction, with it will all living under domain builder.
>>> So no, the end goal is not what Alejandro just did. The end result will
>>> be the way I had it, with a different kconfig option to enable/disable
>>> the multidomain construction path.
>> 
>> Isn't CONFIG_DOMAIN_BUILDER a misnomer then?
>
> Which is why I originally had two kconfig flags, one to enable dtb 
> parsing for domain configuration, which allowed dom0 construction from 
> dtb. Then there was the second flag that was to enable multi-domain 
> construction. I have reworked a portion of the approach in the RFC based 
> on feedback, which is based off of this series. In it, I tried to 
> minimize how much went into common, but you will see how I still had to 
> rework the kconfig flags.
>
> v/r,
> dps

Does it really make sense to have a flag to restrict multidomain while
allowing parsing the DTB? There's virtually nothing compiled out in that
case.

If you did it that way because it doesn't initially build several
domains, that's just transitional and to be expected on any feature
tagged as unsupported with (EXPERIMENTAL) in the name.

What if I collapse everything under a single CONFIG_MULTIDOMAIN_BUILDER
that compiles-in support for parsing DTBs while introducing an
unconditional builder as I go? From what I'm seeing, there are no
breaking changes in the series and the end goal is to have the builder
be unconditionally used, after all.

In fact, with the bindings code in common, I can also collapse everything
in core.c (and later domain.c) into a single arch/x86/domain-builder.c
that is unconditionally compiled in. The DTB parsing logic is already 
in separate files and that can be compiled out with
CONFIG_MULTIDOMAIN_BUILDER flag.

In retrospect, after looking at dom0less long enough there's bootfdt.c and
bootinfo.c with similar intent, but far more ad-hoc cohesion. While the
builder wants to be in common, no other arch is in a position to take
it. It needs merging with the stuff done in bootfdt.c/bootinfo.c

So, in short. I'm planning to:

  1. Collapse domain-builder/{core,domain}.c into domain-builder.c
     under arch/x86. There's little reason to have them separate.
  2. Remove CONFIG_DOMAIN_BUILDER, and replace it with something that
     reflects the intent of using a DTB. Either CONFIG_MULTIDOMAIN_BUILDER
     or CONFIG_DTB_BUILDER. Or maybe even CONFIG_DTB_BOOT.
      * I specifically want to avoid CONFIG_BOOTFDT, because that'd
        create confusion with the already existing bootfdt.c in common.

and, from the discussion in the other thread about code sharing in the
spirit of getting somewhere soon:

  3. Do minimal parsing at builder_init() (module identification,
  basically), and do the full parsing by the create_dom0() mark,
  immediately before constructing all domains. With the unflattened
  tree.

Moving forward I for sure want to merge the boot paths in the x86
builder with those of arm/riscv. Having a unified boot frontend can only
bring niceness.

Cheers,
Alejandro