[Qemu-devel] [PATCH 0/6] arm_boot/vexpress-a15: Support >4GB of RAM

[Qemu-devel] [PATCH 0/6] arm_boot/vexpress-a15: Support >4GB of RAM

[Peter Maydell]

This patchset adds support for booting with >4GB of RAM on the
Versatile Express Cortex-A15 model. There are some caveats:
 * you need an LPAE A15 kernel
 * you need to be booting with device tree
 * your device tree blob needs to specify #address-cells and
   #size-cells as 2 (so addresses and sizes are 64 bit), which
   means you'll need to tweak the stock kernel dtb
 * you need a minor kernel patch which stops the kernel throwing away
   the high 32 bits of the RAM size: http://patches.linaro.org/9856/
 * you need a 64 bit host, obviously

This patchset sits on top of the LPAE patches and that bugfix
patch for 64 bit cp register writes.

Peter: I've cc'd you as device-tree.[ch] maintainer since I've
added some new helper functions there.

Peter Maydell (6):
  hw/arm_boot.c: Make ram_size a target_phys_addr_t
  hw/arm_boot.c: Consistently use ram_size from arm_boot_info struct
  hw/arm_boot.c: Check for RAM sizes exceeding ATAGS capacity
  device_tree: Add support for reading device tree properties
  hw/arm_boot.c: Support DTBs which use 64 bit addresses
  hw/vexpress.c: Allow >4GB of RAM for Cortex-A15 daughterboard

 device_tree.c |   30 ++++++++++++++++++++++++++++++
 device_tree.h |    4 ++++
 hw/arm-misc.h |    2 +-
 hw/arm_boot.c |   46 +++++++++++++++++++++++++++++++++++++++++-----
 hw/vexpress.c |   13 ++++++++++---
 5 files changed, 86 insertions(+), 9 deletions(-)

[Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Peter Maydell]

