[Qemu-devel] [PATCH target-arm v5 0/5] Reset and Halting modifications + Zynq SMP

[Qemu-devel] [PATCH target-arm v5 0/5] Reset and Halting modifications + Zynq SMP

[Peter Crosthwaite]

Hi All,

The clock controller module in the Zynq platform has the ability to halt
and reset arbitrary devices, including the CPU. We use this feature to implement
SMP Linux - the kernel halts CPU1 then rewrites the vector table to the
secondary entry point and the resets+unhalts.

This series adds SMP support to the Zynq machine, and patches the Zynq SLCR
(the clock controller) to have GPIOs connected to the CPUs. The GPIOs
cause and ARM CPU reset.

Only the reset side is implemented (which is good enough for SMP linux
as it stands). Future work is to implement the halting behaviour as
well.

changed since v4 (PMM review):
Convert to GPIO scheme
Implemented custom secondary cpu reset
OCM Macro cleanup
changed since v3:
Removed halting patches
Reduced to minimal change needed for SMP Zynq


Peter Crosthwaite (5):
  arm: zynq: Macroify OCM Base and Size
  arm: zynq: added SMP support
  zynq_slcr: Implement CPU reset
  arm: Implement reset GPIO.
  arm: zynq: Connect CPU resets to SLCR

 hw/arm/xilinx_zynq.c | 87 ++++++++++++++++++++++++++++++++++++++++------------
 hw/misc/zynq_slcr.c  | 16 ++++++++++
 target-arm/cpu.c     | 23 ++++++++++++++
 target-arm/cpu.h     |  8 +++--
 4 files changed, 112 insertions(+), 22 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH target-arm v5 1/5] arm: zynq: Macroify OCM Base and Size

[Peter Crosthwaite]

Convert magic numbers for OCM to Macros.

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

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

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 98e0958..33a47e0 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -36,7 +36,9 @@
 
 #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
 
-#define MPCORE_PERIPHBASE 0xF8F00000
+#define MPCORE_PERIPHBASE   0xF8F00000
+#define OCM_BASE            0xfffc0000
+#define OCM_SIZE            (256 << 10)
 
 static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
@@ -147,9 +149,9 @@ static void zynq_init(QEMUMachineInitArgs *args)
     memory_region_add_subregion(address_space_mem, 0, ext_ram);
 
     /* 256K of on-chip memory */
-    memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", 256 << 10);
+    memory_region_init_ram(ocm_ram, NULL, "zynq.ocm_ram", OCM_SIZE);
     vmstate_register_ram_global(ocm_ram);
-    memory_region_add_subregion(address_space_mem, 0xFFFC0000, ocm_ram);
+    memory_region_add_subregion(address_space_mem, OCM_BASE, ocm_ram);
 
     DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
 
-- 
1.8.5.3

[Qemu-devel] [PATCH target-arm v5 2/5] arm: zynq: added SMP support

[Peter Crosthwaite]

Added Linux SMP support for the Xilinx Zynq platform (2x CPUs are
supported)

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed from v4:
Removed dummy bootreg addr (PMM review)
Implemented custom secondary cpu reset (PMM review)
Changed from v3:
Author reset
s/zynq_cpus/cpus
simplified custom secondary bootloader
Rebased
Changed from v2:
macro defined the maximum number of CPUS
Changed from v1:
Addressed PMM review
Shorted secondary bootloop using MVN instruction.
Used default reset secondary instead of custom one.
Rebased against QOM cpu developments.
Few whitespace fixes.

 hw/arm/xilinx_zynq.c | 75 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 16 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 33a47e0..63390f7 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -27,6 +27,8 @@
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
 
+#define MAX_CPUS 2
+
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
 #define NUM_QSPI_BUSSES 2
@@ -40,10 +42,43 @@
 #define OCM_BASE            0xfffc0000
 #define OCM_SIZE            (256 << 10)
 
+/* Put SMP bootloader up top of OCM  */
+#define SMP_BOOT_ADDR ((uint64_t)OCM_BASE + OCM_SIZE - sizeof(zynq_smpboot))
+
 static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 
