[Qemu-devel] [PATCH arm-devs v1 0/6] Fix Support for ARM A9 CBAR

[Qemu-devel] [PATCH arm-devs v1 0/6] Fix Support for ARM A9 CBAR

[Peter Crosthwaite]

Hi Peter,

This patch series fixed the Configuration base address init logic for
ARM CPUs, most notably for A9. Fixes both Zynq and Highbank which both
hade broken CBAR.

Regards,
Peter


Peter Crosthwaite (6):
  target-arm: Define and use ARM_FEATURE_CBAR
  target-arm/cpu: Convert reset CBAR to a property
  arm/highbank: Use object_new() rather than cpu_arm_init()
  arm/highbank: Fix CBAR intialisation
  arm/xilinx_zynq: Use object_new() rather than cpu_arm_init()
  arm/xilinx_zynq: Implement CBAR intialisation

 hw/arm/highbank.c    | 21 +++++++++++++++------
 hw/arm/xilinx_zynq.c | 20 ++++++++++++++++----
 target-arm/cpu.c     | 18 +++++++++---------
 target-arm/cpu.h     |  1 +
 target-arm/helper.c  |  9 +++++++++
 5 files changed, 50 insertions(+), 19 deletions(-)

-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 1/6] target-arm: Define and use ARM_FEATURE_CBAR

[Peter Crosthwaite]

Some processors (notably A9 within Highbank) define and use the
CP15 configuration base address (CBAR). This is vendor specific
so its best implemented as a CPU property (otherwise we would need
vendor specific child classes for every ARM implementation).

This patch prepares support for converting CBAR reset value to
a CPU property by moving the CP registration out of the CPU
init fn, as registration will need to happen at realize time
to pick up any property updates. The easiest way to do this
is vie definition of a new ARM_FEATURE to flag the existence
of the register.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 target-arm/cpu.c    | 11 ++---------
 target-arm/cpu.h    |  1 +
 target-arm/helper.c |  9 +++++++++
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d40f2a7..a82fa61 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -585,6 +585,7 @@ static void cortex_a9_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_VFP_FP16);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
+    set_feature(&cpu->env, ARM_FEATURE_CBAR);
     /* Note that A9 supports the MP extensions even for
      * A9UP and single-core A9MP (which are both different
      * and valid configurations; we don't model A9UP).
@@ -612,15 +613,7 @@ static void cortex_a9_initfn(Object *obj)
     cpu->clidr = (1 << 27) | (1 << 24) | 3;
     cpu->ccsidr[0] = 0xe00fe015; /* 16k L1 dcache. */
     cpu->ccsidr[1] = 0x200fe015; /* 16k L1 icache. */
-    {
-        ARMCPRegInfo cbar = {
-            .name = "CBAR", .cp = 15, .crn = 15,  .crm = 0, .opc1 = 4,
-            .opc2 = 0, .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar,
-            .fieldoffset = offsetof(CPUARMState, cp15.c15_config_base_address)
-        };
-        define_one_arm_cp_reg(cpu, &cbar);
-        define_arm_cp_regs(cpu, cortexa9_cp_reginfo);
-    }
+    define_arm_cp_regs(cpu, cortexa9_cp_reginfo);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 9f110f1..859750a 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -461,6 +461,7 @@ enum arm_features {
     ARM_FEATURE_CACHE_DIRTY_REG, /* 1136/1176 cache dirty status register */
     ARM_FEATURE_CACHE_BLOCK_OPS, /* v6 optional cache block operations */
     ARM_FEATURE_MPIDR, /* has cp15 MPIDR */
+    ARM_FEATURE_CBAR, /* has cp15 CBAR */
     ARM_FEATURE_PXN, /* has Privileged Execute Never bit */
     ARM_FEATURE_LPAE, /* has Large Physical Address Extension */
     ARM_FEATURE_V8,
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3445813..1bf0305 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1744,6 +1744,15 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_one_arm_cp_reg(cpu, &auxcr);
     }
 
