| Message ID | 20230801202006.20322-2-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyperlaunch domain roles and capabilities | expand |
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@apertussolutions.com> Thanks for the patch, this is a good cleanup! > --- > 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. 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? > + 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 >
On 02.08.2023 02:24, Stefano Stabellini wrote: > On Tue, 1 Aug 2023, Daniel P. Smith wrote: >> --- 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. > > 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? Under "Non-standard tokens" we have both __inline__ and __attribute__ listed, which I think is enough to cover this specific case as well? Jan
On 01.08.2023 22:20, 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. It might help if you at least outlined here why/how this is going to change. > --- 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) > +{ > + static int init_domain_id = 0; This may then also help with the question on why you use a static variable here. (In any event the type of this variable wants to be correct; plain int isn't appropriate ... > + return d->domain_id == init_domain_id; ... for this comparison.) Jan
On Wed, 2 Aug 2023, Jan Beulich wrote: > On 02.08.2023 02:24, Stefano Stabellini wrote: > > On Tue, 1 Aug 2023, Daniel P. Smith wrote: > >> --- 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. > > > > 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? > > Under "Non-standard tokens" we have both __inline__ and __attribute__ > listed, which I think is enough to cover this specific case as well? I think we should add always_inline explicitely to docs/misra/C-language-toolchain.rst to avoid any doubts about it
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@apertussolutions.com> > > 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 >>
On 8/2/23 03:46, Jan Beulich wrote: > On 01.08.2023 22:20, 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. > > It might help if you at least outlined here why/how this is going to > change. Okay, I will try expanding on this further. >> --- 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) >> +{ >> + static int init_domain_id = 0; > > This may then also help with the question on why you use a static > variable here. (In any event the type of this variable wants to > be correct; plain int isn't appropriate ... Ah, so this is a dated patch that I brought because of the abstraction it made. The intent in the original series for making it static was in preparation to handle the shim case where init_domid() would have return a non-zero value. So the static can be dropped and changed to domid_t. >> + return d->domain_id == init_domain_id; > > ... for this comparison.) > > Jan v/r, dps
Hi Daniel, On 03/08/2023 14:33, Daniel P. Smith wrote: > On 8/2/23 03:46, Jan Beulich wrote: >> On 01.08.2023 22:20, 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. >> >> It might help if you at least outlined here why/how this is going to >> change. > > Okay, I will try expanding on this further. > >>> --- 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) >>> +{ >>> + static int init_domain_id = 0; >> >> This may then also help with the question on why you use a static >> variable here. (In any event the type of this variable wants to >> be correct; plain int isn't appropriate ... > > Ah, so this is a dated patch that I brought because of the abstraction > it made. The intent in the original series for making it static was in > preparation to handle the shim case where init_domid() would have return > a non-zero value. > > So the static can be dropped and changed to domid_t. Looking at one of the follow-up patch, I see: static always_inline bool is_initial_domain(const struct domain *d) { - static int init_domain_id = 0; - - return d->domain_id == init_domain_id; + return d->role & ROLE_UNBOUNDED_DOMAIN; } So is there any point to have the local variable? IOW, can't this simply be "d->domain_id == 0"? Cheers,
On 8/3/23 09:36, Julien Grall wrote: > Hi Daniel, Hey Julien, > On 03/08/2023 14:33, Daniel P. Smith wrote: >> On 8/2/23 03:46, Jan Beulich wrote: >>> On 01.08.2023 22:20, 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. >>> >>> It might help if you at least outlined here why/how this is going to >>> change. >> >> Okay, I will try expanding on this further. >> >>>> --- 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) >>>> +{ >>>> + static int init_domain_id = 0; >>> >>> This may then also help with the question on why you use a static >>> variable here. (In any event the type of this variable wants to >>> be correct; plain int isn't appropriate ... >> >> Ah, so this is a dated patch that I brought because of the abstraction >> it made. The intent in the original series for making it static was in >> preparation to handle the shim case where init_domid() would have >> return a non-zero value. >> >> So the static can be dropped and changed to domid_t. > > Looking at one of the follow-up patch, I see: > > static always_inline bool is_initial_domain(const struct domain *d) > { > - static int init_domain_id = 0; > - > - return d->domain_id == init_domain_id; > + return d->role & ROLE_UNBOUNDED_DOMAIN; > } > > So is there any point to have the local variable? IOW, can't this simply > be "d->domain_id == 0"? Nope, you are right. All need for it drops with the static gone. v/r, dps
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) +{ + static int init_domain_id = 0; + + 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
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@apertussolutions.com> --- 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(-)