| Message ID | 20230911040442.2541398-10-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory PART I | expand |
Hi Penny, On 11/09/2023 06:04, Penny Zheng wrote: > > > In case there is a /reserved-memory node already present in the host dtb, > current Xen codes would create yet another /reserved-memory node specially s/codes/code/ > for the static shm in Dom0 Device Tree. > > Xen will use write_properties() to copy the reserved memory nodes from host dtb > to Dom0 FDT, so we want to insert the shm node along with the copying. > And avoiding duplication, we add a checking before make_resv_memory_node(). > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > v3 -> v4: > new commit > --- > xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++--- > xen/arch/arm/include/asm/kernel.h | 2 ++ > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 02d903be78..dad234e4b5 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo, > return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment); > } > > +static int __init make_shm_memory_node(const struct domain *d, > + void *fdt, > + int addrcells, int sizecells, > + const struct kernel_info *kinfo); > static int __init write_properties(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, > } > } > > + if ( dt_node_path_is_equal(node, "/reserved-memory") ) > + { > + kinfo->resv_mem = true; kinfo structure is used to store per-domain parameters and presence of reserved memory in host dtb does not fit into this. Moreover, there is no need to introduce yet another flag for that given that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks to check if there are reserved regions. > + > + /* shared memory provided. */ > + if ( kinfo->shminfo.nr_banks != 0 ) > + { > + uint32_t addrcells = dt_n_addr_cells(node), > + sizecells = dt_n_size_cells(node); > + > + res = make_shm_memory_node(d, kinfo->fdt, > + addrcells, sizecells, kinfo); I haven't yet looked at previous patches but does it make sense to request passing both kinfo->fdt and kinfo? IMO, the latter would be sufficient. This would apply to other functions you modified. > + if ( res ) > + return res; > + } > + } > + > return 0; > } > > @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > return res; > } > > - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); > - if ( res ) > - return res; > + /* Avoid duplicate /reserved-memory nodes in Device Tree */ > + if ( !kinfo->resv_mem ) No need for a new flag as mentioned above. Just before this line of code there is a check: if ( bootinfo.reserved_mem.nr_banks > 0 ) { res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &bootinfo.reserved_mem); if ( res ) return res; } so you can just append the following: else { res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); if ( res ) return res; } to achieve the same without a need for a new flag. ~Michal
Hi Michal On 2023/9/11 17:40, Michal Orzel wrote: > Hi Penny, > > On 11/09/2023 06:04, Penny Zheng wrote: >> >> >> In case there is a /reserved-memory node already present in the host dtb, >> current Xen codes would create yet another /reserved-memory node specially > s/codes/code/ > >> for the static shm in Dom0 Device Tree. >> >> Xen will use write_properties() to copy the reserved memory nodes from host dtb >> to Dom0 FDT, so we want to insert the shm node along with the copying. >> And avoiding duplication, we add a checking before make_resv_memory_node(). >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> >> --- >> v3 -> v4: >> new commit >> --- >> xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++--- >> xen/arch/arm/include/asm/kernel.h | 2 ++ >> 2 files changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 02d903be78..dad234e4b5 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >> return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment); >> } >> >> +static int __init make_shm_memory_node(const struct domain *d, >> + void *fdt, >> + int addrcells, int sizecells, >> + const struct kernel_info *kinfo); >> static int __init write_properties(struct domain *d, struct kernel_info *kinfo, >> const struct dt_device_node *node) >> { >> @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, >> } >> } >> >> + if ( dt_node_path_is_equal(node, "/reserved-memory") ) >> + { >> + kinfo->resv_mem = true; > kinfo structure is used to store per-domain parameters and presence of reserved memory in host dtb > does not fit into this. Moreover, there is no need to introduce yet another flag for that given > that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks to check if there are > reserved regions. > >> + >> + /* shared memory provided. */ >> + if ( kinfo->shminfo.nr_banks != 0 ) >> + { >> + uint32_t addrcells = dt_n_addr_cells(node), >> + sizecells = dt_n_size_cells(node); >> + >> + res = make_shm_memory_node(d, kinfo->fdt, >> + addrcells, sizecells, kinfo); > I haven't yet looked at previous patches but does it make sense to request passing both kinfo->fdt and kinfo? > IMO, the latter would be sufficient. This would apply to other functions you modified. > yes, the latter would be sufficient. I'll fix it in next version. >> + if ( res ) >> + return res; >> + } >> + } >> + >> return 0; >> } >> >> @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, >> return res; >> } >> >> - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); >> - if ( res ) >> - return res; >> + /* Avoid duplicate /reserved-memory nodes in Device Tree */ >> + if ( !kinfo->resv_mem ) > No need for a new flag as mentioned above. Just before this line of code there is a check: > if ( bootinfo.reserved_mem.nr_banks > 0 ) > { > res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, > &bootinfo.reserved_mem); > if ( res ) > return res; > } > so you can just append the following: > else > { > res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); > if ( res ) > return res; > } > to achieve the same without a need for a new flag. Right now, bootinfo.reserved_mem is not only containing reserved regions described in host FDT /reserved-memory, but also the reserved ones for domain only, like in xen,static-mem = <xxx>. So simply with non-zero bootinfo.reserved_mem.nr_banks, we couldn't tell whether we had a /reserved-memory node in host FDT. I agree that kinfo is not a good place to store whether host FDT has a /reserved-memory node. Maybe bootinfo.has_resv_memory_node? > > ~Michal
Hi Penny, On 11/09/2023 12:39, Penny Zheng wrote: > > > Hi Michal > > On 2023/9/11 17:40, Michal Orzel wrote: >> Hi Penny, >> >> On 11/09/2023 06:04, Penny Zheng wrote: >>> >>> >>> In case there is a /reserved-memory node already present in the host dtb, >>> current Xen codes would create yet another /reserved-memory node specially >> s/codes/code/ >> >>> for the static shm in Dom0 Device Tree. >>> >>> Xen will use write_properties() to copy the reserved memory nodes from host dtb >>> to Dom0 FDT, so we want to insert the shm node along with the copying. >>> And avoiding duplication, we add a checking before make_resv_memory_node(). >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> >>> --- >>> v3 -> v4: >>> new commit >>> --- >>> xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++--- >>> xen/arch/arm/include/asm/kernel.h | 2 ++ >>> 2 files changed, 30 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 02d903be78..dad234e4b5 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >>> return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment); >>> } >>> >>> +static int __init make_shm_memory_node(const struct domain *d, >>> + void *fdt, >>> + int addrcells, int sizecells, >>> + const struct kernel_info *kinfo); >>> static int __init write_properties(struct domain *d, struct kernel_info *kinfo, >>> const struct dt_device_node *node) >>> { >>> @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, >>> } >>> } >>> >>> + if ( dt_node_path_is_equal(node, "/reserved-memory") ) >>> + { >>> + kinfo->resv_mem = true; >> kinfo structure is used to store per-domain parameters and presence of reserved memory in host dtb >> does not fit into this. Moreover, there is no need to introduce yet another flag for that given >> that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks to check if there are >> reserved regions. >> >>> + >>> + /* shared memory provided. */ >>> + if ( kinfo->shminfo.nr_banks != 0 ) >>> + { >>> + uint32_t addrcells = dt_n_addr_cells(node), >>> + sizecells = dt_n_size_cells(node); >>> + >>> + res = make_shm_memory_node(d, kinfo->fdt, >>> + addrcells, sizecells, kinfo); >> I haven't yet looked at previous patches but does it make sense to request passing both kinfo->fdt and kinfo? >> IMO, the latter would be sufficient. This would apply to other functions you modified. >> > > yes, the latter would be sufficient. I'll fix it in next version. > > >>> + if ( res ) >>> + return res; >>> + } >>> + } >>> + >>> return 0; >>> } >>> >>> @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, >>> return res; >>> } >>> >>> - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); >>> - if ( res ) >>> - return res; >>> + /* Avoid duplicate /reserved-memory nodes in Device Tree */ >>> + if ( !kinfo->resv_mem ) >> No need for a new flag as mentioned above. Just before this line of code there is a check: >> if ( bootinfo.reserved_mem.nr_banks > 0 ) >> { >> res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, >> &bootinfo.reserved_mem); >> if ( res ) >> return res; >> } >> so you can just append the following: >> else >> { >> res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); >> if ( res ) >> return res; >> } >> to achieve the same without a need for a new flag. > > > Right now, bootinfo.reserved_mem is not only containing reserved regions > described in host FDT /reserved-memory, but also the reserved ones for > domain only, like in xen,static-mem = <xxx>. > So simply with non-zero bootinfo.reserved_mem.nr_banks, we couldn't tell > whether we had a /reserved-memory node in host FDT. > > I agree that kinfo is not a good place to store whether host FDT has a > /reserved-memory node. Maybe bootinfo.has_resv_memory_node? Yes, I see. So we have 2 options: 1) Introduce flag under bootinfo (just like static_heap) 2) Inside make_resv_memory_node(), do a check e.g.: for ( i = 0; i < bootinfo.reserved_mem.nr_banks && (bootinfo.reserved_mem.bank[i].type == MEMBANK_DEFAULT); i++ ); to see if there is a /reserved-memory node (i > 0). ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 02d903be78..dad234e4b5 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct kernel_info *kinfo, return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment); } +static int __init make_shm_memory_node(const struct domain *d, + void *fdt, + int addrcells, int sizecells, + const struct kernel_info *kinfo); static int __init write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo, } } + if ( dt_node_path_is_equal(node, "/reserved-memory") ) + { + kinfo->resv_mem = true; + + /* shared memory provided. */ + if ( kinfo->shminfo.nr_banks != 0 ) + { + uint32_t addrcells = dt_n_addr_cells(node), + sizecells = dt_n_size_cells(node); + + res = make_shm_memory_node(d, kinfo->fdt, + addrcells, sizecells, kinfo); + if ( res ) + return res; + } + } + return 0; } @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); - if ( res ) - return res; + /* Avoid duplicate /reserved-memory nodes in Device Tree */ + if ( !kinfo->resv_mem ) + { + res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo); + if ( res ) + return res; + } } res = fdt_end_node(kinfo->fdt); diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 590bc56f6c..d20c8bc2a8 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -38,6 +38,8 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; + /* Whether we have /reserved-memory node in host Device Tree */ + bool resv_mem; /* Static shared memory banks */ struct { unsigned int nr_banks;
In case there is a /reserved-memory node already present in the host dtb, current Xen codes would create yet another /reserved-memory node specially for the static shm in Dom0 Device Tree. Xen will use write_properties() to copy the reserved memory nodes from host dtb to Dom0 FDT, so we want to insert the shm node along with the copying. And avoiding duplication, we add a checking before make_resv_memory_node(). Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v3 -> v4: new commit --- xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++--- xen/arch/arm/include/asm/kernel.h | 2 ++ 2 files changed, 30 insertions(+), 3 deletions(-)