Make the RAM size in arm_boot_info a target_phys_addr_t so
it can express RAM sizes up to the limit imposed by the
physical address size.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm-misc.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/arm-misc.h b/hw/arm-misc.h
index 1f96229..c313027 100644
--- a/hw/arm-misc.h
+++ b/hw/arm-misc.h
@@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
 
 /* arm_boot.c */
 struct arm_boot_info {
-    int ram_size;
+    target_phys_addr_t ram_size;
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *initrd_filename;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Make the RAM size in arm_boot_info a target_phys_addr_t so
> it can express RAM sizes up to the limit imposed by the
> physical address size.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  hw/arm-misc.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 1f96229..c313027 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>
>  /* arm_boot.c */
>  struct arm_boot_info {
> -    int ram_size;
> +    target_phys_addr_t ram_size;
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *initrd_filename;
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Andreas Färber]

Am 05.07.2012 19:00, schrieb Peter Maydell:
> Make the RAM size in arm_boot_info a target_phys_addr_t so
> it can express RAM sizes up to the limit imposed by the
> physical address size.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm-misc.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 1f96229..c313027 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>  
>  /* arm_boot.c */
>  struct arm_boot_info {
> -    int ram_size;
> +    target_phys_addr_t ram_size;
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *initrd_filename;

Didn't we conclude in lengthy and emotional discussions that int64_t was
the best compromise to solve the highbank and pseries image loading issues?

What I still dislike about target_phys_addr_t is that "ram_size" is not
an address but a size.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Alexander Graf]

On 06.07.2012, at 15:48, Andreas Färber wrote:

> Am 05.07.2012 19:00, schrieb Peter Maydell:
>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>> it can express RAM sizes up to the limit imposed by the
>> physical address size.
>> 
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> hw/arm-misc.h |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
>> index 1f96229..c313027 100644
>> --- a/hw/arm-misc.h
>> +++ b/hw/arm-misc.h
>> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>> 
>> /* arm_boot.c */
>> struct arm_boot_info {
>> -    int ram_size;
>> +    target_phys_addr_t ram_size;
>>     const char *kernel_filename;
>>     const char *kernel_cmdline;
>>     const char *initrd_filename;
> 
> Didn't we conclude in lengthy and emotional discussions that int64_t was
> the best compromise to solve the highbank and pseries image loading issues?
> 
> What I still dislike about target_phys_addr_t is that "ram_size" is not
> an address but a size.

But isn't MAX(size) always defined to be smaller than or equal to MAX(addr)? So target_phys_addr_t is _always_ a type that is big enough to hold the information.


Alex

Re: [Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Andreas Färber]

Am 06.07.2012 15:54, schrieb Alexander Graf:
> 
> On 06.07.2012, at 15:48, Andreas Färber wrote:
> 
>> Am 05.07.2012 19:00, schrieb Peter Maydell:
>>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>>> it can express RAM sizes up to the limit imposed by the
>>> physical address size.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> hw/arm-misc.h |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
>>> index 1f96229..c313027 100644
>>> --- a/hw/arm-misc.h
>>> +++ b/hw/arm-misc.h
>>> @@ -25,7 +25,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>>>
>>> /* arm_boot.c */
>>> struct arm_boot_info {
>>> -    int ram_size;
>>> +    target_phys_addr_t ram_size;
>>>     const char *kernel_filename;
>>>     const char *kernel_cmdline;
>>>     const char *initrd_filename;
>>
>> Didn't we conclude in lengthy and emotional discussions that int64_t was
>> the best compromise to solve the highbank and pseries image loading issues?
>>
>> What I still dislike about target_phys_addr_t is that "ram_size" is not
>> an address but a size.
> 
> But isn't MAX(size) always defined to be smaller than or equal to MAX(addr)? So target_phys_addr_t is _always_ a type that is big enough to hold the information.

I'm not disputing that. If you do
typedef target_phys_addr_t/* or whatever */ target_phys_size_t;
target_phys_size_t ram_size;
then I'm happy as well, I just dislike writing target_phys_addr_t size.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/6] hw/arm_boot.c: Make ram_size a target_phys_addr_t

[Peter Maydell]

On 6 July 2012 14:48, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.07.2012 19:00, schrieb Peter Maydell:
>> Make the RAM size in arm_boot_info a target_phys_addr_t so
>> it can express RAM sizes up to the limit imposed by the
>> physical address size.

> Didn't we conclude in lengthy and emotional discussions that int64_t was
> the best compromise to solve the highbank and pseries image loading issues?

uint64_t, but yes, you're right. Will change.

-- PMM

[Qemu-devel] [PATCH 2/6] hw/arm_boot.c: Consistently use ram_size from arm_boot_info struct

[Peter Maydell]

Clean up the mix of getting the RAM size from the global ram_size
and from the ram_size field in the arm_boot_info structure, so
that we always use the structure field.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_boot.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index a1e6ddb..29ae324 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -357,7 +357,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     if (kernel_size < 0) {
         entry = info->loader_start + KERNEL_LOAD_ADDR;
         kernel_size = load_image_targphys(info->kernel_filename, entry,
-                                          ram_size - KERNEL_LOAD_ADDR);
+                                          info->ram_size - KERNEL_LOAD_ADDR);
         is_linux = 1;
     }
     if (kernel_size < 0) {
@@ -371,7 +371,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             initrd_size = load_image_targphys(info->initrd_filename,
                                               info->loader_start
                                               + INITRD_LOAD_ADDR,
-                                              ram_size - INITRD_LOAD_ADDR);
+                                              info->ram_size
+                                              - INITRD_LOAD_ADDR);
             if (initrd_size < 0) {
                 fprintf(stderr, "qemu: could not load initrd '%s'\n",
                         info->initrd_filename);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/6] hw/arm_boot.c: Consistently use ram_size from arm_boot_info struct

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Clean up the mix of getting the RAM size from the global ram_size
> and from the ram_size field in the arm_boot_info structure, so
> that we always use the structure field.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  hw/arm_boot.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index a1e6ddb..29ae324 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -357,7 +357,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      if (kernel_size < 0) {
>          entry = info->loader_start + KERNEL_LOAD_ADDR;
>          kernel_size = load_image_targphys(info->kernel_filename, entry,
> -                                          ram_size - KERNEL_LOAD_ADDR);
> +                                          info->ram_size - KERNEL_LOAD_ADDR);
>          is_linux = 1;
>      }
>      if (kernel_size < 0) {
> @@ -371,7 +371,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>              initrd_size = load_image_targphys(info->initrd_filename,
>                                                info->loader_start
>                                                + INITRD_LOAD_ADDR,
> -                                              ram_size - INITRD_LOAD_ADDR);
> +                                              info->ram_size
> +                                              - INITRD_LOAD_ADDR);

No blocker, but can we de-indent one or two tabs, to not have to have
this arg split this across so many lines?

>              if (initrd_size < 0) {
>                  fprintf(stderr, "qemu: could not load initrd '%s'\n",
>                          info->initrd_filename);
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH 2/6] hw/arm_boot.c: Consistently use ram_size from arm_boot_info struct

[Peter Maydell]

On 6 July 2012 03:00, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
> On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> @@ -371,7 +371,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>              initrd_size = load_image_targphys(info->initrd_filename,
>>                                                info->loader_start
>>                                                + INITRD_LOAD_ADDR,
>> -                                              ram_size - INITRD_LOAD_ADDR);
>> +                                              info->ram_size
>> +                                              - INITRD_LOAD_ADDR);
>
> No blocker, but can we de-indent one or two tabs, to not have to have
> this arg split this across so many lines?

