| Message ID | 1373977512-28932-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 70976c41c1def9d6e8b664c64cdf83b1ea0daa03 |
| Headers | show |
Hi Peter, On Tue, Jul 16, 2013 at 10:25 PM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> > --- > 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? Regards, Peter
On 16 July 2013 15:31, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Hi Peter, > > On Tue, Jul 16, 2013 at 10:25 PM, Peter Maydell > <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> >> --- >> 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. (hwaddr are always 64 bits for everybody now.) -- PMM
Hi Peter, On Wed, Jul 17, 2013 at 12:43 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 July 2013 15:31, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> Hi Peter, >> >> On Tue, Jul 16, 2013 at 10:25 PM, Peter Maydell >> <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> >>> --- >>> 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 <peter.crosthwaite@xilinx.com> Regards, Peter > (hwaddr are always 64 bits for everybody now.) > > -- PMM >
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; - } - mem_reg_property[acells + scells - 1] = cpu_to_be32(binfo->ram_size); - hival = cpu_to_be32(binfo->ram_size >> 32); - if (scells > 1) { - mem_reg_property[acells + scells - 2] = hival; - } else if (hival != 0) { + if (scells < 2 && binfo->ram_size >= (1ULL << 32)) { + /* This is user error so deserves a friendlier error message + * than the failure of setprop_sized_cells would provide + */ fprintf(stderr, "qemu: dtb file not compatible with " "RAM size > 4GB\n"); goto fail; } - rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property, - mem_reg_propsize * sizeof(uint32_t)); + rc = qemu_devtree_setprop_sized_cells(fdt, "/memory", "reg", + acells, binfo->loader_start, + scells, binfo->ram_size); if (rc < 0) { fprintf(stderr, "couldn't set /memory/reg\n"); goto fail;
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 <peter.maydell@linaro.org> --- hw/arm/boot.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)