Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2021 15:11, Luca Fancellu wrote:
> 
> 
>> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 08.04.2021 11:48, Luca Fancellu wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>>>     struct domain *dom0;
>>>     int rv;
>>>
>>> -    if ( d != hardware_domain || d->domain_id == 0 )
>>> +    if ( !is_hardware_domain(d) || d->domain_id == 0 )
>>>         return 0;
>>>
>>>     rv = xsm_init_hardware_domain(XSM_HOOK, d);
>>> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>>>     err = err ?: -EILSEQ; /* Release build safety. */
>>>
>>>     d->is_dying = DOMDYING_dead;
>>> -    if ( hardware_domain == d )
>>> +    if ( is_hardware_domain(d) )
>>>         hardware_domain = old_hwdom;
>>>     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>>
>> While these may seem like open-coding of is_hardware_domain(), I
>> think it would be better to leave them alone. In neither of the two
>> cases is it possible for d to be NULL afaics, and hence your
>> addition to is_hardware_domain() doesn't matter here.
> 
> Yes that is right, the only thing is that we have a nice function
> “Is_hardware_domain” and we and up comparing “manually”.
> It looks weird to me, but I can change it back if you don’t agree.

Well, from the time when late-hwdom was introduced I seem to vaguely
recall that the way it's done was on purpose. It pretty certainly was
also at that time when is_hardware_domain() (or whatever predecessor
predicate) was introduced, which suggests to me that if the above
were meant to use it, they would have been switched at the same time.

Jan