| Message ID | 20210408094818.8173-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Prevent Dom0 to be loaded when using dom0less | expand |
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
> 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. > >> --- 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
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
> On 8 Apr 2021, at 15:36, Jan Beulich <jbeulich@suse.com> wrote: > > 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. Perfect, I will change them back and add all the modification we discussed In the v3. Thank you for your feedback. Cheers, Luca > > Jan
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index b71b099e6f..b761d90c40 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -412,7 +412,7 @@ bool is_assignable_irq(unsigned int irq) */ bool irq_type_set_by_domain(const struct domain *d) { - return (d == hardware_domain); + return is_hardware_domain(d); } /* diff --git a/xen/common/domain.c b/xen/common/domain.c index d85984638a..e8ec3ba445 100644 --- 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); diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c index aef358d880..8b8e3a00ba 100644 --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c @@ -1168,7 +1168,7 @@ static int ipmmu_reassign_device(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if ( t && t != hardware_domain ) + if ( t && !is_hardware_domain(t) ) return -EPERM; if ( t == s ) diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c index 53d150cdb6..d115df7320 100644 --- a/xen/drivers/passthrough/arm/smmu-v3.c +++ b/xen/drivers/passthrough/arm/smmu-v3.c @@ -3366,7 +3366,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if (t && t != hardware_domain) + if ( t && !is_hardware_domain(t) ) return -EPERM; if (t == s) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 3e8aa37866..932fdfd6dd 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2670,7 +2670,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if (t && t != hardware_domain) + if ( t && !is_hardware_domain(t) ) return -EPERM; if (t == s) diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 1da90f207d..738bda5ef3 100644 --- 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)) struct vtimer { struct vcpu *v; diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 5485d08afb..bfc9d2577c 100644 --- 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 check is for functionality specific to a control domain */
The function is_hardware_domain() returns true if the hardware_domain and the passed domain is NULL, here we add a check to return false if there is no hardware_domain. Among the common and arm codebase there are few cases where the hardware_domain variable is checked to see if the current domain is equal to the hardware_domain, change this cases to use is_hardware_domain() function instead. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/irq.c | 2 +- xen/common/domain.c | 4 ++-- xen/drivers/passthrough/arm/ipmmu-vmsa.c | 2 +- xen/drivers/passthrough/arm/smmu-v3.c | 2 +- xen/drivers/passthrough/arm/smmu.c | 2 +- xen/include/asm-arm/domain.h | 2 +- xen/include/xen/sched.h | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-)