+/* Entry point for secondary CPU. Zynq Linux SMP protocol is to just reset
+ * the secondary to unpen, so any infinite loop will do the trick. Use a WFI
+ * loop as that will cause the emulated CPU to halt (and remove itself from
+ * the work queue pending an interrupt that never comes).
+ */
+static uint32_t zynq_smpboot[] = {
+    0xe320f003, /* wfi */
+    0xeafffffd, /* b       <b wfi> */
+};
+
+static void zynq_write_secondary_boot(ARMCPU *cpu,
+                                      const struct arm_boot_info *info)
+{
+    int n;
+
+    for (n = 0; n < ARRAY_SIZE(zynq_smpboot); n++) {
+        zynq_smpboot[n] = tswap32(zynq_smpboot[n]);
+    }
+    rom_add_blob_fixed("smpboot", zynq_smpboot, sizeof(zynq_smpboot),
+                       SMP_BOOT_ADDR);
+}
+
+static void zynq_reset_secondary(ARMCPU *cpu,
+                                  const struct arm_boot_info *info)
+{
+    CPUARMState *env = &cpu->env;
+
+    env->regs[15] = info->smp_loader_start;
+}
+
 static struct arm_boot_info zynq_binfo = {};
 
 static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
@@ -110,7 +145,7 @@ static void zynq_init(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     ObjectClass *cpu_oc;
-    ARMCPU *cpu;
+    ARMCPU *cpu[MAX_CPUS];
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
@@ -125,17 +160,20 @@ static void zynq_init(QEMUMachineInitArgs *args)
     }
     cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
 
