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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan