[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated


  • To: Julien Grall <julien@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Penny Zheng <Penny.Zheng@xxxxxxx>
  • Date: Wed, 29 Jun 2022 08:00:58 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • 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=2; 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=J/1TKzmdiSLsy6p3WNNRvQVY1fgUO+Mpr2uf4Sm5CK4=; b=YMn5HtwDhtYfmTPmrkDwc2WTmbvi9CRBdiDx5BpenqUQ6bR2doFTQW96qjlFI+MqGs21oK5HZ0+vIkPv4I7fYYUaVUAb+gSl2fB/62+8ln+1/hobd7WUnmg3Ia1LJzNpEyM93iLRy6g1T/lJrXnc9WX2/e+R/u7HvIrSr5c8wy33k33tmjECKEVGOdIBZMikBxXlgRKfG/Vw6MCy0vBhNu4vA/njAI1uCt0OP7Kgew0RF2leQfJPerHerWaltndTnF2YD25Dnwy18URRzPEtFSpPIt21Kya4x8SCA6TYvFF+/Y8cJSKPxpl1CZd78zCyDA1/8snmqErLZyeEV8Xr7Q==
  • 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=J/1TKzmdiSLsy6p3WNNRvQVY1fgUO+Mpr2uf4Sm5CK4=; b=GH3jDjeLTNpoPB+oyJC7Ni7d2sFZISAK2UderePj2Cfp27++WdygQgJa9xxtPMjA1Kzf3iTE6idzKU1wXA9LZHvYKaaiFs3UFa0ijfAMX8CzH46XHp2XWgHwkxIQvoBoS8lKdhcvk5h/2junhxewRAwcOmjfRIATD79Wc2ePlIABbtN+oWKYcEGOAgq0+izH0OS3m4P/M3Oox+aEBYfrFlgdC0F+CfuS/kn8Rsa/o4bbcG5kKmHh9ZUqVX+jXMQxfNh0G4GcWqbsS6MqmZdj6KJx3IFsqO7V0KKtGRWvavYaPVE4CYcthiYnRvpqsLVS+hMA8YO9VGDzPhCEArws0A==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=czEI22XMJRqNO1sriJ24tXIt48VekGN5LqOiXWxiqxWlUnV5Sim4t0YCVP12CASSWcUJWp4jAXi4YUtCOhCE1/y1sGUuVj4BvYOGTXr1h9QUH3j4UeUpA0be2qO1O6GY8tLqbNWPpMl4Z2FWqXUqNKgs0fRwT4Q7DTt5CpV4HwoA8RMdjVBHxvyNs0uhHPoNKeesclM+UvSpxeIo/73PSM+Q0z9pUTJQVqTWFMB/77Qfdwk/s+7IfVeEvh4VpKpoFoXUUxxnBIlUogW57W+/vGz2mdq/C0l60+Cj2WtdsZcDxAkFKOYl9GL/U1hqxk42hdFz805TqjTiiH484d25bw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iQH/iPbk0XStfDdv4D/V1eZfZyU2XveeqOcg6mZT9zU83FsZNyIwP/jptG/0NVC4bGGsR4JedoK1xFXE+z0AzVU6GpWsrLw20cHkjqqtwLt5PNS116CfGeUYUIMcICHDIUe5Ylh34L5a+JknQPPicPUzVIjQxCmu9KJXZ4QCOTEzVOK6DKXuQ+uKsPwFw7apB3zoW3jUngPNluSv9L775WNVb0x1+/2SHGdwgyUkifyD1gL5eqm84TUZRPDi4sWzrxhS6PivpkU0VFJz0kvi9+GgaDRrZ9G0sNozOnUWDTAojpBz2BNZ4ouHZKp20MVxOw6ziZ24BQcA9T5OH/PwQg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Wei Chen <Wei.Chen@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 29 Jun 2022 08:01:13 +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: AQHYhGRF6qbL+APV6ECW1Ou9EE75yq1e9ZGAgAcdi0A=
  • Thread-topic: [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, June 25, 2022 3:18 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v5 5/8] xen/arm: Add additional reference to owner
> domain when the owner is allocated
> 
> Hi Penny,
> 
> On 20/06/2022 06:11, Penny Zheng wrote:
> > Borrower domain will fail to get a page ref using the owner domain
> > during allocation, when the owner is created after borrower.
> >
> > So here, we decide to get and add the right amount of reference, which
> > is the number of borrowers, when the owner is allocated.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 changes:
> > - no change
> > ---
> > v3 change:
> > - printk rather than dprintk since it is a serious error
> > ---
> > v2 change:
> > - new commit
> > ---
> >   xen/arch/arm/domain_build.c | 62
> +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d4fd64e2bd..650d18f5ef 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -799,6 +799,34 @@ static mfn_t __init
> > acquire_shared_memory_bank(struct domain *d,
> >
> >   }
> >
> > +static int __init acquire_nr_borrower_domain(struct domain *d,
> > +                                             paddr_t pbase, paddr_t psize,
> > +                                             unsigned long
> > +*nr_borrowers) {
> > +    unsigned long bank;
> > +
> > +    /* Iterate reserved memory to find requested shm bank. */
> > +    for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> > +    {
> > +        paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> > +        paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> > +
> > +        if ( pbase == bank_start && psize == bank_size )
> > +            break;
> > +    }
> > +
> > +    if ( bank == bootinfo.reserved_mem.nr_banks )
> > +        return -ENOENT;
> > +
> > +    if ( d == dom_io )
> > +        *nr_borrowers =
> bootinfo.reserved_mem.bank[bank].nr_shm_domain;
> > +    else
> > +        /* Exclude the owner domain itself. */
> NIT: I think this comment wants to be just above the 'if' and expanded to
> explain why the "dom_io" is not included. AFAIU, this is because "dom_io" is
> not described in the Device-Tree, so it would not be taken into account for
> nr_shm_domain.
> 
> > +        *nr_borrowers =
> > + bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1;
> 
> TBH, given the use here. I would have consider to not increment
> nr_shm_domain if the role was owner in parsing code. This is v5 now, so I
> would be OK with the comment above.
> 
> But I would suggest to consider it as a follow-up.
> 

LTM, it is not a big change, I'll try to include it in the next serie~

> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Func allocate_shared_memory is supposed to be only called
> >    * from the owner.
> > @@ -810,6 +838,8 @@ static int __init allocate_shared_memory(struct
> domain *d,
> >   {
> >       mfn_t smfn;
> >       int ret = 0;
> > +    unsigned long nr_pages, nr_borrowers, i;
> > +    struct page_info *page;
> >
> >       dprintk(XENLOG_INFO,
> >               "Allocate static shared memory BANK
> > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -824,6 +854,7 @@ static int __init
> allocate_shared_memory(struct domain *d,
> >        * DOMID_IO is the domain, like DOMID_XEN, that is not auto-
> translated.
> >        * It sees RAM 1:1 and we do not need to create P2M mapping for it
> >        */
> > +    nr_pages = PFN_DOWN(psize);
> >       if ( d != dom_io )
> >       {
> >           ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
> > PFN_DOWN(psize)); @@ -835,6 +866,37 @@ static int __init
> allocate_shared_memory(struct domain *d,
> >           }
> >       }
> >
> > +    /*
> > +     * Get the right amount of references per page, which is the number of
> > +     * borrow domains.
> > +     */
> > +    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    /*
> > +     * Instead of let borrower domain get a page ref, we add as many
> 
> Typo: s/let/letting/
> 
> > +     * additional reference as the number of borrowers when the owner
> > +     * is allocated, since there is a chance that owner is created
> > +     * after borrower.
> 
> What if the borrower is created first? Wouldn't this result to add pages in 
> the
> P2M without reference?
> 
> If yes, then I think this is worth an explaination.
> 

Yes, it is intended to be the way you said, and I'll add a comment to explain.

> > +     */
> > +    page = mfn_to_page(smfn);
> 
> Where do you validate the range [smfn, nr_pages]?
> 
> > +    for ( i = 0; i < nr_pages; i++ )
> > +    {
> > +        if ( !get_page_nr(page + i, d, nr_borrowers) )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "Failed to add %lu references to page %"PRI_mfn".\n",
> > +                   nr_borrowers, mfn_x(smfn) + i);
> > +            goto fail;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +
> > + fail:
> > +    while ( --i >= 0 )
> > +        put_page_nr(page + i, nr_borrowers);
> >       return ret;
> >   }
> >
> 
> Cheers,
> 
> --
> Julien Grall

 


Rackspace

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