On 29/06/2022 08:13, Penny Zheng wrote:
Hi Julien
Hi Penny,
-----Original Message-----
From: Julien Grall <jul...@xen.org>
Sent: Saturday, June 25, 2022 2:22 AM
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
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?
[...]
+ 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.
Cheers,
--
Julien Grall
