| Message ID | 20230130040535.188236-2-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory region overlap check in device tree | expand |
On Mon, 30 Jan 2023, Henry Wang wrote: > As we are having more and more types of static region, and all of > these static regions are defined in bootinfo.reserved_mem, it is > necessary to add the overlap check of reserved memory regions in Xen, > because such check will help user to identify the misconfiguration in > the device tree at the early stage of boot time. > > Currently we have 3 types of static region, namely > (1) static memory > (2) static heap > (3) static shared memory > > (1) and (2) are parsed by the function `device_tree_get_meminfo()` and > (3) is parsed using its own logic. All of parsed information of these > types will be stored in `struct meminfo`. > > Therefore, to unify the overlap checking logic for all of these types, > this commit firstly introduces a helper `meminfo_overlap_check()` and > a function `check_reserved_regions_overlap()` to check if an input > physical address range is overlapping with the existing memory regions > defined in bootinfo. After that, use `check_reserved_regions_overlap()` > in `device_tree_get_meminfo()` to do the overlap check of (1) and (2) > and replace the original overlap check of (3) with > `check_reserved_regions_overlap()`. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > v2 -> v3: > 1. Use "[start, end]" format in printk error message. > 2. Change the return type of helper functions to bool. > 3. Use 'start' and 'size' in helper functions to describe a region. > v1 -> v2: > 1. Split original `overlap_check()` to `meminfo_overlap_check()`. > 2. Rework commit message. > --- > xen/arch/arm/bootfdt.c | 13 +++++----- > xen/arch/arm/include/asm/setup.h | 2 ++ > xen/arch/arm/setup.c | 41 ++++++++++++++++++++++++++++++++ > 3 files changed, 49 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 0085c28d74..e2f6c7324b 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node, > for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) > { > device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); > + if ( mem == &bootinfo.reserved_mem && > + check_reserved_regions_overlap(start, size) ) > + return -EINVAL; > /* Some DT may describe empty bank, ignore them */ > if ( !size ) > continue; > @@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node, > return -EINVAL; > } ~ > - if ( (end <= mem->bank[i].start) || (paddr >= bank_end) ) > + if ( check_reserved_regions_overlap(paddr, size) ) > + return -EINVAL; > + else > { > if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) > continue; > @@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node, > return -EINVAL; > } > } > - else > - { > - printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n", > - mem->bank[i].start, bank_end); > - return -EINVAL; > - } > } > } > > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index a926f30a2b..f0592370ea 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e, > size_t boot_fdt_info(const void *fdt, paddr_t paddr); > const char *boot_fdt_cmdline(const void *fdt); > > +bool check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size); > + > struct bootmodule *add_boot_module(bootmodule_kind kind, > paddr_t start, paddr_t size, bool domU); > struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 1f26f67b90..636604aafa 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -261,6 +261,32 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, > cb(s, e); > } > > +static bool __init meminfo_overlap_check(struct meminfo *meminfo, > + paddr_t region_start, > + paddr_t region_size) > +{ > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > + paddr_t region_end = region_start + region_size; > + unsigned int i, bank_num = meminfo->nr_banks; > + > + for ( i = 0; i < bank_num; i++ ) > + { > + bank_start = meminfo->bank[i].start; > + bank_end = bank_start + meminfo->bank[i].size; > + > + if ( region_end <= bank_start || region_start >= bank_end ) > + continue; > + else > + { > + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n", > + region_start, region_end, i, bank_start, bank_end); > + return true; > + } > + } > + > + return false; > +} > + > void __init fw_unreserved_regions(paddr_t s, paddr_t e, > void (*cb)(paddr_t, paddr_t), > unsigned int first) > @@ -271,7 +297,22 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e, > cb(s, e); > } > > +/* > + * Given an input physical address range, check if this range is overlapping > + * with the existing reserved memory regions defined in bootinfo. > + * Return true if the input physical address range is overlapping with any > + * existing reserved memory regions, otherwise false. > + */ > +bool __init check_reserved_regions_overlap(paddr_t region_start, > + paddr_t region_size) > +{ > + /* Check if input region is overlapping with bootinfo.reserved_mem banks */ > + if ( meminfo_overlap_check(&bootinfo.reserved_mem, > + region_start, region_size) ) > + return true; > > + return false; > +} > > struct bootmodule __init *add_boot_module(bootmodule_kind kind, > paddr_t start, paddr_t size, > -- > 2.25.1 >
Hi Henry, On 30/01/2023 04:05, Henry Wang wrote: > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index a926f30a2b..f0592370ea 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e, > size_t boot_fdt_info(const void *fdt, paddr_t paddr); > const char *boot_fdt_cmdline(const void *fdt); > > +bool check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size); > + > struct bootmodule *add_boot_module(bootmodule_kind kind, > paddr_t start, paddr_t size, bool domU); > struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 1f26f67b90..636604aafa 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -261,6 +261,32 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, > cb(s, e); > } > > +static bool __init meminfo_overlap_check(struct meminfo *meminfo, > + paddr_t region_start, > + paddr_t region_size) So the interface looks nicer but now... > +{ > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > + paddr_t region_end = region_start + region_size; > + unsigned int i, bank_num = meminfo->nr_banks; > + > + for ( i = 0; i < bank_num; i++ ) > + { > + bank_start = meminfo->bank[i].start; > + bank_end = bank_start + meminfo->bank[i].size; > + > + if ( region_end <= bank_start || region_start >= bank_end ) ... it clearly shows how this check would be wrong when either the bank or the region is at the end of the address space. You may say it doesn't overlap when it could (e.g. when region_end < region_start). That said, unless we rework 'bank', we would not properly solve the problem. But that's likely a bigger piece of work and not something I would request. So for now, I would suggest to add a comment. Stefano, what do you think? > + continue; > + else > + { > + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n", ']' usually mean inclusive. But here, 'end' is exclusive. So you want '['. This could be fixed on commit. BTW, the same comments applies for the second patch. > + region_start, region_end, i, bank_start, bank_end); > + return true; > + } > + } > + > + return false; > +} > + > void __init fw_unreserved_regions(paddr_t s, paddr_t e, > void (*cb)(paddr_t, paddr_t), > unsigned int first) > @@ -271,7 +297,22 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e, > cb(s, e); > } > > +/* > + * Given an input physical address range, check if this range is overlapping > + * with the existing reserved memory regions defined in bootinfo. > + * Return true if the input physical address range is overlapping with any > + * existing reserved memory regions, otherwise false. > + */ > +bool __init check_reserved_regions_overlap(paddr_t region_start, > + paddr_t region_size) > +{ > + /* Check if input region is overlapping with bootinfo.reserved_mem banks */ > + if ( meminfo_overlap_check(&bootinfo.reserved_mem, > + region_start, region_size) ) > + return true; > > + return false; > +} > > struct bootmodule __init *add_boot_module(bootmodule_kind kind, > paddr_t start, paddr_t size, Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for > bootinfo.reserved_mem > > Hi Henry, > > > +{ > > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > > + paddr_t region_end = region_start + region_size; > > + unsigned int i, bank_num = meminfo->nr_banks; > > + > > + for ( i = 0; i < bank_num; i++ ) > > + { > > + bank_start = meminfo->bank[i].start; > > + bank_end = bank_start + meminfo->bank[i].size; > > + > > + if ( region_end <= bank_start || region_start >= bank_end ) > > ... it clearly shows how this check would be wrong when either the bank > or the region is at the end of the address space. You may say it doesn't > overlap when it could (e.g. when region_end < region_start). Here do you mean if the region is at the end of the addr space, "region_start + region_end" will overflow and cause region_end < region_start? If so... > > That said, unless we rework 'bank', we would not properly solve the > problem. But that's likely a bigger piece of work and not something I > would request. > > So for now, I would suggest to add a comment. Stefano, what do you think? ...I am not really sure if simply adding a comment here would help, because when the overflow happens, we are already doomed because of the messed-up device tree. Would adding a `BUG_ON(region_end < region_start)` make sense to you? > > > + continue; > > + else > > + { > > + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with > bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n", > > ']' usually mean inclusive. But here, 'end' is exclusive. So you want '['. Oh, now I understand the misunderstanding in our communication in v1: I didn't know '[' means exclusive because I was educated to use ')' [1] so I thought you meant inclusive. Sorry for this. To keep consistency, may I use ')' here? Because I think this is the current way in the code base, for example see: xen/include/xen/numa.h L99: [*start, *end) xen/drivers/passthrough/amd/iommu_acpi.c L177: overlap [%lx,%lx) > > This could be fixed on commit. > > BTW, the same comments applies for the second patch. I will fix this patch and #2 in v4. [1] https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 31/01/2023 02:25, Henry Wang wrote: > Hi Julien, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for >> bootinfo.reserved_mem >> >> Hi Henry, >> >>> +{ >>> + paddr_t bank_start = INVALID_PADDR, bank_end = 0; >>> + paddr_t region_end = region_start + region_size; >>> + unsigned int i, bank_num = meminfo->nr_banks; >>> + >>> + for ( i = 0; i < bank_num; i++ ) >>> + { >>> + bank_start = meminfo->bank[i].start; >>> + bank_end = bank_start + meminfo->bank[i].size; >>> + >>> + if ( region_end <= bank_start || region_start >= bank_end ) >> >> ... it clearly shows how this check would be wrong when either the bank >> or the region is at the end of the address space. You may say it doesn't >> overlap when it could (e.g. when region_end < region_start). > > Here do you mean if the region is at the end of the addr space, > "region_start + region_end" will overflow and cause > region_end < region_start? If so... Yes. > >> >> That said, unless we rework 'bank', we would not properly solve the >> problem. But that's likely a bigger piece of work and not something I >> would request. >> >> So for now, I would suggest to add a comment. Stefano, what do you think? > > ...I am not really sure if simply adding a comment here would help, > because when the overflow happens, we are already doomed because > of the messed-up device tree. Not necessarily. This could happen if the region is right at the top of the address (e.g. finishing at 2^64 - 1). As the 'end' is exclusive, then it would be equal to 0. I think this is less likely for arm64, but this could happen for 32-bit Arm as we will allow the admin to reduce paddr_t from 64-bit to 32-bit. > > Would adding a `BUG_ON(region_end < region_start)` make sense to you? No for the reason I stated above. > >> >>> + continue; >>> + else >>> + { >>> + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with >> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n", >> >> ']' usually mean inclusive. But here, 'end' is exclusive. So you want '['. > > Oh, now I understand the misunderstanding in our communication in v1: > I didn't know '[' means exclusive because I was educated to use ')' [1] so I > thought you meant inclusive. Sorry for this. No worries. That might be only me using [ and ) interchangeably :). > > To keep consistency, may I use ')' here? Because I think this is the current > way in the code base, for example see: > xen/include/xen/numa.h L99: [*start, *end) > xen/drivers/passthrough/amd/iommu_acpi.c L177: overlap [%lx,%lx) I am fine with that. >> BTW, the same comments applies for the second patch. > > I will fix this patch and #2 in v4. I am happy to deal with it on commit if you want. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for > bootinfo.reserved_mem > > I will fix this patch and #2 in v4. > > I am happy to deal with it on commit if you want. Including adding the comment for both patches? This would be wonderful and very nice of you to do that. But if your time is limited I am also more than happy to respin the patch (probably even with Stefano's Reviewed-by tag if he is ok with it) to reduce your burden. That said, if I need to respin the patch, it would be good to get some hints about the wording of the comments to avoid another v+1 just because of my inaccurate wording :) Thanks very much! Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 31/01/2023 09:30, Henry Wang wrote: > Hi Julien, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH v3 1/3] xen/arm: Add memory overlap check for >> bootinfo.reserved_mem >>> I will fix this patch and #2 in v4. >> >> I am happy to deal with it on commit if you want. > > Including adding the comment for both patches? This would be wonderful > and very nice of you to do that. But if your time is limited I am also more > than happy to respin the patch (probably even with Stefano's Reviewed-by > tag if he is ok with it) to reduce your burden. That said, if I need to respin the > patch, it would be good to get some hints about the wording of the comments > to avoid another v+1 just because of my inaccurate wording :) Good idea. My suggestion would be: TODO: '*_end' could be 0 if the bank/region is at the end of the physical address space. This is for now not handled as it requires more rework. Cheers,
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 0085c28d74..e2f6c7324b 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node, for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ ) { device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); + if ( mem == &bootinfo.reserved_mem && + check_reserved_regions_overlap(start, size) ) + return -EINVAL; /* Some DT may describe empty bank, ignore them */ if ( !size ) continue; @@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node, return -EINVAL; } - if ( (end <= mem->bank[i].start) || (paddr >= bank_end) ) + if ( check_reserved_regions_overlap(paddr, size) ) + return -EINVAL; + else { if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) continue; @@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node, return -EINVAL; } } - else - { - printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n", - mem->bank[i].start, bank_end); - return -EINVAL; - } } } diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index a926f30a2b..f0592370ea 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e, size_t boot_fdt_info(const void *fdt, paddr_t paddr); const char *boot_fdt_cmdline(const void *fdt); +bool check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size); + struct bootmodule *add_boot_module(bootmodule_kind kind, paddr_t start, paddr_t size, bool domU); struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 1f26f67b90..636604aafa 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -261,6 +261,32 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, cb(s, e); } +static bool __init meminfo_overlap_check(struct meminfo *meminfo, + paddr_t region_start, + paddr_t region_size) +{ + paddr_t bank_start = INVALID_PADDR, bank_end = 0; + paddr_t region_end = region_start + region_size; + unsigned int i, bank_num = meminfo->nr_banks; + + for ( i = 0; i < bank_num; i++ ) + { + bank_start = meminfo->bank[i].start; + bank_end = bank_start + meminfo->bank[i].size; + + if ( region_end <= bank_start || region_start >= bank_end ) + continue; + else + { + printk("Region: [%#"PRIpaddr", %#"PRIpaddr"] overlapping with bank[%u]: [%#"PRIpaddr", %#"PRIpaddr"]\n", + region_start, region_end, i, bank_start, bank_end); + return true; + } + } + + return false; +} + void __init fw_unreserved_regions(paddr_t s, paddr_t e, void (*cb)(paddr_t, paddr_t), unsigned int first) @@ -271,7 +297,22 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e, cb(s, e); } +/* + * Given an input physical address range, check if this range is overlapping + * with the existing reserved memory regions defined in bootinfo. + * Return true if the input physical address range is overlapping with any + * existing reserved memory regions, otherwise false. + */ +bool __init check_reserved_regions_overlap(paddr_t region_start, + paddr_t region_size) +{ + /* Check if input region is overlapping with bootinfo.reserved_mem banks */ + if ( meminfo_overlap_check(&bootinfo.reserved_mem, + region_start, region_size) ) + return true; + return false; +} struct bootmodule __init *add_boot_module(bootmodule_kind kind, paddr_t start, paddr_t size,
As we are having more and more types of static region, and all of these static regions are defined in bootinfo.reserved_mem, it is necessary to add the overlap check of reserved memory regions in Xen, because such check will help user to identify the misconfiguration in the device tree at the early stage of boot time. Currently we have 3 types of static region, namely (1) static memory (2) static heap (3) static shared memory (1) and (2) are parsed by the function `device_tree_get_meminfo()` and (3) is parsed using its own logic. All of parsed information of these types will be stored in `struct meminfo`. Therefore, to unify the overlap checking logic for all of these types, this commit firstly introduces a helper `meminfo_overlap_check()` and a function `check_reserved_regions_overlap()` to check if an input physical address range is overlapping with the existing memory regions defined in bootinfo. After that, use `check_reserved_regions_overlap()` in `device_tree_get_meminfo()` to do the overlap check of (1) and (2) and replace the original overlap check of (3) with `check_reserved_regions_overlap()`. Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- v2 -> v3: 1. Use "[start, end]" format in printk error message. 2. Change the return type of helper functions to bool. 3. Use 'start' and 'size' in helper functions to describe a region. v1 -> v2: 1. Split original `overlap_check()` to `meminfo_overlap_check()`. 2. Rework commit message. --- xen/arch/arm/bootfdt.c | 13 +++++----- xen/arch/arm/include/asm/setup.h | 2 ++ xen/arch/arm/setup.c | 41 ++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-)