+    if (arm_feature(env, ARM_FEATURE_CBAR)) {
+        ARMCPRegInfo cbar = {
+            .name = "CBAR", .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0,
+            .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar,
+            .fieldoffset = offsetof(CPUARMState, cp15.c15_config_base_address)
+        };
+        define_one_arm_cp_reg(cpu, &cbar);
+    }
+
     /* Generic registers whose values depend on the implementation */
     {
         ARMCPRegInfo sctlr = {
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Crosthwaite]

The reset Value of the CP15 CBAR is a vendor (machine) configurable
property. Define arm_cpu_properties and add it.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 target-arm/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index a82fa61..f1c5f6b 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -20,6 +20,7 @@
 
 #include "cpu.h"
 #include "qemu-common.h"
+#include "hw/qdev-properties.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
 #endif
@@ -847,6 +848,11 @@ typedef struct ARMCPUInfo {
     void (*class_init)(ObjectClass *oc, void *data);
 } ARMCPUInfo;
 
+static Property arm_cpu_properties[] = {
+    DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static const ARMCPUInfo arm_cpus[] = {
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
     { .name = "arm926",      .initfn = arm926_initfn },
@@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
 
     acc->parent_realize = dc->realize;
     dc->realize = arm_cpu_realizefn;
+    dc->props = arm_cpu_properties;
 
     acc->parent_reset = cc->reset;
     cc->reset = arm_cpu_reset;
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 3/6] arm/highbank: Use object_new() rather than cpu_arm_init()

[Peter Crosthwaite]

To allow the machine model to set device properties before CPU
realization.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/arm/highbank.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index fe98ef1..70831ba 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -228,11 +228,18 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
         }
     }
 
+    cpu_model = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
+
     for (n = 0; n < smp_cpus; n++) {
         ARMCPU *cpu;
-        cpu = cpu_arm_init(cpu_model);
-        if (cpu == NULL) {
-            fprintf(stderr, "Unable to find CPU definition\n");
+        Error *err = NULL;
+
+        cpu = ARM_CPU(object_new(cpu_model));
+        g_free((void *)cpu_model);
+
+        object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+        if (err) {
+            fprintf(stderr, "%s\n", error_get_pretty(err));
             exit(1);
         }
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 4/6] arm/highbank: Fix CBAR intialisation

[Peter Crosthwaite]

Fix the CBAR initialisation by using the newly defined static property.
CBAR is now set before realization, so the intended value is now
actually used.

So I have kinda tested this. I booted an ARM kernel on Highbank with the
stock Highbank DTB. It doesnt boot (and I will be doing something
wrong), but before this patch I got this:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at /workspaces/pcrost/public/linux2.git/arch/arm/mm/ioremap.c:301 __arm_ioremap_pfn_caller+0x180/0x198()
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W 3.13.0-rc1-next-20131126-dirty #2
[<c0015164>] (unwind_backtrace) from [<c00118c0>] (show_stack+0x10/0x14)
[<c00118c0>] (show_stack) from [<c02bd5fc>] (dump_stack+0x78/0x90)
[<c02bd5fc>] (dump_stack) from [<c001f110>] (warn_slowpath_common+0x68/0x84)
[<c001f110>] (warn_slowpath_common) from [<c001f1f4>] (warn_slowpath_null+0x1c/0x24)
[<c001f1f4>] (warn_slowpath_null) from [<c0017c6c>] (__arm_ioremap_pfn_caller+0x180/0x198)
[<c0017c6c>] (__arm_ioremap_pfn_caller) from [<c0017cd8>] (__arm_ioremap_caller+0x54/0x5c)
[<c0017cd8>] (__arm_ioremap_caller) from [<c0017d10>] (__arm_ioremap+0x18/0x1c)
[<c0017d10>] (__arm_ioremap) from [<c03913c0>] (highbank_init_irq+0x34/0x8c)
[<c03913c0>] (highbank_init_irq) from [<c038c228>] (init_IRQ+0x28/0x2c)
[<c038c228>] (init_IRQ) from [<c03899ec>] (start_kernel+0x234/0x398)
[<c03899ec>] (start_kernel) from [<00008074>] (0x8074)
---[ end trace 3406ff24bd97382f ]---

Which dissappears with this patch.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/arm/highbank.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 70831ba..f0d7949 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -237,14 +237,16 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
         cpu = ARM_CPU(object_new(cpu_model));
         g_free((void *)cpu_model);
 
+        object_property_set_int(OBJECT(cpu), GIC_BASE_ADDR, "cbar", &err);
+        if (err) {
+            fprintf(stderr, "%s\n", error_get_pretty(err));
+            exit(1);
+        }
         object_property_set_bool(OBJECT(cpu), true, "realized", &err);
         if (err) {
             fprintf(stderr, "%s\n", error_get_pretty(err));
             exit(1);
         }
-
-        /* This will become a QOM property eventually */
-        cpu->reset_cbar = GIC_BASE_ADDR;
         cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ);
     }
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 5/6] arm/xilinx_zynq: Use object_new() rather than cpu_arm_init()

