Hi Andrew, > -----Original Message----- > From: Andrew Cooper <andrew.coop...@citrix.com> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > On 13/10/2022 09:38, Henry Wang wrote: > > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > > when the domain is created. Considering the worst case of page tables > > and keep a buffer, populate 16 pages as the default value to the P2M > > pages pool in arch_domain_create() at the domain creation stage to > > satisfy the GICv2 requirement. > > > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M > pool") > > Suggested-by: Julien Grall <jgr...@amazon.com> > > Signed-off-by: Henry Wang <henry.w...@arm.com> > > --- > > This should also be backported to 4.13, 4.14, 4.15 and 4.16. > > --- > > xen/arch/arm/domain.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index 2c84e6dbbb..e40e2bcba1 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, > > BUG(); > > } > > > > + spin_lock(&d->arch.paging.lock); > > + /* > > + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 > area > > + * when the domain is created. Considering the worst case for page > > + * tables and keep a buffer, populate 16 pages to the P2M pages pool > here. > > + */ > > + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) > > + { > > + p2m_set_allocation(d, 0, NULL); > > + spin_unlock(&d->arch.paging.lock); > > + goto fail; > > + } > > + spin_unlock(&d->arch.paging.lock); > > Generally, this would be better written as > > spin_lock(); > if ( rc = p2m_set_allocation(16) ) > p2m_set_allocation(0) > spin_unlock(); > > if ( rc ) > goto fail; > > to reduce the number of spin_unlock() calls and make the error paths > more clear. However...
I think in Arm's arch_domain_create(), all the error handling are the same style using: if ( (rc = <function>) !=0 ) goto fail; and we need to keep them the same? But I think I will drop the p2m_set_allocation(d, 0, NULL); as the arch_domain_destroy(d) in fail: d->is_dying = DOMDYING_dead; arch_domain_destroy(d); will clean-up the pool. Kind regards, Henry > > > + > > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > > goto fail; > > > > ... you've got a problem on this error path, so the set allocation to 0 > needs to be in the fail: path with suitable locking. > > There are perhaps better ways of doing it in 4.15(?) and later, but not > in earlier versions. As this is a fix to a bug in a security patch, > simplicity is generally the better approach. > > ~Andrew