-    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
+    for (n = 0; n < smp_cpus; n++) {
+        cpu[n] = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
 
-    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
+        object_property_set_int(OBJECT(cpu[n]), MPCORE_PERIPHBASE, "reset-cbar",
+                                &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
+        object_property_set_bool(OBJECT(cpu[n]), true, "realized", &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
     }
 
     /* max 2GB ram */
@@ -167,12 +205,14 @@ static void zynq_init(QEMUMachineInitArgs *args)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
 
     dev = qdev_create(NULL, "a9mpcore_priv");
-    qdev_prop_set_uint32(dev, "num-cpu", 1);
+    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
     qdev_init_nofail(dev);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
-    sysbus_connect_irq(busdev, 0,
-                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
+    for (n = 0; n < smp_cpus; n++) {
+        sysbus_connect_irq(busdev, n,
+                           qdev_get_gpio_in(DEVICE(cpu[n]), ARM_CPU_IRQ));
+    }
 
     for (n = 0; n < 64; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
@@ -230,7 +270,10 @@ static void zynq_init(QEMUMachineInitArgs *args)
     zynq_binfo.kernel_filename = kernel_filename;
     zynq_binfo.kernel_cmdline = kernel_cmdline;
     zynq_binfo.initrd_filename = initrd_filename;
-    zynq_binfo.nb_cpus = 1;
+    zynq_binfo.nb_cpus = smp_cpus;
+    zynq_binfo.write_secondary_boot = zynq_write_secondary_boot;
+    zynq_binfo.secondary_cpu_reset_hook = zynq_reset_secondary;
+    zynq_binfo.smp_loader_start = SMP_BOOT_ADDR;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
@@ -241,7 +284,7 @@ static QEMUMachine zynq_machine = {
     .desc = "Xilinx Zynq Platform Baseboard for Cortex-A9",
     .init = zynq_init,
     .block_default_type = IF_SCSI,
-    .max_cpus = 1,
+    .max_cpus = MAX_CPUS,
     .no_sdcard = 1,
 };
 
-- 
1.8.5.3

[Qemu-devel] [PATCH target-arm v5 3/5] zynq_slcr: Implement CPU reset

[Peter Crosthwaite]

Implement the CPU reset IO line of the A9_CPU_RST_CTRL register
(offset 0x244). This is trivial GPIO mapping straight to the register
bits.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v4:
Use GPIOs instead (PMM review)
changed from v3:
Author reset
Use CPU reset rather than device reset
use extract32 rather than << &.
Removed halting functionality
changed from v2:
used device halting API instead of talking to the cpu.

 hw/misc/zynq_slcr.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index e42a5b0..053b4b4 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -114,6 +114,10 @@ typedef enum {
   RESET_MAX
 } ResetValues;
 
+#define ZYNQ_SLCR_NUM_CPUS 2
+
+#define A9_CPU_RST_CTRL_RST_SHIFT 0
+
 #define TYPE_ZYNQ_SLCR "xilinx,zynq_slcr"
 #define ZYNQ_SLCR(obj) OBJECT_CHECK(ZynqSLCRState, (obj), TYPE_ZYNQ_SLCR)
 
@@ -121,6 +125,7 @@ typedef struct ZynqSLCRState {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
+    qemu_irq cpu_resets[ZYNQ_SLCR_NUM_CPUS];
 
     union {
         struct {
@@ -345,6 +350,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
                           uint64_t val, unsigned size)
 {
     ZynqSLCRState *s = (ZynqSLCRState *)opaque;
+    int i;
 
     DB_PRINT("offset: %08x data: %08x\n", (unsigned)offset, (unsigned)val);
 
@@ -399,6 +405,14 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
                 goto bad_reg;
             }
             s->reset[(offset - 0x200) / 4] = val;
+            if (offset - 0x200 == A9_CPU * 4) { /* CPU Reset */
+                for (i = 0; i < ZYNQ_SLCR_NUM_CPUS; ++i) {
+                    bool rst = extract32(val, A9_CPU_RST_CTRL_RST_SHIFT + i, 1);
+
+                    qemu_set_irq(s->cpu_resets[i], rst);
+                    DB_PRINT("%sresetting cpu %d\n", rst ? "un-" : "", i);
+                }
+            }
             break;
         case 0x300:
             s->apu_ctrl = val;
@@ -500,6 +514,8 @@ static int zynq_slcr_init(SysBusDevice *dev)
     memory_region_init_io(&s->iomem, OBJECT(s), &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
 
+    qdev_init_gpio_out(DEVICE(dev), s->cpu_resets, ZYNQ_SLCR_NUM_CPUS);
+
     return 0;
 }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Crosthwaite]

Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
CPUs from device land without the need for the much unwanted reset API
calls.

The CPU is halted as long as the pin is held in reset. Releasing the
reset starts the CPU running again.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Until we have named GPIO support we are stuck with the hardcoded
indexing.

 target-arm/cpu.c | 23 +++++++++++++++++++++++
 target-arm/cpu.h |  8 +++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 52efd5d..83fd56e 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -196,6 +196,27 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0);
 #endif
 }
+
+static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
+{
+    CPUState *cpu = opaque;
+
+    if (level) {
+        cpu_reset(cpu);
+        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
+    } else {
+        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
+        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
+    }
+}
+
+
+static void arm_cpu_kvm_reset_gpio(void *opaque, int irq, int level)
+{
+#ifdef CONFIG_KVM
+    qemu_log_mask(LOG_UNIMP, "ARM Reset GPIO not implemented for KVM\n");
+#endif
+}
 #endif
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -218,8 +239,10 @@ static void arm_cpu_initfn(Object *obj)
     /* Our inbound IRQ and FIQ lines */
     if (kvm_enabled()) {
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 2);
+        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_reset_gpio, 1);
     } else {
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 2);
+        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_reset_gpio, 1);
     }
 
     cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 198b6b8..7612d9f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -78,9 +78,11 @@
 #define offsetoflow32(S, M) offsetof(S, M)
 #endif
 
-/* Meanings of the ARMCPU object's two inbound GPIO lines */
-#define ARM_CPU_IRQ 0
-#define ARM_CPU_FIQ 1
+/* Meanings of the ARMCPU object's inbound GPIO lines.  */
+#define ARM_CPU_IRQ         0
+#define ARM_CPU_FIQ         1
+/* reset GPIO is inited after irqs, so its index is one past FIQ */
+#define ARM_CPU_RESET       (ARM_CPU_FIQ + 1)
 
 typedef void ARMWriteCPFunc(void *opaque, int cp_info,
                             int srcreg, int operand, uint32_t value);
