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

[Jan Beulich <jbeulich@xxxxxxxx>]

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