[Peter Crosthwaite]

To allow the machine model to set device properties before CPU
realization.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/arm/xilinx_zynq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 46924a0..b4553e8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -110,15 +110,20 @@ static void zynq_init(QEMUMachineInitArgs *args)
     SysBusDevice *busdev;
     qemu_irq pic[64];
     NICInfo *nd;
+    Error *err = NULL;
     int n;
 
     if (!cpu_model) {
         cpu_model = "cortex-a9";
     }
+    cpu_model = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
 
-    cpu = cpu_arm_init(cpu_model);
-    if (!cpu) {
-        fprintf(stderr, "Unable to find CPU definition\n");
+    cpu = ARM_CPU(object_new(cpu_model));
+    g_free((void *)cpu_model);
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err) {
+        fprintf(stderr, "%s\n", error_get_pretty(err));
         exit(1);
     }
 
-- 
1.8.4.4

[Qemu-devel] [PATCH arm-devs v1 6/6] arm/xilinx_zynq: Implement CBAR intialisation

[Peter Crosthwaite]

Fix the CBAR initialisation by using the newly defined static property.
Zynq will now correctly init the CBAR to the SCU base address.

Needed to boot Linux on the xilinx_zynq machine model.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/arm/xilinx_zynq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index b4553e8..e21a0ab 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -35,6 +35,8 @@
 
 #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
 
+#define SCU_BASE_ADDR 0xF8F00000
+
 static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
@@ -121,6 +123,11 @@ static void zynq_init(QEMUMachineInitArgs *args)
     cpu = ARM_CPU(object_new(cpu_model));
     g_free((void *)cpu_model);
 
+    object_property_set_int(OBJECT(cpu), SCU_BASE_ADDR, "cbar", &err);
+    if (err) {
+        fprintf(stderr, "%s\n", error_get_pretty(err));
+        exit(1);
+    }
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
     if (err) {
         fprintf(stderr, "%s\n", error_get_pretty(err));
@@ -159,7 +166,7 @@ static void zynq_init(QEMUMachineInitArgs *args)
     qdev_prop_set_uint32(dev, "num-cpu", 1);
     qdev_init_nofail(dev);
     busdev = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(busdev, 0, 0xF8F00000);
+    sysbus_mmio_map(busdev, 0, SCU_BASE_ADDR);
     sysbus_connect_irq(busdev, 0,
                        qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
 
-- 
1.8.4.4

Re: [Qemu-devel] [PATCH arm-devs v1 3/6] arm/highbank: Use object_new() rather than cpu_arm_init()

[Peter Maydell]

On 27 November 2013 09:02, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> To allow the machine model to set device properties before CPU
> realization.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/arm/highbank.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index fe98ef1..70831ba 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -228,11 +228,18 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
>          }
>      }
>
> +    cpu_model = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> +

Please use cpu_class_by_name() rather than hand-constructing
the classname. (see also the hw/arm/virt.c code I posted the other
day, which needs to set a property for other reasons.)

