Re: [PATCH v3 1/7] xen: introduce hardware domain create flag

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2025 20:16, Jason Andryuk wrote:
> On 2025-04-04 03:38, Jan Beulich wrote:
>> On 03.04.2025 23:46, Jason Andryuk wrote:
>>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>
>>> Add and use a new internal create domain flag to specify the hardware
>>> domain.  This removes the hardcoding of domid 0 as the hardware domain.
>>>
>>> This allows more flexibility with domain creation.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> v3:
>>> Or-in CDF_hardware for late hwdom case
>>
>> Except that ...
>>
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -820,13 +820,18 @@ struct domain *domain_create(domid_t domid,
>>>       d->is_privileged = flags & CDF_privileged;
>>>   
>>>       /* Sort out our idea of is_hardware_domain(). */
>>> -    if ( domid == 0 || domid == hardware_domid )
>>> +    if ( (flags & CDF_hardware) || domid == hardware_domid )
>>>       {
>>>           if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED 
>>> )
>>>               panic("The value of hardware_dom must be a valid domain 
>>> ID\n");
>>>   
>>> +        /* late_hwdom is only allowed for dom0. */
>>> +        if ( hardware_domain && hardware_domain->domain_id )
>>> +            return ERR_PTR(-EINVAL);
>>> +
>>>           old_hwdom = hardware_domain;
>>>           hardware_domain = d;
>>> +        flags |= CDF_hardware;
>>>       }
>>
>> ... this isn't quite enough. You're only modifying what will go out of scope
>> when returning from the function. What's at least equally important to OR 
>> into
>> is d->cdf.
> 
> Yes, thanks for catching that.
> 
> I'll move d->cdf assignment to after here instead of or-ing in a second 
> time.

Not sure that'll be good to do - intermediate code (in particular passing
d to other functions) may need to have that set already. And even if not
now, then maybe going forward.

> With that, it seems like it should also be removed from old_hwdom?
> 
> old_hwdom->cdf &= ~CDF_hardware

Oh, indeed. Thanks in turn for catching this further aspect.

Jan