RE: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io

From: Penny Zheng <Penny.Zheng@arm.com>
To: Julien Grall <julien@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Wei Chen <Wei.Chen@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>, Wei Liu <wl@xen.org>
Subject: RE: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
Date: Mon, 4 Jul 2022 07:20:24 +0000	[thread overview]
Message-ID: <DU2PR08MB7325AF32FF119BCDB5890C58F7BE9@DU2PR08MB7325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <5a49381c-c69d-88dc-1bba-783241dbfe23@xen.org>

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, June 29, 2022 6:35 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
> 
> 
> 
> On 29/06/2022 08:13, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 

Hi Julien
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Saturday, June 25, 2022 2:22 AM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap
> >> <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu
> >> <wl@xen.org>
> >> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to
> >> the default owner dom_io
> >>
> >> Hi Penny,
> >>
> >> On 20/06/2022 06:11, Penny Zheng wrote:
> >>> From: Penny Zheng <penny.zheng@arm.com>
> >>>
> >>> This commit introduces process_shm to cope with static shared memory
> >>> in domain construction.
> >>>
> >>> DOMID_IO will be the default owner of memory pre-shared among
> >> multiple
> >>> domains at boot time, when no explicit owner is specified.
> >>
> >> The document in patch #1 suggest the page will be shared with
> dom_shared.
> >> But here you say "DOMID_IO".
> >>
> >> Which one is correct?
> >>
> >
> > I’ll fix the documentation, DOM_IO is the last call.
> >
> >>>
> >>> This commit only considers allocating static shared memory to dom_io
> >>> when owner domain is not explicitly defined in device tree, all the
> >>> left, including the "borrower" code path, the "explicit owner" code
> >>> path, shall be introduced later in the following patches.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> ---
> >>> v5 change:
> >>> - refine in-code comment
> >>> ---
> >>> v4 change:
> >>> - no changes
> >>> ---
> >>> v3 change:
> >>> - refine in-code comment
> >>> ---
> >>> v2 change:
> >>> - instead of introducing a new system domain, reuse the existing
> >>> dom_io
> >>> - make dom_io a non-auto-translated domain, then no need to create
> >>> P2M for it
> >>> - change dom_io definition and make it wider to support static shm
> >>> here too
> >>> - introduce is_shm_allocated_to_domio to check whether static shm is
> >>> allocated yet, instead of using shm_mask bitmap
> >>> - add in-code comment
> >>> ---
> >>>    xen/arch/arm/domain_build.c | 132
> >> +++++++++++++++++++++++++++++++++++-
> >>>    xen/common/domain.c         |   3 +
> >>>    2 files changed, 134 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index 7ddd16c26d..91a5ace851 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -527,6 +527,10 @@ static bool __init
> >> append_static_memory_to_bank(struct domain *d,
> >>>        return true;
> >>>    }
> >>>
> >>> +/*
> >>> + * If cell is NULL, pbase and psize should hold valid values.
> >>> + * Otherwise, cell will be populated together with pbase and psize.
> >>> + */
> >>>    static mfn_t __init acquire_static_memory_bank(struct domain *d,
> >>>                                                   const __be32 **cell,
> >>>                                                   u32 addr_cells,
> >>> u32 size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> >> acquire_static_memory_bank(struct domain *d,
> >>>        mfn_t smfn;
> >>>        int res;
> >>>
> >>> -    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> >>> +    if ( cell )
> >>> +        device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> >>> + psize);
> >>
> >> I think this is a bit of a hack. To me it sounds like this should be
> >> moved out to a separate helper. This will also make the interface of
> >> acquire_shared_memory_bank() less questionable (see below).
> >>
> >
> > Ok,  I'll try to not reuse acquire_static_memory_bank in
> > acquire_shared_memory_bank.
> 
> I am OK with that so long it doesn't involve too much duplication.
> 
> >>>        ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> >>> PAGE_SIZE));
> >>
> >> In the context of your series, who is checking that both psize and
> >> pbase are suitably aligned?
> >>
> >
> > Actually, the whole parsing process is redundant for the static shared
> memory.
> > I've already parsed it and checked it before in process_shm.
> 
> I looked at process_shm() and couldn't find any code that would check pbase
> and psize are suitable aligned (ASSERT() doesn't count).
> 
> >
> >>> +    return true;
> >>> +}
> >>> +
> >>> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> >>> +                                               u32 addr_cells, u32 size_cells,
> >>> +                                               paddr_t *pbase,
> >>> +paddr_t *psize)
> >>
> >> There is something that doesn't add-up in this interface. The use of
> >> pointer implies that pbase and psize may be modified by the function. So...
> >>
> >
> > Just like you points out before, it's a bit hacky to use
> > acquire_static_memory_bank, and the pointer used here is also due to
> > it. Internal parsing process of acquire_static_memory_bank needs pointer
> to deliver the result.
> >
> > I’ll rewrite acquire_shared_memory, and it will be like:
> > "
> > static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> >                                                 paddr_t pbase, paddr_t
> > psize) {
> >      mfn_t smfn;
> >      unsigned long nr_pfns;
> >      int res;
> >
> >      /*
> >       * Pages of statically shared memory shall be included
> >       * in domain_tot_pages().
> >       */
> >      nr_pfns = PFN_DOWN(psize);
> >      if ( d->max_page + nr_pfns > UINT_MAX )
> 
> On Arm32, this check would always be true a 32-bit unsigned value is always
> below UINT_MAX. On arm64, you might get away because nr_pfns is
> unsigned long (so I think the type promotion works). But this is fragile.
> 
> I would suggest to use the following check:
> 
> (UINT_MAX - d->max_page) < nr_pfns
> 
> >      {
> >          printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> >                 d, psize);
> >          return INVALID_MFN;
> >      }
> >      d->max_pages += nr_pfns;
> >
> >      smfn = maddr_to_mfn(pbase);
> >      res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> >      if ( res )
> >      {
> >          printk(XENLOG_ERR
> >                 "%pd: failed to acquire static memory: %d.\n", d, res);
> >          return INVALID_MFN;
> >      }
> >
> >      return smfn
> > }
> > "
> >
> >>> +{
> >>> +    /*
> >>> +     * Pages of statically shared memory shall be included
> >>> +     * in domain_tot_pages().
> >>> +     */
> >>> +    d->max_pages += PFN_DOWN(*psize);
> >>
> >> ... it sounds a bit strange to use psize here. If psize, can't be
> >> modified than it should probably not be a pointer.
> >>
> >> Also, where do you check that d->max_pages will not overflow?
> >>
> >
> > I'll check the overflow as follows:
> 
> See above about the check.
> 
> > "
> >      nr_pfns = PFN_DOWN(psize);
> >      if ( d->max_page + nr_pfns > UINT_MAX )
> >      {
> >          printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> >                 d, psize);
> >          return INVALID_MFN;
> >      }
> >      d->max_pages += nr_pfns;
> > "
> >
> >>> +
> >>> +    return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> >>> +                                      pbase, psize);
> >>> +
> >>> +}
> >>> +
> >>> +/*
> >>> + * Func allocate_shared_memory is supposed to be only called
> >>
> >> I am a bit concerned with the word "supposed". Are you implying that
> >> it may be called by someone that is not the owner? If not, then it
> >> should be "should".
> >>
> >> Also NIT: Spell out completely "func". I.e "The function".
> >>
> >>> + * from the owner.
> >>
> >> I read from as "current should be the owner". But I guess this is not
> >> what you mean here. Instead it looks like you mean "d" is the owner.
> >> So I would write "d should be the owner of the shared area".
> >>
> >> It would be good to have a check/ASSERT confirm this (assuming this
> >> is easy to write).
> >>
> >
> > The check is already in the calling path, I guess...
> 
> Can you please confirm it?
> 

I mean that allocate_shared_memory is only called under the following condition, and
it confirms it is the right owner.
"
        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
        {
            /* Allocate statically shared pages to the owner domain. */
            ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
                                         addr_cells, size_cells,
                                         pbase, psize, gbase);
