| Message ID | 20221205025753.2178965-2-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory region overlap check in device tree | expand |
On Mon, 5 Dec 2022, 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. Therefore, to unify the checking logic for all of these > types of static region, this commit firstly introduces a helper > `check_reserved_regions_overlap()` to check if an input physical > address range is overlapping with the existing reserved memory regions > defined in bootinfo. After that, use this helper in > `device_tree_get_meminfo()` to do the overlap check of (1) and (2) > and replace the original overlap check of (3) with this new helper. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> I wonder if the check should only be done #ifdef DEBUG. The idea would be that a given static configuration should be validated and corrected before going into production. By the time you go in production, it is too late to do checks anyway. Especially the panic below. Julien, Bertrand, what do you think about this? > --- > xen/arch/arm/bootfdt.c | 13 ++++---- > xen/arch/arm/include/asm/setup.h | 2 ++ > xen/arch/arm/setup.c | 52 ++++++++++++++++++++++++++++++++ > 3 files changed, 60 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 6014c0f852..b31379b9ac 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -91,6 +91,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; > @@ -485,7 +488,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; > @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); > > +int 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 4395640019..94d232605e 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, > cb(s, e); > } > > +static int __init overlap_check(void *bootinfo_type, > + paddr_t region_start, paddr_t region_end) > +{ > + unsigned int i, num = 0; > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > + char *type_str = "NONAME"; > + > + if ( bootinfo_type == &bootinfo.reserved_mem ) > + { > + num = bootinfo.reserved_mem.nr_banks; > + type_str = "reserved_mem"; > + } > + else > + panic("Invalid bootinfo type passed to overlap check\n"); > + > + for ( i = 0; i < num; i++ ) > + { > + if ( bootinfo_type == &bootinfo.reserved_mem ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_end = bank_start + bootinfo.reserved_mem.bank[i].size; > + } > + > + if ( region_end <= bank_start || region_start >= bank_end ) > + continue; > + else > + { > + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", > + type_str, region_start, region_end, i, bank_start, bank_end); > + return -EINVAL; > + } > + } > + > + return 0; > +} As much as I dislike MACROs in general I think this function should be written as a MACRO so that we can write it once for all use cases. The below in not compiled and not tested, just for explanation purposes. Look how much simpler the code becomes. #define overlap_check(bootinfo, \ num, \ region_start, \ region_end) \ ({ \ unsigned int i, ret; \ paddr_t bank_start = INVALID_PADDR, bank_end = 0; \ \ for ( i = 0; i < num; i++ ) \ { \ bank_start = bootinfo->start; \ bank_end = bank_start + bootinfo->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); \ ret = -EINVAL; \ break; \ } \ } \ \ retval = 0; \ retval;\ }) And the caller: check_reserved_regions_overlap(&bootinfo.reserved_mem, bootinfo.reserved_mem.nr_banks, start, size);
Hi Stefano, On 07/12/2022 01:37, Stefano Stabellini wrote: > On Mon, 5 Dec 2022, 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. Therefore, to unify the checking logic for all of these >> types of static region, this commit firstly introduces a helper >> `check_reserved_regions_overlap()` to check if an input physical >> address range is overlapping with the existing reserved memory regions >> defined in bootinfo. After that, use this helper in >> `device_tree_get_meminfo()` to do the overlap check of (1) and (2) >> and replace the original overlap check of (3) with this new helper. >> >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > I wonder if the check should only be done #ifdef DEBUG. The idea would > be that a given static configuration should be validated and corrected > before going into production. By the time you go in production, it is > too late to do checks anyway. Especially the panic below. > > Julien, Bertrand, what do you think about this? The integrator may be a different person (or even a different company) than the one building Xen. So I think, the new check shoudl not be protected by CONFIG_DEBUG. That said, any output in bootfd will only printed when earlyprintk is enabled. I think we should consider to support dynamic early printk. Anyway, that's something that doesn't need to be handled in this series. > > >> --- >> xen/arch/arm/bootfdt.c | 13 ++++---- >> xen/arch/arm/include/asm/setup.h | 2 ++ >> xen/arch/arm/setup.c | 52 ++++++++++++++++++++++++++++++++ >> 3 files changed, 60 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >> index 6014c0f852..b31379b9ac 100644 >> --- a/xen/arch/arm/bootfdt.c >> +++ b/xen/arch/arm/bootfdt.c >> @@ -91,6 +91,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; >> @@ -485,7 +488,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; >> @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); >> >> +int 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 4395640019..94d232605e 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, >> cb(s, e); >> } >> >> +static int __init overlap_check(void *bootinfo_type, >> + paddr_t region_start, paddr_t region_end) >> +{ >> + unsigned int i, num = 0; >> + paddr_t bank_start = INVALID_PADDR, bank_end = 0; >> + char *type_str = "NONAME"; >> + >> + if ( bootinfo_type == &bootinfo.reserved_mem ) >> + { >> + num = bootinfo.reserved_mem.nr_banks; >> + type_str = "reserved_mem"; >> + } >> + else >> + panic("Invalid bootinfo type passed to overlap check\n"); >> + >> + for ( i = 0; i < num; i++ ) >> + { >> + if ( bootinfo_type == &bootinfo.reserved_mem ) >> + { >> + bank_start = bootinfo.reserved_mem.bank[i].start; >> + bank_end = bank_start + bootinfo.reserved_mem.bank[i].size; >> + } >> + >> + if ( region_end <= bank_start || region_start >= bank_end ) >> + continue; >> + else >> + { >> + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", >> + type_str, region_start, region_end, i, bank_start, bank_end); >> + return -EINVAL; >> + } >> + } >> + >> + return 0; >> +} > > As much as I dislike MACROs in general I think this function should be > written as a MACRO so that we can write it once for all use cases. The > below in not compiled and not tested, just for explanation purposes. > Look how much simpler the code becomes. I agree the duplication is not nice. But it is not clear to me why a static inline function cannot be used. > > #define overlap_check(bootinfo, \ > num, \ > region_start, \ > region_end) \ > ({ \ > unsigned int i, ret; \ > paddr_t bank_start = INVALID_PADDR, bank_end = 0; \ > \ > for ( i = 0; i < num; i++ ) \ > { \ > bank_start = bootinfo->start; \ > bank_end = bank_start + bootinfo->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); \ > ret = -EINVAL; \ > break; \ > } \ > } \ > \ > retval = 0; \ > retval;\ > }) > > > And the caller: > > check_reserved_regions_overlap(&bootinfo.reserved_mem, > bootinfo.reserved_mem.nr_banks, > start, size); Cheers,
On Wed, 7 Dec 2022, Julien Grall wrote: > Hi Stefano, > > On 07/12/2022 01:37, Stefano Stabellini wrote: > > On Mon, 5 Dec 2022, 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. Therefore, to unify the checking logic for all of these > > > types of static region, this commit firstly introduces a helper > > > `check_reserved_regions_overlap()` to check if an input physical > > > address range is overlapping with the existing reserved memory regions > > > defined in bootinfo. After that, use this helper in > > > `device_tree_get_meminfo()` to do the overlap check of (1) and (2) > > > and replace the original overlap check of (3) with this new helper. > > > > > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > > > I wonder if the check should only be done #ifdef DEBUG. The idea would > > be that a given static configuration should be validated and corrected > > before going into production. By the time you go in production, it is > > too late to do checks anyway. Especially the panic below. > > > > Julien, Bertrand, what do you think about this? > > The integrator may be a different person (or even a different company) than > the one building Xen. > > So I think, the new check shoudl not be protected by CONFIG_DEBUG. It is almost like we need something else to say "this is really a production build, disable all checks, I want it to go fast and be as small as possible". Maybe it would be better as a new kconfig option? In any case, this patch is OK as is. > That said, any output in bootfd will only printed when earlyprintk is enabled. > I think we should consider to support dynamic early printk. Anyway, that's > something that doesn't need to be handled in this series. +1 > > > --- > > > xen/arch/arm/bootfdt.c | 13 ++++---- > > > xen/arch/arm/include/asm/setup.h | 2 ++ > > > xen/arch/arm/setup.c | 52 ++++++++++++++++++++++++++++++++ > > > 3 files changed, 60 insertions(+), 7 deletions(-) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index 6014c0f852..b31379b9ac 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -91,6 +91,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; > > > @@ -485,7 +488,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; > > > @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); > > > +int 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 4395640019..94d232605e 100644 > > > --- a/xen/arch/arm/setup.c > > > +++ b/xen/arch/arm/setup.c > > > @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t s, > > > paddr_t e, > > > cb(s, e); > > > } > > > +static int __init overlap_check(void *bootinfo_type, > > > + paddr_t region_start, paddr_t region_end) > > > +{ > > > + unsigned int i, num = 0; > > > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > > > + char *type_str = "NONAME"; > > > + > > > + if ( bootinfo_type == &bootinfo.reserved_mem ) > > > + { > > > + num = bootinfo.reserved_mem.nr_banks; > > > + type_str = "reserved_mem"; > > > + } > > > + else > > > + panic("Invalid bootinfo type passed to overlap check\n"); > > > + > > > + for ( i = 0; i < num; i++ ) > > > + { > > > + if ( bootinfo_type == &bootinfo.reserved_mem ) > > > + { > > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > > + bank_end = bank_start + bootinfo.reserved_mem.bank[i].size; > > > + } > > > + > > > + if ( region_end <= bank_start || region_start >= bank_end ) > > > + continue; > > > + else > > > + { > > > + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" overlapping > > > with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", > > > + type_str, region_start, region_end, i, bank_start, > > > bank_end); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > > As much as I dislike MACROs in general I think this function should be > > written as a MACRO so that we can write it once for all use cases. The > > below in not compiled and not tested, just for explanation purposes. > > Look how much simpler the code becomes. > > I agree the duplication is not nice. But it is not clear to me why a static > inline function cannot be used. You mean a macro generating static inline functions? It cannot be a single static inline function because the bootinfo arguments are of three different types, it just happens that all three have a "start" and "size" struct member so it works great with a macro, but doesn't for a function. > > > > #define overlap_check(bootinfo, \ > > num, \ > > region_start, \ > > region_end) \ > > ({ \ > > unsigned int i, ret; \ > > paddr_t bank_start = INVALID_PADDR, bank_end = 0; \ > > \ > > for ( i = 0; i < num; i++ ) \ > > { \ > > bank_start = bootinfo->start; \ > > bank_end = bank_start + bootinfo->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); \ > > ret = -EINVAL; \ > > break; \ > > } \ > > } \ > > \ > > retval = 0; \ > > retval;\ > > }) > > > > > > And the caller: > > > > check_reserved_regions_overlap(&bootinfo.reserved_mem, > > bootinfo.reserved_mem.nr_banks, > > start, size);
Hi Stefano, On 07/12/2022 22:27, Stefano Stabellini wrote: > On Wed, 7 Dec 2022, Julien Grall wrote: >> On 07/12/2022 01:37, Stefano Stabellini wrote: >>> On Mon, 5 Dec 2022, 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. Therefore, to unify the checking logic for all of these >>>> types of static region, this commit firstly introduces a helper >>>> `check_reserved_regions_overlap()` to check if an input physical >>>> address range is overlapping with the existing reserved memory regions >>>> defined in bootinfo. After that, use this helper in >>>> `device_tree_get_meminfo()` to do the overlap check of (1) and (2) >>>> and replace the original overlap check of (3) with this new helper. >>>> >>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> >>> I wonder if the check should only be done #ifdef DEBUG. The idea would >>> be that a given static configuration should be validated and corrected >>> before going into production. By the time you go in production, it is >>> too late to do checks anyway. Especially the panic below. >>> >>> Julien, Bertrand, what do you think about this? >> >> The integrator may be a different person (or even a different company) than >> the one building Xen. >> >> So I think, the new check shoudl not be protected by CONFIG_DEBUG. > > It is almost like we need something else to say "this is really a > production build, disable all checks, I want it to go fast and be as > small as possible". Maybe it would be better as a new kconfig option? I am not convinced this should be a Kconfig option for the same reason as before: the integrator may be a different entity and you want to be able to check your setup with the final binary. So this most likely want to a be a command line option. > > In any case, this patch is OK as is. > > >> That said, any output in bootfd will only printed when earlyprintk is enabled. >> I think we should consider to support dynamic early printk. Anyway, that's >> something that doesn't need to be handled in this series. > > +1 > > >>>> --- >>>> xen/arch/arm/bootfdt.c | 13 ++++---- >>>> xen/arch/arm/include/asm/setup.h | 2 ++ >>>> xen/arch/arm/setup.c | 52 ++++++++++++++++++++++++++++++++ >>>> 3 files changed, 60 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>>> index 6014c0f852..b31379b9ac 100644 >>>> --- a/xen/arch/arm/bootfdt.c >>>> +++ b/xen/arch/arm/bootfdt.c >>>> @@ -91,6 +91,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; >>>> @@ -485,7 +488,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; >>>> @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); >>>> +int 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 4395640019..94d232605e 100644 >>>> --- a/xen/arch/arm/setup.c >>>> +++ b/xen/arch/arm/setup.c >>>> @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t s, >>>> paddr_t e, >>>> cb(s, e); >>>> } >>>> +static int __init overlap_check(void *bootinfo_type, >>>> + paddr_t region_start, paddr_t region_end) >>>> +{ >>>> + unsigned int i, num = 0; >>>> + paddr_t bank_start = INVALID_PADDR, bank_end = 0; >>>> + char *type_str = "NONAME"; >>>> + >>>> + if ( bootinfo_type == &bootinfo.reserved_mem ) >>>> + { >>>> + num = bootinfo.reserved_mem.nr_banks; >>>> + type_str = "reserved_mem"; >>>> + } >>>> + else >>>> + panic("Invalid bootinfo type passed to overlap check\n"); >>>> + >>>> + for ( i = 0; i < num; i++ ) >>>> + { >>>> + if ( bootinfo_type == &bootinfo.reserved_mem ) >>>> + { >>>> + bank_start = bootinfo.reserved_mem.bank[i].start; >>>> + bank_end = bank_start + bootinfo.reserved_mem.bank[i].size; >>>> + } >>>> + >>>> + if ( region_end <= bank_start || region_start >= bank_end ) >>>> + continue; >>>> + else >>>> + { >>>> + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" overlapping >>>> with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", >>>> + type_str, region_start, region_end, i, bank_start, >>>> bank_end); >>>> + return -EINVAL; >>>> + } >>>> + } >>>> + >>>> + return 0; >>>> +} >>> >>> As much as I dislike MACROs in general I think this function should be >>> written as a MACRO so that we can write it once for all use cases. The >>> below in not compiled and not tested, just for explanation purposes. >>> Look how much simpler the code becomes. >> >> I agree the duplication is not nice. But it is not clear to me why a static >> inline function cannot be used. > > You mean a macro generating static inline functions? > > It cannot be a single static inline function because the bootinfo > arguments are of three different types, it just happens that all three > have a "start" and "size" struct member so it works great with a macro, > but doesn't for a function. It is not clear to me what are the three types you are referring to. Looking at the definition of bootinfo is: struct bootinfo { struct meminfo mem; /* The reserved regions are only used when booting using Device-Tree */ struct meminfo reserved_mem; struct bootmodules modules; struct bootcmdlines cmdlines; #ifdef CONFIG_ACPI struct meminfo acpi; #endif bool static_heap; }; cmdlines is something uninteresting here. So we have two types: - bootmodules for modules - meminfo used by reserved_mem, mem and acpi Looking in details the code, now I understand why you suggested the macro. This is far better than the checking what the array type (not very scalable). Personally, I think trying to share the code between the two types is a bit odd. The logic is the same today, but I envision to merge reserved_mem, mem and acpi in a single array (it would look like the E820) as this would make easier to find the caching attributes per regions when mapping the RAM. So sharing the code would not be possible. That said, if you really want to share the code between the two types. Then I would prefer one of the following option: 1) Provide a callback that is used to fetch the information from the array 2) Provide a common structure that could be used by the function. This would match other generic function like sort & co. Cheers,
On Wed, 7 Dec 2022, Julien Grall wrote: > On 07/12/2022 22:27, Stefano Stabellini wrote: > > On Wed, 7 Dec 2022, Julien Grall wrote: > > > On 07/12/2022 01:37, Stefano Stabellini wrote: > > > > On Mon, 5 Dec 2022, 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. Therefore, to unify the checking logic for all of these > > > > > types of static region, this commit firstly introduces a helper > > > > > `check_reserved_regions_overlap()` to check if an input physical > > > > > address range is overlapping with the existing reserved memory regions > > > > > defined in bootinfo. After that, use this helper in > > > > > `device_tree_get_meminfo()` to do the overlap check of (1) and (2) > > > > > and replace the original overlap check of (3) with this new helper. > > > > > > > > > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > > > > > > > I wonder if the check should only be done #ifdef DEBUG. The idea would > > > > be that a given static configuration should be validated and corrected > > > > before going into production. By the time you go in production, it is > > > > too late to do checks anyway. Especially the panic below. > > > > > > > > Julien, Bertrand, what do you think about this? > > > > > > The integrator may be a different person (or even a different company) > > > than > > > the one building Xen. > > > > > > So I think, the new check shoudl not be protected by CONFIG_DEBUG. > > > > It is almost like we need something else to say "this is really a > > production build, disable all checks, I want it to go fast and be as > > small as possible". Maybe it would be better as a new kconfig option? > > I am not convinced this should be a Kconfig option for the same reason as > before: the integrator may be a different entity and you want to be able to > check your setup with the final binary. > > So this most likely want to a be a command line option. > > > > > In any case, this patch is OK as is. > > > > > > > That said, any output in bootfd will only printed when earlyprintk is > > > enabled. > > > I think we should consider to support dynamic early printk. Anyway, that's > > > something that doesn't need to be handled in this series. > > > > +1 > > > > > > > > > --- > > > > > xen/arch/arm/bootfdt.c | 13 ++++---- > > > > > xen/arch/arm/include/asm/setup.h | 2 ++ > > > > > xen/arch/arm/setup.c | 52 > > > > > ++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 60 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > > > index 6014c0f852..b31379b9ac 100644 > > > > > --- a/xen/arch/arm/bootfdt.c > > > > > +++ b/xen/arch/arm/bootfdt.c > > > > > @@ -91,6 +91,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; > > > > > @@ -485,7 +488,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; > > > > > @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); > > > > > +int 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 4395640019..94d232605e 100644 > > > > > --- a/xen/arch/arm/setup.c > > > > > +++ b/xen/arch/arm/setup.c > > > > > @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t > > > > > s, > > > > > paddr_t e, > > > > > cb(s, e); > > > > > } > > > > > +static int __init overlap_check(void *bootinfo_type, > > > > > + paddr_t region_start, paddr_t > > > > > region_end) > > > > > +{ > > > > > + unsigned int i, num = 0; > > > > > + paddr_t bank_start = INVALID_PADDR, bank_end = 0; > > > > > + char *type_str = "NONAME"; > > > > > + > > > > > + if ( bootinfo_type == &bootinfo.reserved_mem ) > > > > > + { > > > > > + num = bootinfo.reserved_mem.nr_banks; > > > > > + type_str = "reserved_mem"; > > > > > + } > > > > > + else > > > > > + panic("Invalid bootinfo type passed to overlap check\n"); > > > > > + > > > > > + for ( i = 0; i < num; i++ ) > > > > > + { > > > > > + if ( bootinfo_type == &bootinfo.reserved_mem ) > > > > > + { > > > > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > > > > + bank_end = bank_start + > > > > > bootinfo.reserved_mem.bank[i].size; > > > > > + } > > > > > + > > > > > + if ( region_end <= bank_start || region_start >= bank_end ) > > > > > + continue; > > > > > + else > > > > > + { > > > > > + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" > > > > > overlapping > > > > > with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", > > > > > + type_str, region_start, region_end, i, bank_start, > > > > > bank_end); > > > > > + return -EINVAL; > > > > > + } > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > As much as I dislike MACROs in general I think this function should be > > > > written as a MACRO so that we can write it once for all use cases. The > > > > below in not compiled and not tested, just for explanation purposes. > > > > Look how much simpler the code becomes. > > > > > > I agree the duplication is not nice. But it is not clear to me why a > > > static > > > inline function cannot be used. > > > > You mean a macro generating static inline functions? > > > > It cannot be a single static inline function because the bootinfo > > arguments are of three different types, it just happens that all three > > have a "start" and "size" struct member so it works great with a macro, > > but doesn't for a function. > > It is not clear to me what are the three types you are referring to. Looking > at the definition of bootinfo is: > > struct bootinfo { > struct meminfo mem; > /* The reserved regions are only used when booting using Device-Tree */ > struct meminfo reserved_mem; > struct bootmodules modules; > struct bootcmdlines cmdlines; > #ifdef CONFIG_ACPI > struct meminfo acpi; > #endif > bool static_heap; > }; > > cmdlines is something uninteresting here. So we have two types: > - bootmodules for modules > - meminfo used by reserved_mem, mem and acpi > > Looking in details the code, now I understand why you suggested the macro. > This is far better than the checking what the array type (not very scalable). Thank you :-) > Personally, I think trying to share the code between the two types is a bit > odd. The logic is the same today, but I envision to merge reserved_mem, mem > and acpi in a single array (it would look like the E820) as this would make > easier to find the caching attributes per regions when mapping the RAM. So > sharing the code would not be possible. > > That said, if you really want to share the code between the two types. Then I > would prefer one of the following option: > 1) Provide a callback that is used to fetch the information from the array > 2) Provide a common structure that could be used by the function. > > This would match other generic function like sort & co. I think option 2) would be the best but I couldn't think of a simple way to do it (without using a union and I thought a union would not make things nicer in this case). Rather than option 1), I think I would rather have 2 different functions to check struct bootmodules and struct meminfo, or the macro.
Hi both, I was lurking around to see how the discussion would go. Thanks for the discussions/inputs in this thread :) > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Subject: Re: [PATCH 1/3] xen/arm: Add memory overlap check for > bootinfo.reserved_mem > > > It cannot be a single static inline function because the bootinfo > > > arguments are of three different types, it just happens that all three > > > have a "start" and "size" struct member so it works great with a macro, > > > but doesn't for a function. > > > > It is not clear to me what are the three types you are referring to. Looking > > at the definition of bootinfo is: > > > > struct bootinfo { > > struct meminfo mem; > > /* The reserved regions are only used when booting using Device-Tree */ > > struct meminfo reserved_mem; > > struct bootmodules modules; > > struct bootcmdlines cmdlines; > > #ifdef CONFIG_ACPI > > struct meminfo acpi; > > #endif > > bool static_heap; > > }; > > > > cmdlines is something uninteresting here. So we have two types: > > - bootmodules for modules > > - meminfo used by reserved_mem, mem and acpi Exactly, we need to check the given input physical address range is not overlapping with any of the banks in bootmodules and meminfo used by reserved_mem & acpi. > > > > Looking in details the code, now I understand why you suggested the > macro. > > This is far better than the checking what the array type (not very scalable). > > Thank you :-) +1, I also thought this would be quite painful to extend in the future (once we add a new member in bootinfo, for example what Penny did in [1], we need to extend the overlap check as well), but I didn't think of using macro so thank you Stefano :) > > > > Personally, I think trying to share the code between the two types is a bit > > odd. The logic is the same today, but I envision to merge reserved_mem, > mem > > and acpi in a single array (it would look like the E820) as this would make > > easier to find the caching attributes per regions when mapping the RAM. So > > sharing the code would not be possible. > > > > That said, if you really want to share the code between the two types. Then > I > > would prefer one of the following option: > > 1) Provide a callback that is used to fetch the information from the array > > 2) Provide a common structure that could be used by the function. > > > > This would match other generic function like sort & co. > > I think option 2) would be the best but I couldn't think of a simple way > to do it (without using a union and I thought a union would not make > things nicer in this case). > > Rather than option 1), I think I would rather have 2 different functions > to check struct bootmodules and struct meminfo, or the macro. I personally don't have specific taste here. I think the option is good one as long as we can (1) share most part of the code (2) make the code easy to extend in the future. So as long as you two reach a consensus here I will change to the agreed method in v2. [1] https://lore.kernel.org/xen-devel/20221115025235.1378931-2-Penny.Zheng@arm.com/ Kind regards, Henry
On Sat, 10 Dec 2022, Henry Wang wrote: > Hi both, > > I was lurking around to see how the discussion would go. Thanks for the > discussions/inputs in this thread :) > > > -----Original Message----- > > From: Stefano Stabellini <sstabellini@kernel.org> > > Subject: Re: [PATCH 1/3] xen/arm: Add memory overlap check for > > bootinfo.reserved_mem > > > > It cannot be a single static inline function because the bootinfo > > > > arguments are of three different types, it just happens that all three > > > > have a "start" and "size" struct member so it works great with a macro, > > > > but doesn't for a function. > > > > > > It is not clear to me what are the three types you are referring to. Looking > > > at the definition of bootinfo is: > > > > > > struct bootinfo { > > > struct meminfo mem; > > > /* The reserved regions are only used when booting using Device-Tree */ > > > struct meminfo reserved_mem; > > > struct bootmodules modules; > > > struct bootcmdlines cmdlines; > > > #ifdef CONFIG_ACPI > > > struct meminfo acpi; > > > #endif > > > bool static_heap; > > > }; > > > > > > cmdlines is something uninteresting here. So we have two types: > > > - bootmodules for modules > > > - meminfo used by reserved_mem, mem and acpi > > Exactly, we need to check the given input physical address range is > not overlapping with any of the banks in bootmodules and meminfo used by > reserved_mem & acpi. > > > > > > > Looking in details the code, now I understand why you suggested the > > macro. > > > This is far better than the checking what the array type (not very scalable). > > > > Thank you :-) > > +1, I also thought this would be quite painful to extend in the future (once we > add a new member in bootinfo, for example what Penny did in [1], we need to > extend the overlap check as well), but I didn't think of using macro so thank you > Stefano :) > > > > > > > > Personally, I think trying to share the code between the two types is a bit > > > odd. The logic is the same today, but I envision to merge reserved_mem, > > mem > > > and acpi in a single array (it would look like the E820) as this would make > > > easier to find the caching attributes per regions when mapping the RAM. So > > > sharing the code would not be possible. > > > > > > That said, if you really want to share the code between the two types. Then > > I > > > would prefer one of the following option: > > > 1) Provide a callback that is used to fetch the information from the array > > > 2) Provide a common structure that could be used by the function. > > > > > > This would match other generic function like sort & co. > > > > I think option 2) would be the best but I couldn't think of a simple way > > to do it (without using a union and I thought a union would not make > > things nicer in this case). > > > > Rather than option 1), I think I would rather have 2 different functions > > to check struct bootmodules and struct meminfo, or the macro. > > I personally don't have specific taste here. I think the option > is good one as long as we can (1) share most part of the code (2) make the > code easy to extend in the future. So as long as you two reach > a consensus here I will change to the agreed method in v2. I think Julien and I already agree on having 2 separate functions to check for struct bootmodules and struct meminfo. Julien, I take you prefer the two separate functions to a MACRO, right?
Hi Stefano, On 12/12/2022 21:29, Stefano Stabellini wrote: >>>> Personally, I think trying to share the code between the two types is a bit >>>> odd. The logic is the same today, but I envision to merge reserved_mem, >>> mem >>>> and acpi in a single array (it would look like the E820) as this would make >>>> easier to find the caching attributes per regions when mapping the RAM. So >>>> sharing the code would not be possible. >>>> >>>> That said, if you really want to share the code between the two types. Then >>> I >>>> would prefer one of the following option: >>>> 1) Provide a callback that is used to fetch the information from the array >>>> 2) Provide a common structure that could be used by the function. >>>> >>>> This would match other generic function like sort & co. >>> >>> I think option 2) would be the best but I couldn't think of a simple way >>> to do it (without using a union and I thought a union would not make >>> things nicer in this case). >>> >>> Rather than option 1), I think I would rather have 2 different functions >>> to check struct bootmodules and struct meminfo, or the macro. >> >> I personally don't have specific taste here. I think the option >> is good one as long as we can (1) share most part of the code (2) make the >> code easy to extend in the future. So as long as you two reach >> a consensus here I will change to the agreed method in v2. > > I think Julien and I already agree on having 2 separate functions to > check for struct bootmodules and struct meminfo. Julien, I take you > prefer the two separate functions to a MACRO, right? In this case, yes because the macro would IMHO a bit too large. Cheers,
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 6014c0f852..b31379b9ac 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -91,6 +91,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; @@ -485,7 +488,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; @@ -496,12 +501,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 fdbf68aadc..6a9f88ecbb 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); +int 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 4395640019..94d232605e 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -270,6 +270,42 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, cb(s, e); } +static int __init overlap_check(void *bootinfo_type, + paddr_t region_start, paddr_t region_end) +{ + unsigned int i, num = 0; + paddr_t bank_start = INVALID_PADDR, bank_end = 0; + char *type_str = "NONAME"; + + if ( bootinfo_type == &bootinfo.reserved_mem ) + { + num = bootinfo.reserved_mem.nr_banks; + type_str = "reserved_mem"; + } + else + panic("Invalid bootinfo type passed to overlap check\n"); + + for ( i = 0; i < num; i++ ) + { + if ( bootinfo_type == &bootinfo.reserved_mem ) + { + bank_start = bootinfo.reserved_mem.bank[i].start; + bank_end = bank_start + bootinfo.reserved_mem.bank[i].size; + } + + if ( region_end <= bank_start || region_start >= bank_end ) + continue; + else + { + printk("%s: Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n", + type_str, region_start, region_end, i, bank_start, bank_end); + return -EINVAL; + } + } + + return 0; +} + void __init fw_unreserved_regions(paddr_t s, paddr_t e, void (*cb)(paddr_t, paddr_t), unsigned int first) @@ -280,7 +316,23 @@ 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 0 if the input physical address range is not overlapping with any + * existing reserved memory regions, otherwise -EINVAL. + */ +int __init check_reserved_regions_overlap(paddr_t region_start, + paddr_t region_size) +{ + paddr_t region_end = region_start + region_size; + /* Check if input region is overlapping with bootinfo.reserved_mem banks */ + if ( overlap_check(&bootinfo.reserved_mem, region_start, region_end) ) + return -EINVAL; + + return 0; +} 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. Therefore, to unify the checking logic for all of these types of static region, this commit firstly introduces a helper `check_reserved_regions_overlap()` to check if an input physical address range is overlapping with the existing reserved memory regions defined in bootinfo. After that, use this helper in `device_tree_get_meminfo()` to do the overlap check of (1) and (2) and replace the original overlap check of (3) with this new helper. Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- xen/arch/arm/bootfdt.c | 13 ++++---- xen/arch/arm/include/asm/setup.h | 2 ++ xen/arch/arm/setup.c | 52 ++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 7 deletions(-)