| Message ID | 20240312130331.78418-12-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt1 | expand |
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > Currently Xen is not exporting the static shared memory regions > to the device tree as /memory node, this commit is fixing this > issue. Looking at the implementation, you will always call make_shm_memory_node() twice. For the first time, to create /memory node and for the second time to create entry under /reserved-memory. Also, you will create a separate /memory node for every single shmem region instead of combining them in a single /memory region like make_memory_node() would do. Can't we reuse this function for simplicity? Also, afaict it is not forbidden to specify shmem range (correct me if I'm wrong), where guest address will be within with RAM allocated by Xen (e.g. GPA RAM range 0x40000000 - 0x60000000 and shmem is at 0x50000000). In this case, you would create yet another /memory node that would result in overlap (i.e. more than one /memory node specifying the same range). ~Michal
> On 25 Mar 2024, at 08:58, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > Hi Michal, > On 12/03/2024 14:03, Luca Fancellu wrote: >> >> >> Currently Xen is not exporting the static shared memory regions >> to the device tree as /memory node, this commit is fixing this >> issue. > Looking at the implementation, you will always call make_shm_memory_node() twice. For the first > time, to create /memory node and for the second time to create entry under /reserved-memory. Also, > you will create a separate /memory node for every single shmem region instead of combining them > in a single /memory region like make_memory_node() would do. Can't we reuse this function for simplicity? You mean using make_memory_node() to populate /reserved-memory and /memory? I feel they are very different In terms of properties to be created, so I don’t think we should create a make_memory_node() that does both. Otherwise if you were suggesting to modify make_memory_node() only for what concerns the /memory nodes, it might be feasible, however there are some parts that need to be skipped with some flags (all the code accessing .type member), if I understand correctly you like this function because it doesn’t create one node for every bank, but it creates reg addresses instead, in that case I could modify the current make_shm_memory_node() to do the same. > > Also, afaict it is not forbidden to specify shmem range (correct me if I'm wrong), where guest address will be > within with RAM allocated by Xen (e.g. GPA RAM range 0x40000000 - 0x60000000 and shmem is at 0x50000000). In this case, > you would create yet another /memory node that would result in overlap (i.e. more than one /memory node specifying the same range). This is a good point I didn’t think about, yes currently the code is creating overlapping nodes in that case, wow so it means I need to compute the non overlapping regions and emit a /memory node then! :) ouch Please let me know if I understood correctly your comments. Cheers, Luca > > ~Michal
Hi Luca, On 26/03/2024 15:19, Luca Fancellu wrote: > > >> On 25 Mar 2024, at 08:58, Michal Orzel <michal.orzel@amd.com> wrote: >> >> Hi Luca, >> > > Hi Michal, > >> On 12/03/2024 14:03, Luca Fancellu wrote: >>> >>> >>> Currently Xen is not exporting the static shared memory regions >>> to the device tree as /memory node, this commit is fixing this >>> issue. >> Looking at the implementation, you will always call make_shm_memory_node() twice. For the first >> time, to create /memory node and for the second time to create entry under /reserved-memory. Also, >> you will create a separate /memory node for every single shmem region instead of combining them >> in a single /memory region like make_memory_node() would do. Can't we reuse this function for simplicity? > > You mean using make_memory_node() to populate /reserved-memory and /memory? I feel they are very different > In terms of properties to be created, so I don’t think we should create a make_memory_node() that does both. > > Otherwise if you were suggesting to modify make_memory_node() only for what concerns the /memory nodes, yes, this is what I meant > it might be feasible, however there are some parts that need to be skipped with some flags (all the code accessing .type > member), if I understand correctly you like this function because it doesn’t create one node for every bank, but it creates > reg addresses instead, in that case I could modify the current make_shm_memory_node() to do the same. My concern is that we will have 2 functions to create memory nodes instead of one that can be reused. I know the issue is with .type member. If skipping results in a worse code, then I'm ok with make_shm_memory_node() used for 2 purposes (would it be possible to create /memory and entry under /reserved in the same call to a function?). > >> >> Also, afaict it is not forbidden to specify shmem range (correct me if I'm wrong), where guest address will be >> within with RAM allocated by Xen (e.g. GPA RAM range 0x40000000 - 0x60000000 and shmem is at 0x50000000). In this case, >> you would create yet another /memory node that would result in overlap (i.e. more than one /memory node specifying the same range). > > This is a good point I didn’t think about, yes currently the code is creating overlapping nodes in that case, wow so it means I > need to compute the non overlapping regions and emit a /memory node then! :) ouch > > Please let me know if I understood correctly your comments. > > Cheers, > Luca > >> >> ~Michal > ~Michal
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index fe2a771d4984..0892020f21a0 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -647,6 +647,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; + /* List static shared memory regions as /memory@<address> nodes */ + ret = make_shm_memory_node(d, kinfo, addrcells, sizecells, false); + if ( ret ) + return ret; + ret = make_resv_memory_node(d, kinfo, addrcells, sizecells); if ( ret ) goto err; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 575e906d81a6..bd7716cd5829 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1723,7 +1723,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, * static shared memory nodes there. */ res = make_shm_memory_node(d, kinfo, dt_n_addr_cells(node), - dt_n_size_cells(node)); + dt_n_size_cells(node), true); if ( res ) return res; } @@ -1780,6 +1780,11 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } + /* List static shared memory regions as /memory@<address> nodes */ + res = make_shm_memory_node(d, kinfo, addrcells, sizecells, false); + if ( res ) + return res; + if ( !res_mem_node_found ) { res = make_resv_memory_node(d, kinfo, addrcells, sizecells); diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index d28b9540d49b..c118bbb1c43b 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -37,7 +37,7 @@ int remove_shm_holes_for_domU(const struct kernel_info *kinfo, int make_shm_memory_node(const struct domain *d, const struct kernel_info *kinfo, int addrcells, - int sizecells); + int sizecells, bool is_resv_mem_node); #else /* !CONFIG_STATIC_SHM */ @@ -85,7 +85,8 @@ static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo, static inline int make_shm_memory_node(const struct domain *d, const struct kernel_info *kinfo, - int addrcells, int sizecells) + int addrcells, int sizecells, + bool is_resv_mem_node) { return 0; } diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 67d5fa3b5d25..cdaf4485c934 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -289,7 +289,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, int __init make_shm_memory_node(const struct domain *d, const struct kernel_info *kinfo, int addrcells, - int sizecells) + int sizecells, bool is_resv_mem_node) { const struct membanks *mem = &kinfo->shm_mem.common; void *fdt = kinfo->fdt; @@ -300,11 +300,15 @@ int __init make_shm_memory_node(const struct domain *d, return 0; /* - * For each shared memory region, a range is exposed under - * the /reserved-memory node as a child node. Each range sub-node is - * named xen-shmem@<address>. + * When is_resv_mem_node is true, it means this function is called to + * create nodes under /reserved-memory, so for each shared memory region, a + * range is exposed under the /reserved-memory node as a child node. Each + * range sub-node is named xen-shmem@<address>. + * Otherwise the function is called under / and will create + * /memory@<address> nodes for each static shared memory region. */ - dt_dprintk("Create xen-shmem node\n"); + dt_dprintk("Create static shared memory %s nodes\n", + is_resv_mem_node ? "/reserved-memory/xen-shmem" : "/memory"); for ( ; i < mem->nr_banks; i++ ) { @@ -316,11 +320,16 @@ int __init make_shm_memory_node(const struct domain *d, __be32 *cells; unsigned int len = (addrcells + sizecells) * sizeof(__be32); - res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start); + res = domain_fdt_begin_node(fdt, + is_resv_mem_node ? "xen-shmem" : "memory", + mem->bank[i].start); if ( res ) return res; - res = fdt_property(fdt, "compatible", compat, sizeof(compat)); + if ( is_resv_mem_node ) + res = fdt_property(fdt, "compatible", compat, sizeof(compat)); + else + res = fdt_property_string(fdt, "device_type", "memory"); if ( res ) return res; @@ -334,20 +343,23 @@ int __init make_shm_memory_node(const struct domain *d, dt_dprintk("Shared memory bank %u: %#"PRIx64"->%#"PRIx64"\n", i, start, start + size); - res = fdt_property_string(fdt, "xen,id", - mem->bank[i].shmem_extra->shm_id); - if ( res ) - return res; + if ( is_resv_mem_node ) + { + res = fdt_property_string(fdt, "xen,id", + mem->bank[i].shmem_extra->shm_id); + if ( res ) + return res; - /* - * TODO: - * - xen,offset: (borrower VMs only) - * 64 bit integer offset within the owner virtual machine's shared - * memory region used for the mapping in the borrower VM - */ - res = fdt_property_u64(fdt, "xen,offset", 0); - if ( res ) - return res; + /* + * TODO: + * - xen,offset: (borrower VMs only) + * 64 bit integer offset within the owner virtual machine's shared + * memory region used for the mapping in the borrower VM + */ + res = fdt_property_u64(fdt, "xen,offset", 0); + if ( res ) + return res; + } res = fdt_end_node(fdt); if ( res ) @@ -536,7 +548,7 @@ int __init make_resv_memory_node(const struct domain *d, if ( res ) return res; - res = make_shm_memory_node(d, kinfo, addrcells, sizecells); + res = make_shm_memory_node(d, kinfo, addrcells, sizecells, true); if ( res ) return res;
Currently Xen is not exporting the static shared memory regions to the device tree as /memory node, this commit is fixing this issue. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/dom0less-build.c | 5 +++ xen/arch/arm/domain_build.c | 7 +++- xen/arch/arm/include/asm/static-shmem.h | 5 ++- xen/arch/arm/static-shmem.c | 54 +++++++++++++++---------- 4 files changed, 47 insertions(+), 24 deletions(-)