"

TBH, apart from that, I don't know how to check if the input d is the right owner, or am I
misunderstanding some your suggestion here?

> [...]
> 
> >>> +        prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> >>> +        if ( !prop )
> >>> +        {
> >>> +            printk("Shared memory node does not provide
> >>> + \"xen,shared-
> >> mem\" property.\n");
> >>> +            return -ENOENT;
> >>> +        }
> >>> +        cells = (const __be32 *)prop->value;
> >>> +        /* xen,shared-mem = <pbase, psize, gbase>; */
> >>> +        device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> >>> +        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> >>> + PAGE_SIZE));
> >>
> >> See above about what ASSERT()s are for.
> >>
> >
> > Do you think address was suitably checked here, is it enough?
> 
> As I wrote before, ASSERT() should not be used to check user inputs.
> They need to happen in both debug and production build.
> 
> > If it is enough, I'll modify above ASSERT() to mfn_valid()
> 
> It is not clear what ASSERT() you are referring to.
> 

For whether page is aligned, I will add the below check:
"
        if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
             !IS_ALIGNED(gbase, PAGE_SIZE) )
        {
            printk("%pd: physical address %lu, size %lu or guest address %lu is not suitably aligned.\n",
                   d, pbase, psize, gbase);
            return -EINVAL;
        }
"
For whether the whole address range is valid, I will add the below check:
"
        for ( i = 0; i < PFN_DOWN(psize); i++ )
            if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
            {
                printk("%pd: invalid physical address %"PRI_paddr" or size %"PRI_paddr"\n",
                       d, pbase, psize);
                return -EINVAL;
            }
"
> Cheers,
> 
> --
> Julien Grall