Hi, julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Saturday, April 9, 2022 5:26 PM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <n...@arm.com>; Stefano Stabellini <sstabell...@kernel.org>; Bertrand
> Marquis <bertrand.marq...@arm.com>; Volodymyr Babchuk
> <volodymyr_babc...@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.zh...@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.zh...@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