-- 
1.8.5.3

[Qemu-devel] [PATCH target-arm v5 5/5] arm: zynq: Connect CPU resets to SLCR

[Peter Crosthwaite]

Wire up the reset line from the SLCR to the CPUs.

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

 hw/arm/xilinx_zynq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 63390f7..1898d5a 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -203,6 +203,10 @@ static void zynq_init(QEMUMachineInitArgs *args)
     dev = qdev_create(NULL, "xilinx,zynq_slcr");
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
+    for (n = 0; n < smp_cpus; n++) {
+        qdev_connect_gpio_out(dev, n,
+                              qdev_get_gpio_in(DEVICE(cpu[n]), ARM_CPU_RESET));
+    }
 
     dev = qdev_create(NULL, "a9mpcore_priv");
     qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH target-arm v5 0/5] Reset and Halting modifications + Zynq SMP

[Peter Crosthwaite]

ping!

On Wed, Jan 15, 2014 at 7:12 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi All,
>
> The clock controller module in the Zynq platform has the ability to halt
> and reset arbitrary devices, including the CPU. We use this feature to implement
> SMP Linux - the kernel halts CPU1 then rewrites the vector table to the
> secondary entry point and the resets+unhalts.
>
> This series adds SMP support to the Zynq machine, and patches the Zynq SLCR
> (the clock controller) to have GPIOs connected to the CPUs. The GPIOs
> cause and ARM CPU reset.
>
> Only the reset side is implemented (which is good enough for SMP linux
> as it stands). Future work is to implement the halting behaviour as
> well.
>
> changed since v4 (PMM review):
> Convert to GPIO scheme
> Implemented custom secondary cpu reset
> OCM Macro cleanup
> changed since v3:
> Removed halting patches
> Reduced to minimal change needed for SMP Zynq
>
>
> Peter Crosthwaite (5):
>   arm: zynq: Macroify OCM Base and Size
>   arm: zynq: added SMP support
>   zynq_slcr: Implement CPU reset
>   arm: Implement reset GPIO.
>   arm: zynq: Connect CPU resets to SLCR
>
>  hw/arm/xilinx_zynq.c | 87 ++++++++++++++++++++++++++++++++++++++++------------
>  hw/misc/zynq_slcr.c  | 16 ++++++++++
>  target-arm/cpu.c     | 23 ++++++++++++++
>  target-arm/cpu.h     |  8 +++--
>  4 files changed, 112 insertions(+), 22 deletions(-)
>
> --
> 1.8.5.3
>

Re: [Qemu-devel] [PATCH target-arm v5 1/5] arm: zynq: Macroify OCM Base and Size

[Peter Maydell]

On 15 January 2014 09:13, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Convert magic numbers for OCM to Macros.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/arm/xilinx_zynq.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 98e0958..33a47e0 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -36,7 +36,9 @@
>
>  #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
>
> -#define MPCORE_PERIPHBASE 0xF8F00000
> +#define MPCORE_PERIPHBASE   0xF8F00000
> +#define OCM_BASE            0xfffc0000
> +#define OCM_SIZE            (256 << 10)

Consistency in whether we capitalize the hex constants
or not would be nice :-)

Also, I think your patch subject prefixes are a bit nonstandard.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v5 2/5] arm: zynq: added SMP support

[Peter Maydell]

On 15 January 2014 09:13, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Added Linux SMP support for the Xilinx Zynq platform (2x CPUs are
> supported)
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v5 3/5] zynq_slcr: Implement CPU reset

[Peter Maydell]

On 15 January 2014 09:14, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Implement the CPU reset IO line of the A9_CPU_RST_CTRL register
> (offset 0x244). This is trivial GPIO mapping straight to the register
> bits.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Maydell]

