|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem
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@xxxxxxx>
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);
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |