Re: [Qemu-devel] [PATCH v3 2/8] arm/boot: Use qemu_devtree_setprop_sized_cells()

[Peter Crosthwaite]

Hi Peter,

On Wed, Jul 17, 2013 at 12:43 AM, Peter Maydell
<address@hidden> wrote:
> On 16 July 2013 15:31, Peter Crosthwaite <address@hidden> wrote:
>> Hi Peter,
>>
>> On Tue, Jul 16, 2013 at 10:25 PM, Peter Maydell
>> <address@hidden> wrote:
>>> Replace the opencoded assembly of the reg property array for the
>>> /memory node with a call to qemu_devtree_setprop_sized_cells().
>>>
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> ---
>>>  hw/arm/boot.c |   29 ++++++++---------------------
>>>  1 file changed, 8 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index a2e4032..1780316 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -227,12 +227,10 @@ static void set_kernel_args_old(const struct 
>>> arm_boot_info *info)
>>>
>>>  static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>>>  {
>>> -    uint32_t *mem_reg_property;
>>> -    uint32_t mem_reg_propsize;
>>>      void *fdt = NULL;
>>>      char *filename;
>>>      int size, rc;
>>> -    uint32_t acells, scells, hival;
>>> +    uint32_t acells, scells;
>>>
>>>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>>>      if (!filename) {
>>> @@ -255,29 +253,18 @@ static int load_dtb(hwaddr addr, const struct 
>>> arm_boot_info *binfo)
>>>          goto fail;
>>>      }
>>>
>>> -    mem_reg_propsize = acells + scells;
>>> -    mem_reg_property = g_new0(uint32_t, mem_reg_propsize);
>>> -    mem_reg_property[acells - 1] = cpu_to_be32(binfo->loader_start);
>>> -    hival = cpu_to_be32(binfo->loader_start >> 32);
>>> -    if (acells > 1) {
>>> -        mem_reg_property[acells - 2] = hival;
>>> -    } else if (hival != 0) {
>>> -        fprintf(stderr, "qemu: dtb file not compatible with "
>>> -                "RAM start address > 4GB\n");
>>> -        goto fail;
>>> -    }
>>
>> So it confused me for a while as to why this check is deleted (and not
>> converted), but I'm guessing it is because binfo->loader_start is a
>> hwaddr which is probably 32 bit? Which I guess would cause a check
>> equivalent to the one below to werror. Is it possible in an arm build
>> for hwaddr to be 64 bit and if so should this check be converted?
>
> It's because the "ram start address won't fit" is basically a
> bug in the implementation of a QEMU machine -- it will basically
> only fire for a board which put its RAM all beyond the 4GB
> boundary (vanishingly unlikely as it would be a really stupid
> bit of h/w design) *and* where the user had a DTB with a one-cell
> address size field. So I'm happy to have that fail via the
> call to set_sized_cells() failing, without calling it out as
> a specific error message.
>

Makes sense. If you respin maybe its worth a mention in commit message
as its more that just a conversion of existing behaviour? But

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> (hwaddr are always 64 bits for everybody now.)
>
> -- PMM
>