On 15 January 2014 09:14, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
> CPUs from device land without the need for the much unwanted reset API
> calls.
>
> The CPU is halted as long as the pin is held in reset. Releasing the
> reset starts the CPU running again.

> +static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
> +{
> +    CPUState *cpu = opaque;
> +
> +    if (level) {
> +        cpu_reset(cpu);
> +        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
> +    } else {
> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
> +        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
> +    }
> +}

I don't think this will work properly. For instance,
cpu_exec() will bring the CPU out of halt if an inbound
interrupt arrives, but we should stay in reset until
the reset line is deasserted.

Also ideally speaking we should probably do the reset
actions on the falling edge of reset, not the rising edge.

Does this work properly when we're running under KVM
rather than using the TCG CPU?

Is there anything really ARM-specific in this reset_gpio
function, or could it be implemented at a common level for
all target architectures?

> -/* Meanings of the ARMCPU object's two inbound GPIO lines */
> -#define ARM_CPU_IRQ 0
> -#define ARM_CPU_FIQ 1
> +/* Meanings of the ARMCPU object's inbound GPIO lines.  */
> +#define ARM_CPU_IRQ         0
> +#define ARM_CPU_FIQ         1
> +/* reset GPIO is inited after irqs, so its index is one past FIQ */
> +#define ARM_CPU_RESET       (ARM_CPU_FIQ + 1)

ARMv8 also has an SError which probably ought to follow IRQ/FIQ,
but I think we can safely renumber GPIO lines without breaking
migration.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Crosthwaite]

On Tue, Jan 28, 2014 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 January 2014 09:14, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
>> CPUs from device land without the need for the much unwanted reset API
>> calls.
>>
>> The CPU is halted as long as the pin is held in reset. Releasing the
>> reset starts the CPU running again.
>
>> +static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
>> +{
>> +    CPUState *cpu = opaque;
>> +
>> +    if (level) {
>> +        cpu_reset(cpu);
>> +        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
>> +    } else {
>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
>> +        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
>> +    }
>> +}
>
> I don't think this will work properly. For instance,
> cpu_exec() will bring the CPU out of halt if an inbound
> interrupt arrives, but we should stay in reset until
> the reset line is deasserted.
>

I see. I guess I'm going to have to save the reset pin state as a
boolean in CPU state (and I guess that means is should be migratable).
Then inhibit the true return from cpu_has_work when the pin is
asserted.

> Also ideally speaking we should probably do the reset
> actions on the falling edge of reset, not the rising edge.
>

Any particular reason? I would have thought that any externally
visible state would best be reset ASAP. For level sensitive behavior,
the transitionals should happen going into the active level. Unless
ARM CPU resets are actually falling edge sensitive (in which case the
CPU would continue to run while the reset is held).

> Does this work properly when we're running under KVM
> rather than using the TCG CPU?
>

I must confess no, I explicitly LOG_UNIMP for KVM, as I have no means
to develop or test ARM KVM.

> Is there anything really ARM-specific in this reset_gpio
> function, or could it be implemented at a common level for
> all target architectures?
>

Not yet, but probably will be ARM specific once I add the cpu reset
pin state. Unless Andreas is happy for that pin state and all this
code to go up to the base TYPE_CPU class.

I wonder however, whether different arch will have level/edge/high/low
variances in reset behavior that must be accommodated.

Andreas, you want this in CPU or should we leave it here in ARM land?

Regards,
Peter

>> -/* Meanings of the ARMCPU object's two inbound GPIO lines */
>> -#define ARM_CPU_IRQ 0
>> -#define ARM_CPU_FIQ 1
>> +/* Meanings of the ARMCPU object's inbound GPIO lines.  */
>> +#define ARM_CPU_IRQ         0
>> +#define ARM_CPU_FIQ         1
>> +/* reset GPIO is inited after irqs, so its index is one past FIQ */
>> +#define ARM_CPU_RESET       (ARM_CPU_FIQ + 1)
>
> ARMv8 also has an SError which probably ought to follow IRQ/FIQ,
> but I think we can safely renumber GPIO lines without breaking
> migration.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Maydell]

