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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Michal

On 2023/12/6 19:35, Michal Orzel wrote:
> 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>
Thx~

> 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.
True~

> 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
Penny Zheng
Many thanks