[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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, -----Original Message----- From: Julien Grall <julien@xxxxxxx> Sent: Saturday, June 25, 2022 2:22 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>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu <wl@xxxxxxx> 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@xxxxxxx> 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 amongmultipledomains 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@xxxxxxx> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> --- 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 __initappend_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 __initacquire_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 calledI 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |