| Message ID | 20220311061123.1883189-9-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Static shared memory on dom0less system | expand |
Hi Penny, On 11/03/2022 06:11, Penny Zheng wrote: > From: Penny Zheng <penny.zheng@arm.com> > > 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@arm.com> > --- > 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(). Cheers,
Hi, julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, April 9, 2022 5:26 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > 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@arm.com> > > > > 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@arm.com> > > --- > > 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
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; + else + { + 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)); + if ( !mfn_valid(mfn) ) + { + dprintk(XENLOG_ERR, + "Domain %pd page number %lx invalid.\n", + d, gfn_x(gfn) + i); + return -EINVAL; + } + + 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)); + } + } + } + + return ret; +} +#endif + /* * Record the current progress. Subsequent hypercall continuations will * logically restart work from this point. @@ -1039,6 +1082,11 @@ int domain_relinquish_resources(struct domain *d) */ domain_vpl011_deinit(d); +#ifdef CONFIG_STATIC_SHM + ret = domain_destroy_shm(d); + if ( ret ) + return ret; +#endif #ifdef CONFIG_IOREQ_SERVER ioreq_server_destroy_all(d); #endif