On 28 January 2014 00:48, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jan 28, 2014 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 15 January 2014 09:14, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
>>> CPUs from device land without the need for the much unwanted reset API
>>> calls.
>>>
>>> The CPU is halted as long as the pin is held in reset. Releasing the
>>> reset starts the CPU running again.
>>
>>> +static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
>>> +{
>>> +    CPUState *cpu = opaque;
>>> +
>>> +    if (level) {
>>> +        cpu_reset(cpu);
>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
>>> +    } else {
>>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
>>> +    }
>>> +}
>>
>> I don't think this will work properly. For instance,
>> cpu_exec() will bring the CPU out of halt if an inbound
>> interrupt arrives, but we should stay in reset until
>> the reset line is deasserted.
>>
>
> I see. I guess I'm going to have to save the reset pin state as a
> boolean in CPU state (and I guess that means is should be migratable).
> Then inhibit the true return from cpu_has_work when the pin is
> asserted.

Maybe we can do it with a CPU_INTERRUPT_RESET ?

>> Also ideally speaking we should probably do the reset
>> actions on the falling edge of reset, not the rising edge.
>>
>
> Any particular reason? I would have thought that any externally
> visible state would best be reset ASAP. For level sensitive behavior,
> the transitionals should happen going into the active level. Unless
> ARM CPU resets are actually falling edge sensitive (in which case the
> CPU would continue to run while the reset is held).

I'm not an expert here, but what I thought happened was:
 * on asserting edge of reset CPU stops running and starts
   resetting its internal state
 * on real hardware you have to hold the line asserted for N clocks
   for this to finish
 * on deasserting edge of reset, CPU samples config lines
   and starts running again

so you should be able to say 'assert reset; change state of
configuration signals; deassert reset', for instance. (On QEMU
that would probably be 'assert reset; change r/w QOM properties;
deassert reset'). Slightly less theoretically, M-class CPUs do
the initial read of the PC from memory in cpu_reset(), so if you
do that on asserting edge then you prevent "hold CPU in reset
and modify the vector table before releasing CPU".

So at least some of what we do in cpu_reset() ought I think
to be done only on deasserting; unless you really want to
split reset into two phases, it seems easiest just to hold the
CPU not-running while reset is asserted and call cpu_reset()
when it is deasserted.

>> Does this work properly when we're running under KVM
>> rather than using the TCG CPU?
>
> I must confess no, I explicitly LOG_UNIMP for KVM, as I have no means
> to develop or test ARM KVM.

I definitely don't want to take this if it doesn't cope with KVM.
I guess I'll have to take a look at how that would work.

>> Is there anything really ARM-specific in this reset_gpio
>> function, or could it be implemented at a common level for
>> all target architectures?
>>
>
> Not yet, but probably will be ARM specific once I add the cpu reset
> pin state. Unless Andreas is happy for that pin state and all this
> code to go up to the base TYPE_CPU class.

It might be possible for most of the mechanics to be shared
(CPU_INTERRUPT_RESET and what setting/clearing it does),
just not the gpio pin.

> I wonder however, whether different arch will have level/edge/high/low
> variances in reset behavior that must be accommodated.

Mmm.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v5 4/5] arm: Implement reset GPIO.

[Andreas Färber]

Hi,

Am 28.01.2014 01:48, schrieb Peter Crosthwaite:
> On Tue, Jan 28, 2014 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Is there anything really ARM-specific in this reset_gpio
>> function, or could it be implemented at a common level for
>> all target architectures?
>>
> 
> Not yet, but probably will be ARM specific once I add the cpu reset
> pin state. Unless Andreas is happy for that pin state and all this
> code to go up to the base TYPE_CPU class.
> 
> I wonder however, whether different arch will have level/edge/high/low
> variances in reset behavior that must be accommodated.
> 
> Andreas, you want this in CPU or should we leave it here in ARM land?

I'm currently swimming in work, so haven't really reviewed this yet...

If you have a good solution that requires additions to common CPU state
then so be it. However, keep in mind that for x86 at least we need to
remain migration-compatible, so state additions would need to go into an
optional VMState subsection for backwards compatibility.

