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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> 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.

> 
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -30,7 +30,7 @@ enum domain_type {
>> #endif
>> 
>> /* The hardware domain has always its memory direct mapped. */
>> -#define is_domain_direct_mapped(d) ((d) == hardware_domain)
>> +#define is_domain_direct_mapped(d) (is_hardware_domain(d))
> 
> Nit: If this was code I'm a maintainer of, I'd ask for the unneeded
> parentheses to be dropped.

Sure I can do that on the next version of the patch

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1022,7 +1022,7 @@ static always_inline bool is_hardware_domain(const 
>> struct domain *d)
>>     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>         return false;
>> 
>> -    return evaluate_nospec(d == hardware_domain);
>> +    return evaluate_nospec((hardware_domain != NULL) && (d == 
>> hardware_domain));
>> }
> 
> This would be the first instance in the tree of an && expression
> inside evaluate_nospec(). I think the generated code will still be
> okay, but I wonder whether this is really needed. Can you point
> out code paths where d may actually be NULL, and where
> 
> static always_inline bool is_hardware_domain(const struct domain *d)
> {
>    if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>        return false;
> 
>    if ( !d )
>        return false;
> 
>    return evaluate_nospec(d == hardware_domain);
> }
> 
> would not behave as intended (i.e. where bad speculation would
> result)? (In any event I think checking d against NULL is preferable
> over checking hardware_domain.)

I agree with you, I will change the code checking if d is NULL the
way it’s written above

Cheers,
Luca

> 
> Jan