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

From: Jan Beulich <jbeulich@suse.com>
To: Luca Fancellu <luca.fancellu@arm.com>
Cc: Bertrand Marquis <bertrand.marquis@arm.com>,
	wei.chen@arm.com, Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Rahul Singh <rahul.singh@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL
Date: Thu, 8 Apr 2021 16:36:05 +0200	[thread overview]
Message-ID: <b05b29ce-263b-0526-61a6-679427305709@suse.com> (raw)
In-Reply-To: <4F221E20-0ABA-499D-A810-4A2A4F20F17D@arm.com>

On 08.04.2021 15:11, Luca Fancellu wrote:
> 
> 
>> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@suse.com> 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