>      for (n = 0; n < smp_cpus; n++) {
>          ARMCPU *cpu;
> -        cpu = cpu_arm_init(cpu_model);
> -        if (cpu == NULL) {
> -            fprintf(stderr, "Unable to find CPU definition\n");
> +        Error *err = NULL;
> +
> +        cpu = ARM_CPU(object_new(cpu_model));
> +        g_free((void *)cpu_model);
> +
> +        object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> +        if (err) {
> +            fprintf(stderr, "%s\n", error_get_pretty(err));
>              exit(1);
>          }

thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Maydell]

On 27 November 2013 09:01, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The reset Value of the CP15 CBAR is a vendor (machine) configurable
> property. Define arm_cpu_properties and add it.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  target-arm/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index a82fa61..f1c5f6b 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -20,6 +20,7 @@
>
>  #include "cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev-properties.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
>  #endif
> @@ -847,6 +848,11 @@ typedef struct ARMCPUInfo {
>      void (*class_init)(ObjectClass *oc, void *data);
>  } ARMCPUInfo;
>
> +static Property arm_cpu_properties[] = {
> +    DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static const ARMCPUInfo arm_cpus[] = {
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>      { .name = "arm926",      .initfn = arm926_initfn },
> @@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>
>      acc->parent_realize = dc->realize;
>      dc->realize = arm_cpu_realizefn;
> +    dc->props = arm_cpu_properties;
>
>      acc->parent_reset = cc->reset;
>      cc->reset = arm_cpu_reset;

Hmm. This means we end up with a cbar property on every
ARM CPU whether that ARM CPU has a cbar or not...

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Andreas Färber]

Am 27.11.2013 10:14, schrieb Peter Maydell:
> On 27 November 2013 09:01, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The reset Value of the CP15 CBAR is a vendor (machine) configurable
>> property. Define arm_cpu_properties and add it.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  target-arm/cpu.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index a82fa61..f1c5f6b 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -20,6 +20,7 @@
>>
>>  #include "cpu.h"
>>  #include "qemu-common.h"
>> +#include "hw/qdev-properties.h"
>>  #if !defined(CONFIG_USER_ONLY)
>>  #include "hw/loader.h"
>>  #endif
>> @@ -847,6 +848,11 @@ typedef struct ARMCPUInfo {
>>      void (*class_init)(ObjectClass *oc, void *data);
>>  } ARMCPUInfo;
>>
>> +static Property arm_cpu_properties[] = {
>> +    DEFINE_PROP_UINT32("cbar", ARMCPU, reset_cbar, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static const ARMCPUInfo arm_cpus[] = {
>>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>>      { .name = "arm926",      .initfn = arm926_initfn },
>> @@ -895,6 +901,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>>
>>      acc->parent_realize = dc->realize;
>>      dc->realize = arm_cpu_realizefn;
>> +    dc->props = arm_cpu_properties;
>>
>>      acc->parent_reset = cc->reset;
>>      cc->reset = arm_cpu_reset;
> 
> Hmm. This means we end up with a cbar property on every
> ARM CPU whether that ARM CPU has a cbar or not...

If we turn it into a dynamic property, we could register it conditional
to ARM_FEATURE_CBAR.

The overall idea of the series makes sense to me. I would caution to
think about the naming scheme though - we might want to have a "cbar"
property to access the current value whereas we'll likely end up having
several reset values to configure over time.

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 arm-devs v1 3/6] arm/highbank: Use object_new() rather than cpu_arm_init()

[Andreas Färber]

Am 27.11.2013 10:10, schrieb Peter Maydell:
> On 27 November 2013 09:02, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> To allow the machine model to set device properties before CPU
>> realization.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/arm/highbank.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
>> index fe98ef1..70831ba 100644
>> --- a/hw/arm/highbank.c
>> +++ b/hw/arm/highbank.c
>> @@ -228,11 +228,18 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
>>          }
>>      }
>>
>> +    cpu_model = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
>> +
> 
> Please use cpu_class_by_name() rather than hand-constructing
> the classname. (see also the hw/arm/virt.c code I posted the other
> day, which needs to set a property for other reasons.)
> 
>>      for (n = 0; n < smp_cpus; n++) {
>>          ARMCPU *cpu;
>> -        cpu = cpu_arm_init(cpu_model);
>> -        if (cpu == NULL) {
>> -            fprintf(stderr, "Unable to find CPU definition\n");
>> +        Error *err = NULL;
>> +
>> +        cpu = ARM_CPU(object_new(cpu_model));
>> +        g_free((void *)cpu_model);
>> +
>> +        object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>> +        if (err) {
>> +            fprintf(stderr, "%s\n", error_get_pretty(err));

And use of error_report() (without \n) will be appreciated. :)

Cheers,
Andreas

>>              exit(1);
>>          }
> 
> thanks
> -- PMM

-- 
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 arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Maydell]

On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote:
> If we turn it into a dynamic property, we could register it conditional
> to ARM_FEATURE_CBAR.

Unfortunately feature flags only get set at realize (in the
per-cpu init function), so we don't know at the point where
we're registering properties whether to have this one or not.

The other option would be to define an a9 cpu class init fn to
put the property in.

> The overall idea of the series makes sense to me. I would caution to
> think about the naming scheme though - we might want to have a "cbar"
> property to access the current value whereas we'll likely end up having
> several reset values to configure over time.

Mmm, maybe. The approach I think we've taken in some other
places (eg the cache controller) is to have the property names
match the silicon's config or control signal names (which in
this case would be PERIPHBASE), but they are not always
consistent from CPU to CPU, so that might be more confusing
than helpful. I don't think I have a strong opinion here, so I'd
be happy with any vaguely consistent-looking plan.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Andreas Färber]

