[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


  • To: Julien Grall <julien@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Penny Zheng <Penny.Zheng@xxxxxxx>
  • Date: Thu, 21 Apr 2022 07:00:37 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Znk8eN52uG/uV4s7QJ837O3y9WTc5AsZ1ioifoeZcH0=; b=XgC8hviRkBXYQySrBg9h36jmwW4/ic3ewkeNY8SJlz7ckvPZo4jw4eqWj6XNSF/i0zs5ygwTYrIKN4/fATWLZIW1O4Ld1uC8Z6+S4R4Bq+GGCxi/wIUJm/bwoZaW7sklfp4rOLIeIRx2+bpdkeh7769AMOlqcj3uttQ44xGN5u/E1H1W4pyzCGuB9pEqMkutTBt1BZzWLYJYCtsjZSnMyaoPSHb+Rba4xx1q2/4boDysqA3FCmjsItXnS4iFnPwepLSfKZiCdjHS0oTX2K25xdgMgACrS9gwPQlBQuRpzAsR8cliku/nFQu0PX9juthZ/4zFC6KsvRO18cOYJzfTlQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mDERpl5q7zIgWDDS3HAkm+RgQ0n0rs9leAb7vp14ja+SVSfkX394efwH2VA1Cd9osVtE5Qe84OmpnvlhcCiz1Mo/5GCPPAA2H5KStEug4KnI81N1se3sbhdq+wEhliEN428gQdkr3gVEHbvzRDI28pLvc3D3ACHM1Jw9ROzmUhKeKJ8WR73WeY5dHbjZZahDmt1cp+y5etqHeO1Ho/sJ61DD58QUcmSEJyG6VzwGHGPfTBE+md9vVNU2v5G055Wff1SlMfTBV9yUmVp1pmzYvmOPYz5U5xhk3o/JTGjNWsrakzgdJz8S5xo0HXa2tA0UlGMKGYt6C0T9zIUsFV9ShA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: nd <nd@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 21 Apr 2022 07:01:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYNQ8XP5NnJaO3/U2NKPTcSWC8l6znfYSAgBKyp0A=
  • Thread-topic: [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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.