The general style at least in ARM related QEMU code is that function
arguments always line up (ie indent to just after the opening paren).
This is what the older code written by Paul does and it's also what
my editor does. I prefor not to break that convention. (I agree this
looks a little ugly, but that's what 80-column restrictions get you.)

-- PMM

[Qemu-devel] [PATCH 3/6] hw/arm_boot.c: Check for RAM sizes exceeding ATAGS capacity

[Peter Maydell]

The legacy ATAGS format for passing information to the kernel only
allows RAM sizes which fit in 32 bits; enforce this restriction
rather than silently doing something weird.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_boot.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 29ae324..7366427 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -399,6 +399,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             bootloader[5] = dtb_start;
         } else {
             bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR;
+            if (info->ram_size >= (1ULL << 32)) {
+                fprintf(stderr, "qemu: RAM size must be less than 4GB to boot"
+                        " Linux kernel without device tree\n");
+                exit(1);
+            }
         }
         bootloader[6] = entry;
         for (n = 0; n < sizeof(bootloader) / 4; n++) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/6] hw/arm_boot.c: Check for RAM sizes exceeding ATAGS capacity

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The legacy ATAGS format for passing information to the kernel only
> allows RAM sizes which fit in 32 bits; enforce this restriction
> rather than silently doing something weird.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm_boot.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 29ae324..7366427 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -399,6 +399,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>              bootloader[5] = dtb_start;
>          } else {
>              bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR;
> +            if (info->ram_size >= (1ULL << 32)) {
> +                fprintf(stderr, "qemu: RAM size must be less than 4GB to boot"
> +                        " Linux kernel without device tree\n");

Error message is a bit weird. Shouldnt it be "RAM size must be less
than 4GB to boot Linux kernel with ATAG command line". DTB shouldnt be
the only way to boot with >4GB.

Regards,
Peter

> +                exit(1);
> +            }
>          }
>          bootloader[6] = entry;
>          for (n = 0; n < sizeof(bootloader) / 4; n++) {
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH 3/6] hw/arm_boot.c: Check for RAM sizes exceeding ATAGS capacity

[Peter Maydell]

