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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 8 Apr 2021, at 15:36, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

Perfect, I will change them back and add all the modification we discussed
In the v3.

Thank you for your feedback.

Cheers,
Luca

> 
> Jan