| Message ID | 20210414091404.14215-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Prevent Dom0 to be loaded when using dom0less | expand |
Hi Luca, > On 14 Apr 2021, at 10:14, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > 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> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers, Bertrand > --- > v4 changes: > - removed unneeded check for domain NULL from is_hardware_domain > introduced in v3 > v3 changes: > - removed unneeded parenthesis for macro is_domain_direct_mapped > - is_hardware_domain() checks for the passed domain and if it is > NULL, it returns false. > - reverted back checks in the function late_hwdom_init > --- > xen/arch/arm/irq.c | 2 +- > 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 +- > 5 files changed, 5 insertions(+), 5 deletions(-) > > 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/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..0a74df9931 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; > -- > 2.17.1 >
Hi Luca, On 14/04/2021 10:14, Luca Fancellu wrote: > 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> > --- > v4 changes: > - removed unneeded check for domain NULL from is_hardware_domain > introduced in v3 After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. I am not against the change, however I think the commit message needs to updated to match what the patch is doing. Can you propose a new commit message? Cheers,
> On 14 Apr 2021, at 12:16, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 14/04/2021 10:14, Luca Fancellu wrote: >> 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> >> --- >> v4 changes: >> - removed unneeded check for domain NULL from is_hardware_domain >> introduced in v3 > > After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. > > I am not against the change, however I think the commit message needs to updated to match what the patch is doing. > > Can you propose a new commit message? Hi Julien, Yes I agree, what about: xen/arm: Reinforce use of is_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. In the eventuality that hardware_domain is NULL, is_hardware_domain will return false because an analysis of the common and arm codebase shows that is_hardware_domain is called always with a non NULL domain pointer. Cheers, Luca > > Cheers, > > -- > Julien Grall
Hi Luca, On 14/04/2021 12:29, Luca Fancellu wrote: >> On 14 Apr 2021, at 12:16, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 14/04/2021 10:14, Luca Fancellu wrote: >>> 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> >>> --- >>> v4 changes: >>> - removed unneeded check for domain NULL from is_hardware_domain >>> introduced in v3 >> >> After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. >> >> I am not against the change, however I think the commit message needs to updated to match what the patch is doing. >> >> Can you propose a new commit message? > > Hi Julien, > > Yes I agree, what about: > > xen/arm: Reinforce use of is_hardware_domain > > Among the common and arm codebase there are few cases where I would drop 'common' because you are only modifying the arm codebase. > 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. > In the eventuality that hardware_domain is NULL, is_hardware_domain > will return false because an analysis of the common and arm codebase > shows that is_hardware_domain is called always with a non NULL > domain pointer. This paragraph seems to come out of the blue. I would drop it. How about: " There are a few places on Arm where we use pretty much an open-coded version of is_hardware_domain(). The main difference, is the helper will also block speculation (not yet implemented on Arm). The existing users are not in hot path, so blocking speculation would not hurt when it is implemented. So remove the open-coded version within the arm codebase. " If you are happy with the commit message, I will commit it the series tomorrow (to give an opportunity to Stefano to review). Cheers,
> On 14 Apr 2021, at 14:45, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 14/04/2021 12:29, Luca Fancellu wrote: >>> On 14 Apr 2021, at 12:16, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 14/04/2021 10:14, Luca Fancellu wrote: >>>> 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> >>>> --- >>>> v4 changes: >>>> - removed unneeded check for domain NULL from is_hardware_domain >>>> introduced in v3 >>> >>> After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. >>> >>> I am not against the change, however I think the commit message needs to updated to match what the patch is doing. >>> >>> Can you propose a new commit message? >> Hi Julien, >> Yes I agree, what about: >> xen/arm: Reinforce use of is_hardware_domain >> Among the common and arm codebase there are few cases where > > I would drop 'common' because you are only modifying the arm codebase. > >> 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. > > >> In the eventuality that hardware_domain is NULL, is_hardware_domain >> will return false because an analysis of the common and arm codebase >> shows that is_hardware_domain is called always with a non NULL >> domain pointer. > > This paragraph seems to come out of the blue. I would drop it. > > How about: > > " > There are a few places on Arm where we use pretty much an open-coded version of is_hardware_domain(). The main difference, is the helper will also block speculation (not yet implemented on Arm). > > The existing users are not in hot path, so blocking speculation would not hurt when it is implemented. So remove the open-coded version within the arm codebase. > " > > If you are happy with the commit message, I will commit it the series tomorrow (to give an opportunity to Stefano to review). > Hi Julien, Yes your version is much better, thank you very much! Cheers, Luca > Cheers, > > -- > Julien Grall
On Wed, 14 Apr 2021, Luca Fancellu wrote: > > On 14 Apr 2021, at 14:45, Julien Grall <julien@xen.org> wrote: > > > > Hi Luca, > > > > On 14/04/2021 12:29, Luca Fancellu wrote: > >>> On 14 Apr 2021, at 12:16, Julien Grall <julien@xen.org> wrote: > >>> > >>> Hi Luca, > >>> > >>> On 14/04/2021 10:14, Luca Fancellu wrote: > >>>> 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> > >>>> --- > >>>> v4 changes: > >>>> - removed unneeded check for domain NULL from is_hardware_domain > >>>> introduced in v3 > >>> > >>> After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. > >>> > >>> I am not against the change, however I think the commit message needs to updated to match what the patch is doing. > >>> > >>> Can you propose a new commit message? > >> Hi Julien, > >> Yes I agree, what about: > >> xen/arm: Reinforce use of is_hardware_domain > >> Among the common and arm codebase there are few cases where > > > > I would drop 'common' because you are only modifying the arm codebase. > > > >> 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. > > > > > >> In the eventuality that hardware_domain is NULL, is_hardware_domain > >> will return false because an analysis of the common and arm codebase > >> shows that is_hardware_domain is called always with a non NULL > >> domain pointer. > > > > This paragraph seems to come out of the blue. I would drop it. > > > > How about: > > > > " > > There are a few places on Arm where we use pretty much an open-coded version of is_hardware_domain(). The main difference, is the helper will also block speculation (not yet implemented on Arm). > > > > The existing users are not in hot path, so blocking speculation would not hurt when it is implemented. So remove the open-coded version within the arm codebase. > > " > > > > If you are happy with the commit message, I will commit it the series tomorrow (to give an opportunity to Stefano to review). > > > > Hi Julien, > > Yes your version is much better, thank you very much! It looks great, thanks for your work on this!
Hi, On 14/04/2021 21:35, Stefano Stabellini wrote: > On Wed, 14 Apr 2021, Luca Fancellu wrote: >>> On 14 Apr 2021, at 14:45, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 14/04/2021 12:29, Luca Fancellu wrote: >>>>> On 14 Apr 2021, at 12:16, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Luca, >>>>> >>>>> On 14/04/2021 10:14, Luca Fancellu wrote: >>>>>> 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> >>>>>> --- >>>>>> v4 changes: >>>>>> - removed unneeded check for domain NULL from is_hardware_domain >>>>>> introduced in v3 >>>>> >>>>> After this change, this patch is only avoid to open-code is_hardware_domain(). Although, it adds an extra speculation barrier. >>>>> >>>>> I am not against the change, however I think the commit message needs to updated to match what the patch is doing. >>>>> >>>>> Can you propose a new commit message? >>>> Hi Julien, >>>> Yes I agree, what about: >>>> xen/arm: Reinforce use of is_hardware_domain >>>> Among the common and arm codebase there are few cases where >>> >>> I would drop 'common' because you are only modifying the arm codebase. >>> >>>> 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. >>> >>> >>>> In the eventuality that hardware_domain is NULL, is_hardware_domain >>>> will return false because an analysis of the common and arm codebase >>>> shows that is_hardware_domain is called always with a non NULL >>>> domain pointer. >>> >>> This paragraph seems to come out of the blue. I would drop it. >>> >>> How about: >>> >>> " >>> There are a few places on Arm where we use pretty much an open-coded version of is_hardware_domain(). The main difference, is the helper will also block speculation (not yet implemented on Arm). >>> >>> The existing users are not in hot path, so blocking speculation would not hurt when it is implemented. So remove the open-coded version within the arm codebase. >>> " >>> >>> If you are happy with the commit message, I will commit it the series tomorrow (to give an opportunity to Stefano to review). >>> >> >> Hi Julien, >> >> Yes your version is much better, thank you very much! > > It looks great, thanks for your work on this! I have committed the series. Thanks for the work! Cheers,
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/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..0a74df9931 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;
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> --- v4 changes: - removed unneeded check for domain NULL from is_hardware_domain introduced in v3 v3 changes: - removed unneeded parenthesis for macro is_domain_direct_mapped - is_hardware_domain() checks for the passed domain and if it is NULL, it returns false. - reverted back checks in the function late_hwdom_init --- xen/arch/arm/irq.c | 2 +- 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 +- 5 files changed, 5 insertions(+), 5 deletions(-)