| Message ID | 20221017191237.11079-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/p2m: XSA-409 fix | expand |
Hi Andrew, On 17/10/2022 20:12, Andrew Cooper wrote: > p2m_init() is mostly trivial initialisation, but has two failable operations > which are on either side of the backpointer trigger for teardown to take > actions. > > p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange > p2m_init() to perform all trivial setup, then set the backpointer, then > perform all failable setup. > > This will simplify a future bugfix which needs to add a third failabile Typo: s/failabile/fallible? > operation. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 17/10/2022 21:24, Julien Grall wrote: > Hi Andrew, > > On 17/10/2022 20:12, Andrew Cooper wrote: >> p2m_init() is mostly trivial initialisation, but has two failable >> operations >> which are on either side of the backpointer trigger for teardown to take >> actions. >> >> p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so >> rearrange >> p2m_init() to perform all trivial setup, then set the backpointer, then >> perform all failable setup. >> >> This will simplify a future bugfix which needs to add a third failabile > > Typo: s/failabile/fallible? Yes, fixed. > >> operation. >> >> No practical change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thanks. ~Andrew
Hi Andrew, > On 17 Oct 2022, at 20:12, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > p2m_init() is mostly trivial initialisation, but has two failable operations Maybe say “two operations that can fail” as the failable does seem fully right :-) But I am not a native speaker so I will let that to you. > which are on either side of the backpointer trigger for teardown to take > actions. > > p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange > p2m_init() to perform all trivial setup, then set the backpointer, then > perform all failable setup. > > This will simplify a future bugfix which needs to add a third failabile > operation. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Henry Wang <Henry.Wang@arm.com> > --- > xen/arch/arm/p2m.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index f17500ddf3a3..6826f6315080 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1754,7 +1754,7 @@ void p2m_final_teardown(struct domain *d) > int p2m_init(struct domain *d) > { > struct p2m_domain *p2m = p2m_get_hostp2m(d); > - int rc = 0; > + int rc; > unsigned int cpu; > > rwlock_init(&p2m->lock); > @@ -1763,11 +1763,6 @@ int p2m_init(struct domain *d) > INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist); > > p2m->vmid = INVALID_VMID; > - > - rc = p2m_alloc_vmid(d); > - if ( rc != 0 ) > - return rc; > - > p2m->max_mapped_gfn = _gfn(0); > p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); > > @@ -1783,8 +1778,6 @@ int p2m_init(struct domain *d) > p2m->clean_pte = is_iommu_enabled(d) && > !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK); > > - rc = p2m_alloc_table(d); > - > /* > * Make sure that the type chosen to is able to store the an vCPU ID > * between 0 and the maximum of virtual CPUS supported as long as > @@ -1797,13 +1790,20 @@ int p2m_init(struct domain *d) > p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID; > > /* > - * Besides getting a domain when we only have the p2m in hand, > - * the back pointer to domain is also used in p2m_teardown() > - * as an end-of-initialization indicator. > + * "Trivial" initialisation is now complete. Set the backpointer so > + * p2m_teardown() and friends know to do something. > */ > p2m->domain = d; > > - return rc; > + rc = p2m_alloc_vmid(d); > + if ( rc ) > + return rc; > + > + rc = p2m_alloc_table(d); > + if ( rc ) > + return rc; > + > + return 0; > } > > /* > -- > 2.11.0 >
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index f17500ddf3a3..6826f6315080 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1754,7 +1754,7 @@ void p2m_final_teardown(struct domain *d) int p2m_init(struct domain *d) { struct p2m_domain *p2m = p2m_get_hostp2m(d); - int rc = 0; + int rc; unsigned int cpu; rwlock_init(&p2m->lock); @@ -1763,11 +1763,6 @@ int p2m_init(struct domain *d) INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist); p2m->vmid = INVALID_VMID; - - rc = p2m_alloc_vmid(d); - if ( rc != 0 ) - return rc; - p2m->max_mapped_gfn = _gfn(0); p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); @@ -1783,8 +1778,6 @@ int p2m_init(struct domain *d) p2m->clean_pte = is_iommu_enabled(d) && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK); - rc = p2m_alloc_table(d); - /* * Make sure that the type chosen to is able to store the an vCPU ID * between 0 and the maximum of virtual CPUS supported as long as @@ -1797,13 +1790,20 @@ int p2m_init(struct domain *d) p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID; /* - * Besides getting a domain when we only have the p2m in hand, - * the back pointer to domain is also used in p2m_teardown() - * as an end-of-initialization indicator. + * "Trivial" initialisation is now complete. Set the backpointer so + * p2m_teardown() and friends know to do something. */ p2m->domain = d; - return rc; + rc = p2m_alloc_vmid(d); + if ( rc ) + return rc; + + rc = p2m_alloc_table(d); + if ( rc ) + return rc; + + return 0; } /*
p2m_init() is mostly trivial initialisation, but has two failable operations which are on either side of the backpointer trigger for teardown to take actions. p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange p2m_init() to perform all trivial setup, then set the backpointer, then perform all failable setup. This will simplify a future bugfix which needs to add a third failabile operation. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Henry Wang <Henry.Wang@arm.com> --- xen/arch/arm/p2m.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)