Am 27.11.2013 11:15, schrieb Peter Maydell:
> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote:
>> If we turn it into a dynamic property, we could register it conditional
>> to ARM_FEATURE_CBAR.
> 
> Unfortunately feature flags only get set at realize (in the
> per-cpu init function), so we don't know at the point where
> we're registering properties whether to have this one or not.

1/6 sets it in instance_init actually. So instance_post_init might do.

> The other option would be to define an a9 cpu class init fn to
> put the property in.

Is it A9-only or would A15, A7, A12, etc. also need it?

>> The overall idea of the series makes sense to me. I would caution to
>> think about the naming scheme though - we might want to have a "cbar"
>> property to access the current value whereas we'll likely end up having
>> several reset values to configure over time.
> 
> Mmm, maybe. The approach I think we've taken in some other
> places (eg the cache controller) is to have the property names
> match the silicon's config or control signal names (which in
> this case would be PERIPHBASE), but they are not always
> consistent from CPU to CPU, so that might be more confusing
> than helpful. I don't think I have a strong opinion here, so I'd
> be happy with any vaguely consistent-looking plan.

Whatever name you choose, I was rather thinking of whether you may want
to call it, e.g., "foo-reset" or "reset-foo" to distinguish from plain
"foo". (Igor's x86 properties series uses "feat-foo" on Anthony's
suggestion to group and parse feature properties.)

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 arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Maydell]

On 27 November 2013 10:27, Andreas Färber <afaerber@suse.de> wrote:
> Am 27.11.2013 11:15, schrieb Peter Maydell:
>> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote:
>>> If we turn it into a dynamic property, we could register it conditional
>>> to ARM_FEATURE_CBAR.
>>
>> Unfortunately feature flags only get set at realize (in the
>> per-cpu init function), so we don't know at the point where
>> we're registering properties whether to have this one or not.
>
> 1/6 sets it in instance_init actually. So instance_post_init might do.

Oh yes, was confusing init and realize.

>> The other option would be to define an a9 cpu class init fn to
>> put the property in.
>
> Is it A9-only or would A15, A7, A12, etc. also need it?

A5, A7, A9, and A15 all have this -- basically all the cores
with memory-mapped peripherals. It is still technically an
IMPDEF register and config signal, though.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Crosthwaite]

Hi Andreas,

On Wed, Nov 27, 2013 at 8:27 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 27.11.2013 11:15, schrieb Peter Maydell:
>> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote:
>>> If we turn it into a dynamic property, we could register it conditional
>>> to ARM_FEATURE_CBAR.
>>
>> Unfortunately feature flags only get set at realize (in the
>> per-cpu init function), so we don't know at the point where
>> we're registering properties whether to have this one or not.
>
> 1/6 sets it in instance_init actually. So instance_post_init might do.
>

Do you have a candidate example of this pattern that I can work off?

