Re: [RFC 1/6] dom0: replace explict zero checks

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/1/23 20:24, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> > A legacy concept is that the initial domain will have a domain id of zero. As a
> > result there are places where a check that a domain is the inital domain is
> > determined by an explicit check that the domid is zero.
> >
> > This commit seeks to abstract this check into a function call and replace all
> > check locations with the function call.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Thanks for the patch, this is a good cleanup!
Thank you for the sentiment as well as giving it a review!

> > ---
> >   xen/common/domain.c         | 4 ++--
> >   xen/common/sched/arinc653.c | 2 +-
> >   xen/common/sched/core.c     | 4 ++--
> >   xen/include/xen/sched.h     | 7 +++++++
> >   4 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 304aa04fa6..8fb3c052f5 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -309,7 +309,7 @@ static int late_hwdom_init(struct domain *d)
> >       struct domain *dom0;
> >       int rv;
> >   
> > -    if ( d != hardware_domain || d->domain_id == 0 )
> > +    if ( d != hardware_domain || is_initial_domain(d) )
> >           return 0;
> >   
> >       rv = xsm_init_hardware_domain(XSM_HOOK, d);
> > @@ -612,7 +612,7 @@ struct domain *domain_create(domid_t domid,
> >       d->is_privileged = flags & CDF_privileged;
> >   
> >       /* Sort out our idea of is_hardware_domain(). */
> > -    if ( domid == 0 || domid == hardware_domid )
> > +    if ( is_initial_domain(d) || domid == hardware_domid )
> >       {
> >           if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
> >               panic("The value of hardware_dom must be a valid domain ID\n");
> > diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
> > index a82c0d7314..31e8270af3 100644
> > --- a/xen/common/sched/arinc653.c
> > +++ b/xen/common/sched/arinc653.c
> > @@ -404,7 +404,7 @@ a653sched_alloc_udata(const struct scheduler *ops, struct 
> > sched_unit *unit,
> >        * Add every one of dom0's units to the schedule, as long as there are
> >        * slots available.
> >        */
> > -    if ( unit->domain->domain_id == 0 )
> > +    if ( is_initial_domain(unit->domain) )
> >       {
> >           entry = sched_priv->num_schedule_entries;
> >   
> > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > index 022f548652..210ad30f94 100644
> > --- a/xen/common/sched/core.c
> > +++ b/xen/common/sched/core.c
> > @@ -585,7 +585,7 @@ int sched_init_vcpu(struct vcpu *v)
> >            */
> >           sched_set_affinity(unit, cpumask_of(0), cpumask_of(0));
> >       }
> > -    else if ( d->domain_id == 0 && opt_dom0_vcpus_pin )
> > +    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
> >       {
> >           /*
> >            * If dom0_vcpus_pin is specified, dom0 vCPUs are pinned 1:1 to
> > @@ -594,7 +594,7 @@ int sched_init_vcpu(struct vcpu *v)
> >           sched_set_affinity(unit, cpumask_of(processor), &cpumask_all);
> >       }
> >   #ifdef CONFIG_X86
> > -    else if ( d->domain_id == 0 )
> > +    else if ( is_initial_domain(d) )
> >       {
> >           /*
> >            * In absence of dom0_vcpus_pin instead, the hard and soft affinity of
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index 854f3e32c0..a9276a7bed 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
> >   void watchdog_domain_init(struct domain *d);
> >   void watchdog_domain_destroy(struct domain *d);
> >   
> > +static always_inline bool is_initial_domain(const struct domain *d)
> I know you used always_inline only because is_hardware_domain just below
> also uses it, but I wonder if we need it.
I was under the impression that access checks like these could end up in 
fast paths, so we wanted to coax the compiler into inlining these 
whenever possible. If others feel it is not needed, I have no objection 
to moving it to just inline.
> Also, Robero, it looks like always_inline is missing from
> docs/misra/C-language-toolchain.rst? Or do we plan to get rid of it?
>
>
> > +{
> > +    static int init_domain_id = 0;
> I take it is done this way because you plan to make it configurable?
As you see in a later patch, it gets changed into an assignable role for 
the domain.
> > +    return d->domain_id == init_domain_id;
> > +}
> > +
> >   /*
> >    * Use this check when the following are both true:
> >    *  - Using this feature or interface requires full access to the hardware
> > --
> > 2.20.1