On 6 July 2012 03:05, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
> On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +            if (info->ram_size >= (1ULL << 32)) {
>> +                fprintf(stderr, "qemu: RAM size must be less than 4GB to boot"
>> +                        " Linux kernel without device tree\n");
>
> Error message is a bit weird. Shouldnt it be "RAM size must be less
> than 4GB to boot Linux kernel with ATAG command line". DTB shouldnt be
> the only way to boot with >4GB.

I'm trying to steer the user toward the way to solve their problem,
which is to use a device tree. I expect less than 1 QEMU user in 100
has any idea what ATAGS are.

> DTB shouldnt be the only way to boot with >4GB.

I'm not sure what you mean here -- we have two boot methods and
only DTB handles large RAM sizes. Or do you mean machines with eg
flash where you could boot by putting a bootloader in flash?

-- PMM

[Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

Add support for reading device tree properties (both generic
and single-cell ones) to QEMU's convenience wrapper layer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 device_tree.c |   30 ++++++++++++++++++++++++++++++
 device_tree.h |    4 ++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index b366fdd..d7a9b6b 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
     return r;
 }
 
+const void *qemu_devtree_getprop(void *fdt, const char *node_path,
+                                 const char *property, int *lenp)
+{
+    int len;
+    const void *r;
+    if (!lenp) {
+        lenp = &len;
+    }
+    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
+    if (!r) {
+        fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__,
+                node_path, property, fdt_strerror(*lenp));
+        exit(1);
+    }
+    return r;
+}
+
+uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
+                                   const char *property)
+{
+    int len;
+    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
+    if (len != 4) {
+        fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n",
+                __func__, node_path, property);
+        exit(1);
+    }
+    return be32_to_cpu(*p);
+}
+
 uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
 {
     uint32_t r;
diff --git a/device_tree.h b/device_tree.h
index 2244270..f7a3e6c 100644
--- a/device_tree.h
+++ b/device_tree.h
@@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
 int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
                                  const char *property,
                                  const char *target_node_path);
+const void *qemu_devtree_getprop(void *fdt, const char *node_path,
+                                 const char *property, int *lenp);
+uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
+                                   const char *property);
 uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
 uint32_t qemu_devtree_alloc_phandle(void *fdt);
 int qemu_devtree_nop_node(void *fdt, const char *node_path);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add support for reading device tree properties (both generic
> and single-cell ones) to QEMU's convenience wrapper layer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  device_tree.c |   30 ++++++++++++++++++++++++++++++
>  device_tree.h |    4 ++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index b366fdd..d7a9b6b 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>      return r;
>  }
>
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp)
> +{
> +    int len;
> +    const void *r;
> +    if (!lenp) {
> +        lenp = &len;
> +    }
> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +    if (!r) {
> +        fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__,
> +                node_path, property, fdt_strerror(*lenp));
> +        exit(1);
> +    }
> +    return r;
> +}
> +
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property)

Hi Peter,

Can we generalise and get functionality for reading cells with offsets
as well? Your function assumes (and asserts) that the property is a
single cell, but can we add a index parameter for reading a non-0th
property out of a multi-cell prop? Needed for reading things like
ranges, regs and interrupt properties.

Regards,
Peter

> +{
> +    int len;
> +    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
> +    if (len != 4) {
> +        fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n",
> +                __func__, node_path, property);
> +        exit(1);
> +    }
> +    return be32_to_cpu(*p);
> +}
> +
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>  {
>      uint32_t r;
> diff --git a/device_tree.h b/device_tree.h
> index 2244270..f7a3e6c 100644
> --- a/device_tree.h
> +++ b/device_tree.h
> @@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>  int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
>                                   const char *property,
>                                   const char *target_node_path);
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp);
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property);
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_devtree_alloc_phandle(void *fdt);
>  int qemu_devtree_nop_node(void *fdt, const char *node_path);
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
> On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
>> +                                   const char *property)
>
> Hi Peter,
>
> Can we generalise and get functionality for reading cells with offsets
> as well? Your function assumes (and asserts) that the property is a
> single cell, but can we add a index parameter for reading a non-0th
> property out of a multi-cell prop? Needed for reading things like
> ranges, regs and interrupt properties.

We could, but in this instance I was just following the pattern
of the existing qemu_devtree_setprop_cell(), which also works only
on single-celled properties. Maybe 'set one cell in a multicell prop'
should be qemu_devtree_setprop_one_cell() ?

-- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
> Can we generalise and get functionality for reading cells with offsets
> as well? Your function assumes (and asserts) that the property is a
> single cell, but can we add a index parameter for reading a non-0th
> property out of a multi-cell prop? Needed for reading things like
> ranges, regs and interrupt properties.

I was playing about with this and I'm really not sure that we should
be providing a "read a single u32 from a u32 array property" at the
device_tree.c layer. For example, for handling the ranges property
what you really want to do is treat it as a list of tuples (including
doing something sensible if it doesn't have the right length to be
a complete list), so the code that knows the structure of the ranges
property is better off calling qemu_devtree_getprop to get a uint32_t*
for the whole array. Then it has the whole thing as a straightforward
C array which will be much easier and more efficient to handle than
constantly bouncing back into the fdt layer to read each uint32_t.

