Re: [PATCH v5 03/10] x86: Replace arch-specific boot_domain with the common one

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Jul 3, 2025 at 8:04 AM CEST, Jan Beulich wrote:
> On 02.07.2025 17:34, Alejandro Vallejo wrote:
>> On Wed Jul 2, 2025 at 5:15 PM CEST, Jan Beulich wrote:
>>> On 02.07.2025 17:09, Alejandro Vallejo wrote:
>>>> On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
>>>>> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>>>>>> --- a/xen/arch/x86/include/asm/bootfdt.h
>>>>>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>>>>>> @@ -3,6 +3,12 @@
>>>>>>  #define X86_BOOTFDT_H
>>>>>>  
>>>>>>  #include <xen/types.h>
>>>>>> +#include <public/xen.h>
>>>>>> +
>>>>>> +struct arch_boot_domain
>>>>>> +{
>>>>>> +    domid_t domid;
>>>>>> +};
>>>>>>  
>>>>>>  struct arch_boot_module
>>>>>>  {
>>>>>> [...]
>>>>>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct 
>>>>>> boot_info *bi)
>>>>>>          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>>>>>  
>>>>>>      /* Create initial domain.  Not d0 for pvshim. */
>>>>>> -    bd->domid = get_initial_domain_id();
>>>>>> -    d = domain_create(bd->domid, &dom0_cfg,
>>>>>> +    bd->arch.domid = get_initial_domain_id();
>>>>>> +    d = domain_create(bd->arch.domid, &dom0_cfg,
>>>>>>                        pv_shim ? 0 : CDF_privileged | CDF_hardware);
>>>>>>      if ( IS_ERR(d) )
>>>>>> -        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>>>>> +        panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>>>>>
>>>>> This being the only place where the (now) arch-specific field is used, why
>>>>> does it exist? A local variable would do? And if it's needed for
>>>>> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
>>>>> arch-specific? Daniel, Jason?
>>>>
>>>> As for the arch-agnostic side of things, arm needs some extra work to be
>>>> able to do it safely. dom0less currently constructs domains immediately 
>>>> after
>>>> parsing them, which is problematic for cases where some domains have the 
>>>> prop
>>>> and others don't. The domid allocation strategy may preclude further 
>>>> otherwise
>>>> good domains from being created just because their domid was stolen by a 
>>>> domain
>>>> that didn't actually care about which domid it got.
>>>>
>>>> It'll eventually want to leave the arch-specific area, but I don't want to 
>>>> do
>>>> that work now.
>>>
>>> But if the domU field is fine to live in a common struct despite being 
>>> unused
>>> on x86, why can't the domid field live in a common struct too, despite being
>>> unused on non-x86? Otherwise it'll be extra churn for no gain to later move 
>>> it
>>> there.
>> 
>> Mostly out of tidiness. Otherwise it's hard to know which fields serve a 
>> purpose
>> where.
>> 
>> I genuinely forgot about the domU field. I'm more than happy to drop that 
>> arch
>> subfield and have domid in the main body of the struct, but I suspect MISRA
>> would have something to say about dead data?
>
> In principle yes (and then also about the domU field), but we rejected the
> respective rule altogether (for now? plus for a reason that I must have forgot
> and that escapes me).
>
> Jan

Actually, moving it to an arch-specific field is rather annoying. everyone but
x86 needs the field. I'll just compile it out for x86 specifically with ifdef
guards, even if it is common code.

For the record, I hope to get rid of it on arm/riscv/ppc entirely later on by
deducing domU vs dom0 from the capabilities property.

Cheers,
Alejandro