Re: [PATCH v5 02/11] xen/arm: avoid repetitive checking in process_shm_node

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 06/12/2023 10:06, Penny Zheng wrote:
> 
> 
> Putting overlap and overflow checking in the loop is causing repetitive
> operation, so this commit extracts both checking outside the loop.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
In general the patch looks good to me:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

That said, there are 2 things I realized during review.

> ---
> v6:
> new commit
> ---
>  xen/arch/arm/static-shmem.c | 39 +++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index cb268cd2ed..1a1a9386e4 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -349,7 +349,7 @@ int __init process_shm_node(const void *fdt, int node, 
> uint32_t address_cells,
>  {
>      const struct fdt_property *prop, *prop_id, *prop_role;
>      const __be32 *cell;
> -    paddr_t paddr, gaddr, size;
> +    paddr_t paddr, gaddr, size, end;
>      struct meminfo *mem = &bootinfo.reserved_mem;
>      unsigned int i;
>      int len;
> @@ -422,6 +422,13 @@ int __init process_shm_node(const void *fdt, int node, 
> uint32_t address_cells,
>          return -EINVAL;
>      }
> 
> +    end = paddr + size;
> +    if ( end <= paddr )
> +    {
> +        printk("fdt: static shared memory region %s overflow\n", shm_id);
> +        return -EINVAL;
> +    }
> +
>      for ( i = 0; i < mem->nr_banks; i++ )
>      {
>          /*
> @@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, 
> uint32_t address_cells,
>                  return -EINVAL;
>              }
>          }
> +        else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
> +            continue;
>          else
>          {
> -            paddr_t end = paddr + size;
> -            paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
> -
> -            if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
You are iterating over reserved memory regions in general, so apart from shmem 
regions there might be truly /reserved ones.
It appears that we don't have overflow check in device_tree_get_meminfo, so 
this second check was the only place to detect that
(protected by a feature, so not very useful) :) This is just an observation and 
I agree to drop it. We should be checking for an
overflow in device_tree_get_meminfo.

The second observation I made is that we don't seem to assign and check the 
return code from device_tree_for_each_node.
This means, that any error while parsing the early fdt (e.g. static shm issues) 
does not stop Xen from booting, which might result in strange behavior later on.
If others agree, I'm ok to send a fix for that.

~Michal