From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Date: Wed, 31 May 2023 15:02:18 +0200 Subject: [PATCH 12/17] lib: utils/fdt: Use heap in FDT domain parsing In-Reply-To: <20230425123230.3943447-13-apatel@ventanamicro.com> References: <20230425123230.3943447-1-apatel@ventanamicro.com> <20230425123230.3943447-13-apatel@ventanamicro.com> Message-ID: <20230531-98638823b710caec91cb1374@orel> List-Id: To: opensbi@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Apr 25, 2023 at 06:02:25PM +0530, Anup Patel wrote: > Let's use heap allocation in FDT domain parsing instead of using > a fixed size global array. > > Signed-off-by: Anup Patel > --- > lib/utils/fdt/fdt_domain.c | 111 +++++++++++++++++++++++-------------- > 1 file changed, 68 insertions(+), 43 deletions(-) > > diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c > index bb6d17d..c4aed4d 100644 > --- a/lib/utils/fdt/fdt_domain.c > +++ b/lib/utils/fdt/fdt_domain.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -219,14 +220,11 @@ skip_device_disable: > fdt_nop_node(fdt, poffset); > } > > -#define FDT_DOMAIN_MAX_COUNT 8 > -#define FDT_DOMAIN_REGION_MAX_COUNT 16 Why not keep this define since we still use 16 below when allocating dom->regions / max_regions? > - > -static u32 fdt_domains_count; > -static struct sbi_domain fdt_domains[FDT_DOMAIN_MAX_COUNT]; > -static struct sbi_hartmask fdt_masks[FDT_DOMAIN_MAX_COUNT]; > -static struct sbi_domain_memregion > - fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT + 1]; > +struct parse_region_data { > + struct sbi_domain *dom; > + u32 region_count; > + u32 max_regions; > +}; > > static int __fdt_parse_region(void *fdt, int domain_offset, > int region_offset, u32 region_access, > @@ -236,7 +234,7 @@ static int __fdt_parse_region(void *fdt, int domain_offset, > u32 val32; > u64 val64; > const u32 *val; > - u32 *region_count = opaque; > + struct parse_region_data *preg = opaque; > struct sbi_domain_memregion *region; > > /* > @@ -252,9 +250,9 @@ static int __fdt_parse_region(void *fdt, int domain_offset, > return SBI_EINVAL; > > /* Find next region of the domain */ > - if (FDT_DOMAIN_REGION_MAX_COUNT <= *region_count) > - return SBI_EINVAL; > - region = &fdt_regions[fdt_domains_count][*region_count]; > + if (preg->max_regions <= preg->region_count) > + return SBI_ENOSPC; > + region = &preg->dom->regions[preg->region_count]; > > /* Read "base" DT property */ > val = fdt_getprop(fdt, region_offset, "base", &len); > @@ -278,7 +276,7 @@ static int __fdt_parse_region(void *fdt, int domain_offset, > if (fdt_get_property(fdt, region_offset, "mmio", NULL)) > region->flags |= SBI_DOMAIN_MEMREGION_MMIO; > > - (*region_count)++; > + preg->region_count++; > > return 0; > } > @@ -291,16 +289,30 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > struct sbi_domain *dom; > struct sbi_hartmask *mask; > struct sbi_hartmask assign_mask; > + struct parse_region_data preg; > int *cold_domain_offset = opaque; > - struct sbi_domain_memregion *reg, *regions; > - int i, err, len, cpus_offset, cpu_offset, doffset; > + struct sbi_domain_memregion *reg; > + int i, err = 0, len, cpus_offset, cpu_offset, doffset; > > - /* Sanity check on maximum domains we can handle */ > - if (FDT_DOMAIN_MAX_COUNT <= fdt_domains_count) > - return SBI_EINVAL; > - dom = &fdt_domains[fdt_domains_count]; > - mask = &fdt_masks[fdt_domains_count]; > - regions = &fdt_regions[fdt_domains_count][0]; > + dom = sbi_zalloc(sizeof(*dom)); > + if (!dom) > + return SBI_ENOMEM; > + > + dom->regions = sbi_calloc(sizeof(*dom->regions), 16 + 1); > + if (!dom->regions) { > + sbi_free(dom); > + return SBI_ENOMEM; > + } > + preg.dom = dom; > + preg.region_count = 0; > + preg.max_regions = 16; > + > + mask = sbi_zalloc(sizeof(*mask)); > + if (!mask) { > + sbi_free(dom->regions); > + sbi_free(dom); > + return SBI_ENOMEM; > + } Can add two more labels above fail_free_all to use in above two failure cases as well. > > /* Read DT node name */ > strncpy(dom->name, fdt_get_name(fdt, domain_offset, NULL), > @@ -316,12 +328,14 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > for (i = 0; i < len; i++) { > cpu_offset = fdt_node_offset_by_phandle(fdt, > fdt32_to_cpu(val[i])); > - if (cpu_offset < 0) > - return cpu_offset; > + if (cpu_offset < 0) { > + err = cpu_offset; > + goto fail_free_all; > + } > > err = fdt_parse_hart_id(fdt, cpu_offset, &val32); > if (err) > - return err; > + goto fail_free_all; > > if (!fdt_node_is_enabled(fdt, cpu_offset)) > continue; > @@ -331,14 +345,10 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > } > > /* Setup memregions from DT */ > - val32 = 0; > - memset(regions, 0, > - sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT + 1)); > - dom->regions = regions; > - err = fdt_iterate_each_memregion(fdt, domain_offset, &val32, > + err = fdt_iterate_each_memregion(fdt, domain_offset, &preg, > __fdt_parse_region); > if (err) > - return err; > + goto fail_free_all; > > /* > * Copy over root domain memregions which don't allow > @@ -354,9 +364,11 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE) || > (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)) > continue; > - if (FDT_DOMAIN_REGION_MAX_COUNT <= val32) > - return SBI_EINVAL; > - memcpy(®ions[val32++], reg, sizeof(*reg)); > + if (preg.max_regions <= preg.region_count) { > + err = SBI_EINVAL; > + goto fail_free_all; > + } > + memcpy(&dom->regions[preg.region_count++], reg, sizeof(*reg)); > } > dom->fw_region_inited = root.fw_region_inited; > > @@ -427,8 +439,10 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > > /* Find /cpus DT node */ > cpus_offset = fdt_path_offset(fdt, "/cpus"); > - if (cpus_offset < 0) > - return cpus_offset; > + if (cpus_offset < 0) { > + err = cpus_offset; > + goto fail_free_all; > + } > > /* HART to domain assignment mask based on CPU DT nodes */ > sbi_hartmask_clear_all(&assign_mask); > @@ -444,22 +458,33 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque) > continue; > > val = fdt_getprop(fdt, cpu_offset, "opensbi-domain", &len); > - if (!val || len < 4) > - return SBI_EINVAL; > + if (!val || len < 4) { > + err = SBI_EINVAL; > + goto fail_free_all; > + } > > doffset = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val)); > - if (doffset < 0) > - return doffset; > + if (doffset < 0) { > + err = doffset; > + goto fail_free_all; > + } > > if (doffset == domain_offset) > sbi_hartmask_set_hart(val32, &assign_mask); > } > > - /* Increment domains count */ > - fdt_domains_count++; > - > /* Register the domain */ > - return sbi_domain_register(dom, &assign_mask); > + err = sbi_domain_register(dom, &assign_mask); > + if (err) > + goto fail_free_all; > + > + return 0; > + > +fail_free_all: > + sbi_free(dom->regions); > + sbi_free(mask); > + sbi_free(dom); > + return err; > } > > int fdt_domains_populate(void *fdt) > -- > 2.34.1 > Otherwise, Reviewed-by: Andrew Jones