>> The other option would be to define an a9 cpu class init fn to
>> put the property in.
>
> Is it A9-only or would A15, A7, A12, etc. also need it?
>
>>> The overall idea of the series makes sense to me. I would caution to
>>> think about the naming scheme though - we might want to have a "cbar"
>>> property to access the current value whereas we'll likely end up having
>>> several reset values to configure over time.
>>
>> Mmm, maybe. The approach I think we've taken in some other
>> places (eg the cache controller) is to have the property names
>> match the silicon's config or control signal names (which in
>> this case would be PERIPHBASE), but they are not always
>> consistent from CPU to CPU, so that might be more confusing
>> than helpful. I don't think I have a strong opinion here, so I'd
>> be happy with any vaguely consistent-looking plan.
>
> Whatever name you choose, I was rather thinking of whether you may want
> to call it, e.g., "foo-reset" or "reset-foo" to distinguish from plain
> "foo". (Igor's x86 properties series uses "feat-foo" on Anthony's
> suggestion to group and parse feature properties.)
>

Is the "periphbase" ever runtime configurable? If not I'm not sure we
need the "reset".

Regards,
Peter

> 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 arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Maydell]

On 27 November 2013 11:39, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Is the "periphbase" ever runtime configurable? If not I'm not sure we
> need the "reset".

You can't runtime configure it (it's a bunch of signals into the
core that determine where the decoder sits the peripherals in
the memory map). However, the CBAR register which on reset
starts out with the value of the base address is a writable
register (writing it won't change where the peripherals live,
it just reads-as-written).

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Andreas Färber]

Hi,

Am 27.11.2013 12:39, schrieb Peter Crosthwaite:
> On Wed, Nov 27, 2013 at 8:27 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 27.11.2013 11:15, schrieb Peter Maydell:
>>> On 27 November 2013 09:42, Andreas Färber <afaerber@suse.de> wrote:
>>>> If we turn it into a dynamic property, we could register it conditional
>>>> to ARM_FEATURE_CBAR.
>>>
>>> Unfortunately feature flags only get set at realize (in the
>>> per-cpu init function), so we don't know at the point where
>>> we're registering properties whether to have this one or not.
>>
>> 1/6 sets it in instance_init actually. So instance_post_init might do.
>>
> 
> Do you have a candidate example of this pattern that I can work off?

For instance_post_init the only user so far will be in hw/core/qdev.c.

target-i386/cpu.c adds a few dynamic properties for the base X86CPU.

Cheers,
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 arm-devs v1 2/6] target-arm/cpu: Convert reset CBAR to a property

[Peter Crosthwaite]

On Wed, Nov 27, 2013 at 9:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 27 November 2013 11:39, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Is the "periphbase" ever runtime configurable? If not I'm not sure we
>> need the "reset".
>
> You can't runtime configure it (it's a bunch of signals into the
> core that determine where the decoder sits the peripherals in
> the memory map).

So that is a top level signal of MPCore rather than A9 CPUs right?
Assuming so, that name "PERIPHBASE" is a ideally property of the the
mpcore container device. And in this ideal world that container
contains the A9 CPUs themselves and propagates "periphbase" through to
the CPU as "cbar" during its own init/realize. Looking at ARM docco,
the definition of CBAR == PERIPHBASE is A9MPCore specific. From ARM
infocentre:

----
Cortex-A9 Technical Reference ManualRevision: r4p1
Home > System Control > Register descriptions > Configuration Base
Address Register

4.3.24. Configuration Base Address Registe

...
Configurations

In Cortex-A9 uniprocessor implementations the base address is set to zero.

In Cortex-A9 MPCore implementations, the base address is reset to
PERIPHBASE[31:13] so that software can determine the location of the
private memory region"
---

So the best name for this register AFAICT is simply CBAR and either
MPCore container or whatever are responsible for setting it to an
appropriate value.

> However, the CBAR register which on reset
> starts out with the value of the base address is a writable
> register (writing it won't change where the peripherals live,
> it just reads-as-written).
>

So with that in mind i think the "reset-" prefix is appropriate.

Regards,
Peter

> -- PMM
>