[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain
Hi, julien > -----Original Message----- > From: Julien Grall <julien@xxxxxxx> > Sent: Saturday, April 9, 2022 5:26 PM > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx > Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand > Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk > <Volodymyr_Babchuk@xxxxxxxx> > Subject: Re: [PATCH v1 08/13] xen/arm: destroy static shared memory when > de-construct domain > > Hi Penny, > > On 11/03/2022 06:11, Penny Zheng wrote: > > From: Penny Zheng <penny.zheng@xxxxxxx> > > > > This commit introduces a new helper destroy_domain_shm to destroy > > static shared memory at domain de-construction. > > > > This patch only considers the scenario where the owner domain is the > > default dom_shared, for user-defined owner domain, it will be covered > > in the following patches. > > > > Since all domains are borrower domains, we could simply remove guest > > P2M foreign mapping of statically shared memory region and drop the > > reference added at guest_physmap_add_shm. > > > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx> > > --- > > xen/arch/arm/domain.c | 48 > +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index > > 1ff1df5d3f..f0bfd67fe5 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -34,6 +34,7 @@ > > #include <asm/platform.h> > > #include <asm/procinfo.h> > > #include <asm/regs.h> > > +#include <asm/setup.h> > > #include <asm/tee/tee.h> > > #include <asm/vfp.h> > > #include <asm/vgic.h> > > @@ -993,6 +994,48 @@ static int relinquish_memory(struct domain *d, > struct page_list_head *list) > > return ret; > > } > > > > +#ifdef CONFIG_STATIC_SHM > > +static int domain_destroy_shm(struct domain *d) { > > + int ret = 0; > > + unsigned long i = 0UL, j; > > + > > + if ( d->arch.shm_mem == NULL ) > > + return ret; > > You already return the value here. So... > > > + else > > ... the else is pointless. > > > + { > > + for ( ; i < d->arch.shm_mem->nr_banks; i++ ) > > + { > > + unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem- > >bank[i].size); > > + gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start); > > + > > + for ( j = 0; j < nr_gfns; j++ ) > > + { > > + mfn_t mfn; > > + > > + mfn = gfn_to_mfn(d, gfn_add(gfn, j)); > > A domain is allowed to modify its P2M. So there are no guarantee that the > GFN will still point to the shared memory. This will allow the guest... > > > + if ( !mfn_valid(mfn) ) > > + { > > + dprintk(XENLOG_ERR, > > + "Domain %pd page number %lx invalid.\n", > > + d, gfn_x(gfn) + i); > > + return -EINVAL; > > ... to actively prevent destruction. > > > + } > > > > + > > + ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, > > 0); > > + if ( ret ) > > + return ret; > > + > > + /* Drop the reference. */ > > + put_page(mfn_to_page(mfn)); > > guest_physmap_remove_page() will already drop the reference taken for the > foreign mapping. I couldn't find any other reference taken, what is the > put_page() for? > > Also, as per above we don't know whether this is a page from the shared page. > So we can't blindly call put_page(). > > However, I don't think we need any specific code here. We can rely on > relinquish_p2m_mappings() to drop any reference. If there is an extra one for > shared mappings, then we should update p2m_put_l3_page(). > True, true. Thanks for pointing this out! In p2m_put_l3_page, it has already called put_page() for foreign mapping, definitely no extra one here! > Cheers, > > -- > Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |