Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

>> 
>>>> 
>>>> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int 
>>>> node, uint32_t address_cells,
>>>>    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, 
>>>> &gaddr);
>>>>    size = dt_next_cell(size_cells, &cell);
>>>> 
>>>> +    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
>>>> +    {
>>>> +        printk("fdt: physical address 0x%"PRIpaddr" is not suitably 
>>>> aligned.\n",
>>>> +               paddr);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
>>>> +    {
>>>> +        printk("fdt: guest address 0x%"PRIpaddr" is not suitably 
>>>> aligned.\n",
>>>> +               gaddr);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if ( !IS_ALIGNED(size, PAGE_SIZE) )
>>> What sense does it make to check for size being aligned before checking for 
>>> size being 0? It would pass this check.
>> 
>> Yes, but in the end we are doing that to print a different error message, so 
>> it would pass
>> for 0 and it’s totally fine, but in the end it will fail afterwards. I don’t 
>> see functional disruptions
>> having this one before the other, what is the concern here?
> It does not cause the functional disruption. It is more about code 
> readability and writing cleaner code.
> It makes more sense to first check for size being 0 rather than whether it's 
> page aligned, since the latter can
> pass if former is true and thus not making much sense.

Ok then I will switch them and check it being different from 0 before the 
alignment check.

Cheers,
Luca