I've also just realised that I'm assuming that the pointer returned
by fdt_getprop() is naturally aligned for a 32 bit integer if the
property is a 32 bit integer -- is that valid?

-- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 6 July 2012 16:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> I've also just realised that I'm assuming that the pointer returned
> by fdt_getprop() is naturally aligned for a 32 bit integer if the
> property is a 32 bit integer -- is that valid?

To answer my own question here, the dtb format mandates that
property data starts at a 4-aligned address, so casting the
pointer to a uint32_t* is safe.

-- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Crosthwaite]

On Sat, Jul 7, 2012 at 1:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote:
>> Can we generalise and get functionality for reading cells with offsets
>> as well? Your function assumes (and asserts) that the property is a
>> single cell, but can we add a index parameter for reading a non-0th
>> property out of a multi-cell prop? Needed for reading things like
>> ranges, regs and interrupt properties.
>
> I was playing about with this and I'm really not sure that we should
> be providing a "read a single u32 from a u32 array property" at the
> device_tree.c layer. For example, for handling the ranges property
> what you really want to do is treat it as a list of tuples

The tuples concept layers on top of the cells concept. The meaning of
cells will differ from property to property but cells are cells. The
setter API manages cells but not tuples, so the same should be true of
the getter API. So if you are dealing with tuples, the device tree
layer should provide access to the cells (which is essentially reading
u32 from u32 array) then you can interpret them as tuples if you wish.

I guess what i'm trying to say is that tuples and cells should be
managed quite separately, the former in the client and the latter in
the device tree API.

 (including
> doing something sensible if it doesn't have the right length to be
> a complete list), so the code that knows the structure of the ranges
> property is better off calling qemu_devtree_getprop to get a uint32_t*
> for the whole array.

The logic for the byte reversal should still be in device tree
however. You have to be_to_cpu each read value after reading that
array which is tedious and an easy omission to make. I think if we are
going to wrap for single cell props then it makes sense for multi cell
as well.

Then it has the whole thing as a straightforward
> C array which will be much easier and more efficient to handle than

Efficient yes, but easier no because of the endian issue. For my few
use cases out of tree I only ever get the one property at a time. I
never parse an entire cell array.

> constantly bouncing back into the fdt layer to read each uint32_t.
>

Constantly bouncing back is safer however. If you hang on to an
in-place pointer into the FDT (as returned by get_prop) and someone
comes along and set_props() then your pointer is corrupted. Ive been
snagged before by doing exactly this and eventually came to the
brute-force approach of just requerying the DTB every touch rather
than try to work with pointers to arrays. duping the property could
work, but its a bit of a mess trying to free the returned copies.

(As an aside this is one of the reasons why my chicken-and-egg machine
model uses coroutines instead of threads. I need the get_prop()
followed by its immediate usage to be atomic).

If user care about efficiency over safety, then your get_prop + cast +
endian_swap approach will always be available to them. I just think a
single idx arg at the end creates more options for users. We could
vararg it and if its omitted it just takes the 0th element to keep the
call sites for the 90% case a little more concise.

Regards,
Peter

> I've also just realised that I'm assuming that the pointer returned
> by fdt_getprop() is naturally aligned for a 32 bit integer if the
> property is a 32 bit integer -- is that valid?
>
> -- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 10 July 2012 07:54, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> If user care about efficiency over safety, then your get_prop + cast +
> endian_swap approach will always be available to them. I just think a
> single idx arg at the end creates more options for users. We could
> vararg it and if its omitted it just takes the 0th element to keep the
> call sites for the 90% case a little more concise.

Hmm, OK, I'll have another go and see how the patch comes out.

-- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 10 July 2012 07:54, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Constantly bouncing back is safer however. If you hang on to an
> in-place pointer into the FDT (as returned by get_prop) and someone
> comes along and set_props() then your pointer is corrupted. Ive been
> snagged before by doing exactly this and eventually came to the
> brute-force approach of just requerying the DTB every touch rather
> than try to work with pointers to arrays. duping the property could
> work, but its a bit of a mess trying to free the returned copies.