Regards,
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 target-arm v5 4/5] arm: Implement reset GPIO.

[Peter Crosthwaite]

On Tue, Jan 28, 2014 at 7:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 January 2014 00:48, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jan 28, 2014 at 3:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 15 January 2014 09:14, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> Implement a reset GPIO for ARM CPUs. This allows individual reset of ARM
>>>> CPUs from device land without the need for the much unwanted reset API
>>>> calls.
>>>>
>>>> The CPU is halted as long as the pin is held in reset. Releasing the
>>>> reset starts the CPU running again.
>>>
>>>> +static void arm_cpu_reset_gpio(void *opaque, int irq, int level)
>>>> +{
>>>> +    CPUState *cpu = opaque;
>>>> +
>>>> +    if (level) {
>>>> +        cpu_reset(cpu);
>>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
>>>> +    } else {
>>>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
>>>> +        cpu_interrupt(cpu, CPU_INTERRUPT_EXITTB);
>>>> +    }
>>>> +}
>>>
>>> I don't think this will work properly. For instance,
>>> cpu_exec() will bring the CPU out of halt if an inbound
>>> interrupt arrives, but we should stay in reset until
>>> the reset line is deasserted.
>>>
>>
>> I see. I guess I'm going to have to save the reset pin state as a
>> boolean in CPU state (and I guess that means is should be migratable).
>> Then inhibit the true return from cpu_has_work when the pin is
>> asserted.
>
> Maybe we can do it with a CPU_INTERRUPT_RESET ?
>
>>> Also ideally speaking we should probably do the reset
>>> actions on the falling edge of reset, not the rising edge.
>>>
>>
>> Any particular reason? I would have thought that any externally
>> visible state would best be reset ASAP. For level sensitive behavior,
>> the transitionals should happen going into the active level. Unless
>> ARM CPU resets are actually falling edge sensitive (in which case the
>> CPU would continue to run while the reset is held).
>
> I'm not an expert here, but what I thought happened was:
>  * on asserting edge of reset CPU stops running and starts
>    resetting its internal state
>  * on real hardware you have to hold the line asserted for N clocks
>    for this to finish
>  * on deasserting edge of reset, CPU samples config lines
>    and starts running again
>
> so you should be able to say 'assert reset; change state of
> configuration signals; deassert reset', for instance. (On QEMU
> that would probably be 'assert reset; change r/w QOM properties;
> deassert reset'). Slightly less theoretically, M-class CPUs do
> the initial read of the PC from memory in cpu_reset(), so if you
> do that on asserting edge then you prevent "hold CPU in reset
> and modify the vector table before releasing CPU".
>
> So at least some of what we do in cpu_reset() ought I think
> to be done only on deasserting; unless you really want to
> split reset into two phases, it seems easiest just to hold the
> CPU not-running while reset is asserted and call cpu_reset()
> when it is deasserted.
>
>>> Does this work properly when we're running under KVM
>>> rather than using the TCG CPU?
>>
>> I must confess no, I explicitly LOG_UNIMP for KVM, as I have no means
>> to develop or test ARM KVM.
>
> I definitely don't want to take this if it doesn't cope with KVM.
> I guess I'll have to take a look at how that would work.
>

Is it as simple as a call to kvm_reset_vcpu?

Regards,
Peter

>>> Is there anything really ARM-specific in this reset_gpio
>>> function, or could it be implemented at a common level for
>>> all target architectures?
>>>
>>
>> Not yet, but probably will be ARM specific once I add the cpu reset
>> pin state. Unless Andreas is happy for that pin state and all this
>> code to go up to the base TYPE_CPU class.
>
> It might be possible for most of the mechanics to be shared
> (CPU_INTERRUPT_RESET and what setting/clearing it does),
> just not the gpio pin.
>
>> I wonder however, whether different arch will have level/edge/high/low
>> variances in reset behavior that must be accommodated.
>
> Mmm.
>
> thanks
> -- PMM
>