Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Julien, > > Rest LGTM: > Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. >>> I don't think there is a need to respin the whole series just for these >>> fixes. >>> You should wait for the committers opinion. >> AFAICT, there are multiple changes requested in various line. So I would >> rather prefer if this is respinned. >> If this is the only patch that requires to change. You could send a new one >> in reply-to this patch. I think b4 is clever enough to pick up the new >> version in that case. > > I was wrong. b4 didn't picked up the new version. Anyway, I have applied the > new patch and send to gitlab for testing. I will merge it once it passes. Thanks a lot for that! Cheers, Luca
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi, On 22/04/2024 11:24, Julien Grall wrote: Hi, On 22/04/2024 10:26, Michal Orzel wrote: On 22/04/2024 10:07, Luca Fancellu wrote: Hi Michal, + for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size ) + { + u64 start = dt_read_number(cells, addrcells); We should no longer use Linux derived types like u64. Use uint64_t. + u64 size = dt_read_number(cells + addrcells, sizecells); + + dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); i is unsigned so the correct format specifier should be %u Right, should have been more careful when copying the code from above +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, + __be32 *reg, int *nr_cells, + int addrcells, int sizecells) +{ + const struct membanks *mem = >shm_mem.common; + unsigned int i; + __be32 *cells; + + BUG_ON(!nr_cells || !reg); + + cells = [*nr_cells]; + for ( i = 0; i < mem->nr_banks; i++ ) + { + u64 start = mem->bank[i].start; ditto Will fix, here paddr_t should be ok isn’t it? yes Rest LGTM: Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. I don't think there is a need to respin the whole series just for these fixes. You should wait for the committers opinion. AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned. If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case. I was wrong. b4 didn't picked up the new version. Anyway, I have applied the new patch and send to gitlab for testing. I will merge it once it passes. Cheers, -- Julien Grall
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Luca, On 22/04/2024 11:39, Luca Fancellu wrote: On 22 Apr 2024, at 11:24, Julien Grall wrote: Hi, On 22/04/2024 10:26, Michal Orzel wrote: On 22/04/2024 10:07, Luca Fancellu wrote: Hi Michal, +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size ) +{ +u64 start = dt_read_number(cells, addrcells); We should no longer use Linux derived types like u64. Use uint64_t. +u64 size = dt_read_number(cells + addrcells, sizecells); + +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); i is unsigned so the correct format specifier should be %u Right, should have been more careful when copying the code from above +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, +__be32 *reg, int *nr_cells, +int addrcells, int sizecells) +{ +const struct membanks *mem = >shm_mem.common; +unsigned int i; +__be32 *cells; + +BUG_ON(!nr_cells || !reg); + +cells = [*nr_cells]; +for ( i = 0; i < mem->nr_banks; i++ ) +{ +u64 start = mem->bank[i].start; ditto Will fix, here paddr_t should be ok isn’t it? yes Rest LGTM: Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. I don't think there is a need to respin the whole series just for these fixes. You should wait for the committers opinion. AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned. If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case. All the other patches are already reviewed by Michal, if you agree with his review it’s ok for me to respin only this one I didn't review the patch in details. But I agree with his comments on it. Cheers, -- Julien Grall
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
> On 22 Apr 2024, at 11:24, Julien Grall wrote: > > Hi, > > On 22/04/2024 10:26, Michal Orzel wrote: >> On 22/04/2024 10:07, Luca Fancellu wrote: >>> >>> >>> Hi Michal, >>> > +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += > reg_size ) > +{ > +u64 start = dt_read_number(cells, addrcells); We should no longer use Linux derived types like u64. Use uint64_t. > +u64 size = dt_read_number(cells + addrcells, sizecells); > + > +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", > + i, start, start + size); i is unsigned so the correct format specifier should be %u >>> >>> Right, should have been more careful when copying the code from above >>> > > +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, > +__be32 *reg, int *nr_cells, > +int addrcells, int sizecells) > +{ > +const struct membanks *mem = >shm_mem.common; > +unsigned int i; > +__be32 *cells; > + > +BUG_ON(!nr_cells || !reg); > + > +cells = [*nr_cells]; > +for ( i = 0; i < mem->nr_banks; i++ ) > +{ > +u64 start = mem->bank[i].start; ditto >>> >>> Will fix, here paddr_t should be ok isn’t it? >> yes >>> Rest LGTM: Reviewed-by: Michal Orzel >>> >>> Thanks, I will send the next one shortly. >> I don't think there is a need to respin the whole series just for these >> fixes. >> You should wait for the committers opinion. > > AFAICT, there are multiple changes requested in various line. So I would > rather prefer if this is respinned. > > If this is the only patch that requires to change. You could send a new one > in reply-to this patch. I think b4 is clever enough to pick up the new > version in that case. All the other patches are already reviewed by Michal, if you agree with his review it’s ok for me to respin only this one Cheers, Luca > > Cheers, > > -- > Julien Grall
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi, On 22/04/2024 10:26, Michal Orzel wrote: On 22/04/2024 10:07, Luca Fancellu wrote: Hi Michal, +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size ) +{ +u64 start = dt_read_number(cells, addrcells); We should no longer use Linux derived types like u64. Use uint64_t. +u64 size = dt_read_number(cells + addrcells, sizecells); + +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); i is unsigned so the correct format specifier should be %u Right, should have been more careful when copying the code from above +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, +__be32 *reg, int *nr_cells, +int addrcells, int sizecells) +{ +const struct membanks *mem = >shm_mem.common; +unsigned int i; +__be32 *cells; + +BUG_ON(!nr_cells || !reg); + +cells = [*nr_cells]; +for ( i = 0; i < mem->nr_banks; i++ ) +{ +u64 start = mem->bank[i].start; ditto Will fix, here paddr_t should be ok isn’t it? yes Rest LGTM: Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. I don't think there is a need to respin the whole series just for these fixes. You should wait for the committers opinion. AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned. If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case. Cheers, -- Julien Grall
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
On 22/04/2024 10:07, Luca Fancellu wrote: > > > Hi Michal, > >>> +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += >>> reg_size ) >>> +{ >>> +u64 start = dt_read_number(cells, addrcells); >> We should no longer use Linux derived types like u64. Use uint64_t. >> >>> +u64 size = dt_read_number(cells + addrcells, sizecells); >>> + >>> +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", >>> + i, start, start + size); >> i is unsigned so the correct format specifier should be %u > > Right, should have been more careful when copying the code from above > >>> >>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, >>> +__be32 *reg, int *nr_cells, >>> +int addrcells, int sizecells) >>> +{ >>> +const struct membanks *mem = >shm_mem.common; >>> +unsigned int i; >>> +__be32 *cells; >>> + >>> +BUG_ON(!nr_cells || !reg); >>> + >>> +cells = [*nr_cells]; >>> +for ( i = 0; i < mem->nr_banks; i++ ) >>> +{ >>> +u64 start = mem->bank[i].start; >> ditto > > Will fix, here paddr_t should be ok isn’t it? yes > >> >> Rest LGTM: >> Reviewed-by: Michal Orzel > > Thanks, I will send the next one shortly. I don't think there is a need to respin the whole series just for these fixes. You should wait for the committers opinion. ~Michal
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Michal, >> +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += >> reg_size ) >> +{ >> +u64 start = dt_read_number(cells, addrcells); > We should no longer use Linux derived types like u64. Use uint64_t. > >> +u64 size = dt_read_number(cells + addrcells, sizecells); >> + >> +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", >> + i, start, start + size); > i is unsigned so the correct format specifier should be %u Right, should have been more careful when copying the code from above >> >> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, >> +__be32 *reg, int *nr_cells, >> +int addrcells, int sizecells) >> +{ >> +const struct membanks *mem = >shm_mem.common; >> +unsigned int i; >> +__be32 *cells; >> + >> +BUG_ON(!nr_cells || !reg); >> + >> +cells = [*nr_cells]; >> +for ( i = 0; i < mem->nr_banks; i++ ) >> +{ >> +u64 start = mem->bank[i].start; > ditto Will fix, here paddr_t should be ok isn’t it? > > Rest LGTM: > Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. Cheers, Luca
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Luca, On 18/04/2024 09:36, 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. > > Given that now make_memory_node needs a parameter 'struct kernel_info' > in order to call the new function shm_mem_node_fill_reg_range, > take the occasion to remove the unused struct domain parameter. > > Signed-off-by: Luca Fancellu > --- > v3: > - removed previous patch that was merging the intervals, rebase >changes. > v2: > - try to use make_memory_node, don't add overlapping ranges of >memory, commit message changed. > v1: > - new patch > --- > --- > xen/arch/arm/dom0less-build.c | 2 +- > xen/arch/arm/domain_build.c | 34 + > xen/arch/arm/include/asm/domain_build.h | 2 +- > xen/arch/arm/include/asm/static-shmem.h | 15 +++ > xen/arch/arm/static-shmem.c | 23 + > 5 files changed, 63 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 51cf03221d56..74f053c242f4 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, > struct kernel_info *kinfo) > if ( ret ) > goto err; > > -ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, > +ret = make_memory_node(kinfo, addrcells, sizecells, > kernel_info_get_mem(kinfo)); > if ( ret ) > goto err; > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 68532ddc084c..15f888169c4e 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -756,15 +756,14 @@ int __init domain_fdt_begin_node(void *fdt, const char > *name, uint64_t unit) > return fdt_begin_node(fdt, buf); > } > > -int __init make_memory_node(const struct domain *d, > -void *fdt, > -int addrcells, int sizecells, > -const struct membanks *mem) > +int __init make_memory_node(const struct kernel_info *kinfo, int addrcells, > +int sizecells, const struct membanks *mem) > { > +void *fdt = kinfo->fdt; > unsigned int i; > int res, reg_size = addrcells + sizecells; > int nr_cells = 0; > -__be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; > +__be32 reg[DT_MEM_NODE_REG_RANGE_SIZE]; > __be32 *cells; > > if ( mem->nr_banks == 0 ) > @@ -797,14 +796,28 @@ int __init make_memory_node(const struct domain *d, > if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN ) > continue; > > -dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", > - i, start, start + size); > - > nr_cells += reg_size; > BUG_ON(nr_cells >= ARRAY_SIZE(reg)); > dt_child_set_range(, addrcells, sizecells, start, size); > } > > +/* > + * static shared memory banks need to be listed as /memory node, so when > + * this function is handling the normal memory, add the banks. > + */ > +if ( mem == kernel_info_get_mem(kinfo) ) > +shm_mem_node_fill_reg_range(kinfo, reg, _cells, addrcells, > +sizecells); > + > +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size > ) > +{ > +u64 start = dt_read_number(cells, addrcells); We should no longer use Linux derived types like u64. Use uint64_t. > +u64 size = dt_read_number(cells + addrcells, sizecells); > + > +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", > + i, start, start + size); i is unsigned so the correct format specifier should be %u > +} > + > dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells); > > res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg)); > @@ -1783,7 +1796,7 @@ static int __init handle_node(struct domain *d, struct > kernel_info *kinfo, > if ( res ) > return res; > > -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, > +res = make_memory_node(kinfo, addrcells, sizecells, > kernel_info_get_mem(kinfo)); > if ( res ) > return res; > @@ -1794,8 +1807,7 @@ static int __init handle_node(struct domain *d, struct > kernel_info *kinfo, > */ > if ( reserved_mem->nr_banks > 0 ) > { > -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, > - reserved_mem); > +res = make_memory_node(kinfo, addrcells, sizecells, > reserved_mem); > if ( res ) > return res; > } > diff --git a/xen/arch/arm/include/asm/domain_build.h > b/xen/arch/arm/include/asm/domain_build.h > index