Incidentally, if you have two separate bits of code both accessing
the DTB in parallel then this sounds like a really weird corner case
use. I would expect that the standard thing would be "at startup
we read the DTB, modify it slightly and after that ignore it",
all of which should be straightforward single threaded code with
no particular control flow/threading/coroutine issues.

-- PMM

Re: [Qemu-devel] [PATCH 4/6] device_tree: Add support for reading device tree properties

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add support for reading device tree properties (both generic
> and single-cell ones) to QEMU's convenience wrapper layer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  device_tree.c |   30 ++++++++++++++++++++++++++++++
>  device_tree.h |    4 ++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index b366fdd..d7a9b6b 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>      return r;
>  }
>
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp)
> +{
> +    int len;
> +    const void *r;
> +    if (!lenp) {
> +        lenp = &len;
> +    }
> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +    if (!r) {
> +        fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__,
> +                node_path, property, fdt_strerror(*lenp));
> +        exit(1);
> +    }
> +    return r;
> +}
> +
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property)
> +{
> +    int len;
> +    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
> +    if (len != 4) {
> +        fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n",
> +                __func__, node_path, property);
> +        exit(1);
> +    }
> +    return be32_to_cpu(*p);
> +}
> +
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>  {
>      uint32_t r;
> diff --git a/device_tree.h b/device_tree.h
> index 2244270..f7a3e6c 100644
> --- a/device_tree.h
> +++ b/device_tree.h
> @@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>  int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
>                                   const char *property,
>                                   const char *target_node_path);
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp);
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property);
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_devtree_alloc_phandle(void *fdt);
>  int qemu_devtree_nop_node(void *fdt, const char *node_path);
> --
> 1.7.1
>

[Qemu-devel] [PATCH 5/6] hw/arm_boot.c: Support DTBs which use 64 bit addresses

[Peter Maydell]

Support the case where the device tree blob specifies that
#address-cells and #size-cells are greater than 1. (This
is needed for device trees which can handle 64 bit physical
addresses and thus total RAM sizes over 4GB.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_boot.c |   36 +++++++++++++++++++++++++++++++++---
 1 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 7366427..8203422 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -216,11 +216,12 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
 {
 #ifdef CONFIG_FDT
-    uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start),
-                                    cpu_to_be32(binfo->ram_size) };
+    uint32_t *mem_reg_property;
+    uint32_t mem_reg_propsize;
     void *fdt = NULL;
     char *filename;
     int size, rc;
+    uint32_t acells, scells, hival;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
     if (!filename) {
@@ -236,8 +237,37 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
     }
     g_free(filename);
 
+    acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
+    scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
+    printf("fdt: acells %d scells %d\n", acells, scells);
+    if (acells == 0 || scells == 0) {
+        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
+        return -1;
+    }
+
+    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");
+        exit(1);
+    }
+    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) {
+        fprintf(stderr, "qemu: dtb file not compatible with "
+                "RAM size > 4GB\n");
+        exit(1);
+    }
+
     rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
-                               sizeof(mem_reg_property));
+                              mem_reg_propsize * sizeof(uint32_t));
     if (rc < 0) {
         fprintf(stderr, "couldn't set /memory/reg\n");
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/6] hw/arm_boot.c: Support DTBs which use 64 bit addresses

[Blue Swirl]

On Thu, Jul 5, 2012 at 5:00 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Support the case where the device tree blob specifies that
> #address-cells and #size-cells are greater than 1. (This
> is needed for device trees which can handle 64 bit physical
> addresses and thus total RAM sizes over 4GB.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm_boot.c |   36 +++++++++++++++++++++++++++++++++---
>  1 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 7366427..8203422 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -216,11 +216,12 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>  static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>  {
>  #ifdef CONFIG_FDT
> -    uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start),
> -                                    cpu_to_be32(binfo->ram_size) };
> +    uint32_t *mem_reg_property;
> +    uint32_t mem_reg_propsize;
>      void *fdt = NULL;
>      char *filename;
>      int size, rc;
> +    uint32_t acells, scells, hival;
>
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>      if (!filename) {
> @@ -236,8 +237,37 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>      }
>      g_free(filename);
>
> +    acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
> +    scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
> +    printf("fdt: acells %d scells %d\n", acells, scells);

Leftover debugging?

