> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Wednesday, June 29, 2022 6:35 PM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Jan Beulich <jbeul...@suse.com>; Wei Liu <w...@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 <jul...@xen.org>
> >> Sent: Saturday, June 25, 2022 2:22 AM
> >> To: Penny Zheng <penny.zh...@arm.com>; xen-
> de...@lists.xenproject.org
> >> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> >> <sstabell...@kernel.org>; Bertrand Marquis
> >> <bertrand.marq...@arm.com>; Volodymyr Babchuk
> >> <volodymyr_babc...@epam.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; George Dunlap
> >> <george.dun...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Wei Liu
> >> <w...@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.zh...@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.zh...@arm.com>
> >>> Reviewed-by: Stefano Stabellini <sstabell...@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