> +    if (acells == 0 || scells == 0) {
> +        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> +        return -1;
> +    }
> +
> +    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");
> +        exit(1);
> +    }
> +    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) {
> +        fprintf(stderr, "qemu: dtb file not compatible with "
> +                "RAM size > 4GB\n");
> +        exit(1);
> +    }
> +
>      rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                               sizeof(mem_reg_property));
> +                              mem_reg_propsize * sizeof(uint32_t));
>      if (rc < 0) {
>          fprintf(stderr, "couldn't set /memory/reg\n");
>      }
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH 5/6] hw/arm_boot.c: Support DTBs which use 64 bit addresses

[Peter Maydell]

On 5 July 2012 19:53, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Thu, Jul 5, 2012 at 5:00 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> @@ -236,8 +237,37 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>>      }
>>      g_free(filename);
>>
>> +    acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
>> +    scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
>> +    printf("fdt: acells %d scells %d\n", acells, scells);
>
> Leftover debugging?

Doh. Yes, will remove printf in v2.

-- PMM

Re: [Qemu-devel] [PATCH 5/6] hw/arm_boot.c: Support DTBs which use 64 bit addresses

[Peter Crosthwaite]

On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Support the case where the device tree blob specifies that
> #address-cells and #size-cells are greater than 1. (This
> is needed for device trees which can handle 64 bit physical
> addresses and thus total RAM sizes over 4GB.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  hw/arm_boot.c |   36 +++++++++++++++++++++++++++++++++---
>  1 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 7366427..8203422 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -216,11 +216,12 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>  static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>  {
>  #ifdef CONFIG_FDT
> -    uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start),
> -                                    cpu_to_be32(binfo->ram_size) };
> +    uint32_t *mem_reg_property;
> +    uint32_t mem_reg_propsize;
>      void *fdt = NULL;
>      char *filename;
>      int size, rc;
> +    uint32_t acells, scells, hival;
>
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>      if (!filename) {
> @@ -236,8 +237,37 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>      }
>      g_free(filename);
>
> +    acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
> +    scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
> +    printf("fdt: acells %d scells %d\n", acells, scells);
> +    if (acells == 0 || scells == 0) {
> +        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> +        return -1;
> +    }
> +
> +    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");
> +        exit(1);
> +    }
> +    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) {
> +        fprintf(stderr, "qemu: dtb file not compatible with "
> +                "RAM size > 4GB\n");
> +        exit(1);
> +    }
> +
>      rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                               sizeof(mem_reg_property));
> +                              mem_reg_propsize * sizeof(uint32_t));
>      if (rc < 0) {
>          fprintf(stderr, "couldn't set /memory/reg\n");
>      }
> --
> 1.7.1
>

[Qemu-devel] [PATCH 6/6] hw/vexpress.c: Allow >4GB of RAM for Cortex-A15 daughterboard

[Peter Maydell]

Now that we have LPAE support and can handle passing 64 bit
RAM sizes to Linux via the device tree, we can lift the
restriction in the Versatile Express A15 daughterboard model
on not having more than 2GB of RAM. Allow up to 30GB, which
is the maximum that can fit in the address map before running
into the (unmodelled) aliases of the first 2GB.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/vexpress.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/vexpress.c b/hw/vexpress.c
index 8072c5a..b2dc8a5 100644
--- a/hw/vexpress.c
+++ b/hw/vexpress.c
@@ -284,9 +284,16 @@ static void a15_daughterboard_init(const VEDBoardInfo *daughterboard,
         cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
     }
 
-    if (ram_size > 0x80000000) {
-        fprintf(stderr, "vexpress-a15: cannot model more than 2GB RAM\n");
-        exit(1);
+    {
+        /* We have to use a separate 64 bit variable here to avoid the gcc
+         * "comparison is always false due to limited range of data type"
+         * warning if we are on a host where ram_addr_t is 32 bits.
+         */
+        uint64_t rsz = ram_size;
+        if (rsz > (30ULL * 1024 * 1024 * 1024)) {
+            fprintf(stderr, "vexpress-a15: cannot model more than 30GB RAM\n");
+            exit(1);
+        }
     }
 
     memory_region_init_ram(ram, "vexpress.highmem", ram_size);
-- 
1.7.1