[Qemu-devel] [PATCH v2 0/2] ARM: Add Versatile Express board model

[Qemu-devel] [PATCH v2 0/2] ARM: Add Versatile Express board model

[Peter Maydell]

This patchset adds support for the ARM Versatile Express board
with Cortex-A9 daughterboard. It's based on some vexpress modelling
work done by Bahadir Balban and Amit Mahajan at B Labs, overhauled
and cleaned up by me (thanks to them for making that work available).

The patchset depends on the MMC cleanup work I posted last week:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg56148.html
as it wants to wire up the MMC status lines.

Changes since v1:
 bump the vmstate version on arm_sysctl, as suggested by Juan Quintela

Peter Maydell (2):
  hw/arm_sysctl.c: Add the Versatile Express system registers
  hw/vexpress.c: Add model of ARM Versatile Express board

 Makefile.target |    1 +
 hw/arm_sysctl.c |   65 +++++++++++++++-
 hw/vexpress.c   |  238 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 302 insertions(+), 2 deletions(-)
 create mode 100644 hw/vexpress.c

[Qemu-devel] [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

Add support for the Versatile Express SYS_CFG registers, which provide
a generic means of reading or writing configuration information from
various parts of the board. We only implement shutdown and reset.

Also make the RESETCTL register RAZ/WI on Versatile Express rather
than reset the board. Other system registers are generally the same
as Versatile and Realview.

This includes a VMState version number bump for arm_sysctl,
since we have new register state to preserve.

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

diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
index 799b007..e4a17f5 100644
--- a/hw/arm_sysctl.c
+++ b/hw/arm_sysctl.c
@@ -27,12 +27,15 @@ typedef struct {
     uint32_t resetlevel;
     uint32_t proc_id;
     uint32_t sys_mci;
+    uint32_t sys_cfgdata;
+    uint32_t sys_cfgctrl;
+    uint32_t sys_cfgstat;
 } arm_sysctl_state;
 
 static const VMStateDescription vmstate_arm_sysctl = {
     .name = "realview_sysctl",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(leds, arm_sysctl_state),
         VMSTATE_UINT16(lockval, arm_sysctl_state),
@@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = {
         VMSTATE_UINT32(flags, arm_sysctl_state),
         VMSTATE_UINT32(nvflags, arm_sysctl_state),
         VMSTATE_UINT32(resetlevel, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -53,6 +59,7 @@ static const VMStateDescription vmstate_arm_sysctl = {
 #define BOARD_ID_EB 0x140
 #define BOARD_ID_PBA8 0x178
 #define BOARD_ID_PBX 0x182
+#define BOARD_ID_VEXPRESS 0x190
 
 static int board_id(arm_sysctl_state *s)
 {
@@ -104,6 +111,10 @@ static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset)
     case 0x38: /* NVFLAGS */
         return s->nvflags;
     case 0x40: /* RESETCTL */
+        if (board_id(s) == BOARD_ID_VEXPRESS) {
+            /* reserved: RAZ/WI */
+            return 0;
+        }
         return s->resetlevel;
     case 0x44: /* PCICTL */
         return 1;
@@ -142,7 +153,23 @@ static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset)
     case 0xcc: /* SYS_TEST_OSC3 */
     case 0xd0: /* SYS_TEST_OSC4 */
         return 0;
+    case 0xa0: /* SYS_CFGDATA */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgdata;
+    case 0xa4: /* SYS_CFGCTRL */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgctrl;
+    case 0xa8: /* SYS_CFGSTAT */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgstat;
     default:
+    bad_reg:
         printf ("arm_sysctl_read: Bad register offset 0x%x\n", (int)offset);
         return 0;
     }
@@ -190,6 +217,10 @@ static void arm_sysctl_write(void *opaque, target_phys_addr_t offset,
         s->nvflags &= ~val;
         break;
     case 0x40: /* RESETCTL */
+        if (board_id(s) == BOARD_ID_VEXPRESS) {
+            /* reserved: RAZ/WI */
+            break;
+        }
         if (s->lockval == LOCK_VALUE) {
             s->resetlevel = val;
             if (val & 0x100)
@@ -216,7 +247,37 @@ static void arm_sysctl_write(void *opaque, target_phys_addr_t offset,
     case 0x98: /* OSCRESET3 */
     case 0x9c: /* OSCRESET4 */
         break;
+    case 0xa0: /* SYS_CFGDATA */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgdata = val;
+        return;
+    case 0xa4: /* SYS_CFGCTRL */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgctrl = val & ~(3 << 18);
+        s->sys_cfgstat = 1;            /* complete */
+        switch (s->sys_cfgctrl) {
+        case 0xc0800000:            /* SYS_CFG_SHUTDOWN to motherboard */
+            qemu_system_shutdown_request();
+            break;
+        case 0xc0900000:            /* SYS_CFG_REBOOT to motherboard */
+            qemu_system_reset_request();
+            break;
+        default:
+            s->sys_cfgstat |= 2;        /* error */
+        }
+        return;
+    case 0xa8: /* SYS_CFGSTAT */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgstat = val & 3;
+        return;
     default:
+    bad_reg:
         printf ("arm_sysctl_write: Bad register offset 0x%x\n", (int)offset);
         return;
     }
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/2] hw/vexpress.c: Add model of ARM Versatile Express board

[Peter Maydell]

Add a model of the ARM Versatile Express board (with A9MPx4
daughterboard).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 Makefile.target |    1 +
 hw/vexpress.c   |  238 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 239 insertions(+), 0 deletions(-)
 create mode 100644 hw/vexpress.c

diff --git a/Makefile.target b/Makefile.target
index 220589e..949bd4e 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -315,6 +315,7 @@ obj-arm-y += framebuffer.o
 obj-arm-y += syborg.o syborg_fb.o syborg_interrupt.o syborg_keyboard.o
 obj-arm-y += syborg_serial.o syborg_timer.o syborg_pointer.o syborg_rtc.o
 obj-arm-y += syborg_virtio.o
+obj-arm-y += vexpress.o
 
 obj-sh4-y = shix.o r2d.o sh7750.o sh7750_regnames.o tc58128.o
 obj-sh4-y += sh_timer.o sh_serial.o sh_intc.o sh_pci.o sm501.o
diff --git a/hw/vexpress.c b/hw/vexpress.c
new file mode 100644
index 0000000..1ed3578
--- /dev/null
+++ b/hw/vexpress.c
@@ -0,0 +1,238 @@
+/*
+ * ARM Versatile Express emulation.
+ *
+ * Copyright (c) 2010 - 2011 B Labs Ltd.
+ * Copyright (c) 2011 Linaro Limited
+ * Written by Bahadir Balban, Amit Mahajan, Peter Maydell
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "sysbus.h"
+#include "arm-misc.h"
+#include "primecell.h"
+#include "devices.h"
+#include "net.h"
+#include "sysemu.h"
+#include "boards.h"
+
+#define SMP_BOOT_ADDR 0xe0000000
+
+#define VEXPRESS_BOARD_ID 0x8e0
+
+static struct arm_boot_info vexpress_binfo = {
+    .smp_loader_start = SMP_BOOT_ADDR,
+};
+
+static void secondary_cpu_reset(void *opaque)
+{
+  CPUState *env = opaque;
+
+  cpu_reset(env);
+  /* Set entry point for secondary CPUs.  This assumes we're using
+     the init code from arm_boot.c.  Real hardware resets all CPUs
+     the same.  */
+  env->regs[15] = SMP_BOOT_ADDR;
+}
+
+static void vexpress_a9_init(ram_addr_t ram_size,
+                     const char *boot_device,
+                     const char *kernel_filename, const char *kernel_cmdline,
+                     const char *initrd_filename, const char *cpu_model)
+{
+    CPUState *env = NULL;
+    ram_addr_t ram_offset, vram_offset, sram_offset;
+    DeviceState *dev, *sysctl;
+    SysBusDevice *busdev;
+    qemu_irq *irqp;
+    qemu_irq pic[64];
+    int n;
+    qemu_irq cpu_irq[4];
+    uint32_t proc_id;
+    uint32_t sys_id;
+    ram_addr_t low_ram_size, vram_size, sram_size;
+
+    if (!cpu_model) {
+        cpu_model = "cortex-a9";
+    }
+
+    for (n = 0; n < smp_cpus; n++) {
+        env = cpu_init(cpu_model);
+        if (!env) {
+            fprintf(stderr, "Unable to find CPU definition\n");
+            exit(1);
+        }
+        irqp = arm_pic_init_cpu(env);
+        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
+        if (n > 0) {
+            qemu_register_reset(secondary_cpu_reset, env);
+        }
+    }
+
+    if (ram_size > 0x40000000) {
+        /* 1GB is the maximum the address space permits */
+        fprintf(stderr, "vexpress: cannot model more than 1GB RAM\n");
+        exit(1);
+    }
+
+    ram_offset = qemu_ram_alloc(NULL, "vexpress.highmem", ram_size);
+    low_ram_size = ram_size;
+    if (low_ram_size > 0x4000000) {
+        low_ram_size = 0x4000000;
+    }
+    /* RAM is from 0x60000000 upwards. The bottom 64MB of the
+     * address space should in theory be remappable to various
+     * things including ROM or RAM; we always map the RAM there.
+     */
+    cpu_register_physical_memory(0x0, low_ram_size, ram_offset | IO_MEM_RAM);
+    cpu_register_physical_memory(0x60000000, ram_size,
+                                 ram_offset | IO_MEM_RAM);
+
+    /* 0x1e000000 A9MPCore (SCU) private memory region */
+    dev = qdev_create(NULL, "a9mpcore_priv");
+    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
+    qdev_init_nofail(dev);
+    busdev = sysbus_from_qdev(dev);
+    vexpress_binfo.smp_priv_base = 0x1e000000;
+    sysbus_mmio_map(busdev, 0, vexpress_binfo.smp_priv_base);
+    for (n = 0; n < smp_cpus; n++) {
+        sysbus_connect_irq(busdev, n, cpu_irq[n]);
+    }
+    /* Interrupts [42:0] are from the motherboard;
+     * [47:43] are reserved; [63:48] are daughterboard
+     * peripherals. Note that some documentation numbers
+     * external interrupts starting from 32 (because the
+     * A9MP has internal interrupts 0..31).
+     */
+    for (n = 0; n < 64; n++) {
+        pic[n] = qdev_get_gpio_in(dev, n);
+    }
+
+    /* Motherboard peripherals CS7 : 0x10000000 .. 0x10020000 */
+    sys_id = 0x1190f500;
+    proc_id = 0x0c000191;
+
+    /* 0x10000000 System registers */
+    sysctl = qdev_create(NULL, "realview_sysctl");
+    qdev_prop_set_uint32(sysctl, "sys_id", sys_id);
+    qdev_init_nofail(sysctl);
+    qdev_prop_set_uint32(sysctl, "proc_id", proc_id);
+    sysbus_mmio_map(sysbus_from_qdev(sysctl), 0, 0x10000000);
+
+    /* 0x10001000 SP810 system control */
+    /* 0x10002000 serial bus PCI */
+    /* 0x10004000 PL041 audio */
+
+    dev = sysbus_create_varargs("pl181", 0x10005000, pic[9], pic[10], NULL);
+    /* Wire up MMC card detect and read-only signals */
+    qdev_connect_gpio_out(dev, 0,
+                          qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
+    qdev_connect_gpio_out(dev, 1,
+                          qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
+
+    sysbus_create_simple("pl050_keyboard", 0x10006000, pic[12]);
+    sysbus_create_simple("pl050_mouse", 0x10007000, pic[13]);
+
+    sysbus_create_simple("pl011", 0x10009000, pic[5]);
+    sysbus_create_simple("pl011", 0x1000a000, pic[6]);
+    sysbus_create_simple("pl011", 0x1000b000, pic[7]);
+    sysbus_create_simple("pl011", 0x1000c000, pic[8]);
+
+    /* 0x1000f000 SP805 WDT */
+
+    sysbus_create_simple("sp804", 0x10011000, pic[2]);
+    sysbus_create_simple("sp804", 0x10012000, pic[3]);
+
+    /* 0x10016000 Serial Bus DVI */
+
+    sysbus_create_simple("pl031", 0x10017000, pic[4]); /* RTC */
+
+    /* 0x1001a000 Compact Flash */
+
+    /* 0x1001f000 PL111 CLCD (motherboard) */
+
+    /* Daughterboard peripherals : 0x10020000 .. 0x20000000 */
+
+    /* 0x10020000 PL111 CLCD (daughterboard) */
+    sysbus_create_simple("pl110", 0x10020000, pic[44]);
+
+    /* 0x10060000 AXI RAM */
+    /* 0x100e0000 PL341 Dynamic Memory Controller */
+    /* 0x100e1000 PL354 Static Memory Controller */
+    /* 0x100e2000 System Configuration Controller */
+
+    sysbus_create_simple("sp804", 0x100e4000, pic[48]);
+    /* 0x100e5000 SP805 Watchdog module */
+    /* 0x100e6000 BP147 TrustZone Protection Controller */
+    /* 0x100e9000 PL301 'Fast' AXI matrix */
+    /* 0x100ea000 PL301 'Slow' AXI matrix */
+    /* 0x100ec000 TrustZone Address Space Controller */
+    /* 0x10200000 CoreSight debug APB */
+    /* 0x1e00a000 PL310 L2 Cache Controller */
+
+    /* CS0: NOR0 flash          : 0x40000000 .. 0x44000000 */
+    /* CS4: NOR1 flash          : 0x44000000 .. 0x48000000 */
+    /* CS2: SRAM                : 0x48000000 .. 0x4a000000 */
+    sram_size = 0x2000000;
+    sram_offset = qemu_ram_alloc(NULL, "vexpress.sram", sram_size);
+    cpu_register_physical_memory(0x48000000, sram_size,
+                                 sram_offset | IO_MEM_RAM);
+
+    /* CS3: USB, ethernet, VRAM : 0x4c000000 .. 0x50000000 */
+
+    /* 0x4c000000 Video RAM */
+    vram_size = 0x800000;
+    vram_offset = qemu_ram_alloc(NULL, "vexpress.vram", vram_size);
+    cpu_register_physical_memory(0x4c000000, vram_size,
+                                 vram_offset | IO_MEM_RAM);
+
+    /* 0x4e000000 LAN9118 Ethernet */
+    if (nd_table[0].vlan) {
+        lan9118_init(&nd_table[0], 0x4e000000, pic[15]);
+    }
+
+    /* 0x4f000000 ISP1761 USB */
+
+    /* ??? Hack to map an additional page of ram for the secondary CPU
+       startup code.  I guess this works on real hardware because the
+       BootROM happens to be in ROM/flash or in memory that isn't clobbered
+       until after Linux boots the secondary CPUs.  */
+    ram_offset = qemu_ram_alloc(NULL, "vexpress.hack", 0x1000);
+    cpu_register_physical_memory(SMP_BOOT_ADDR, 0x1000,
+                                 ram_offset | IO_MEM_RAM);
+
+    vexpress_binfo.ram_size = ram_size;
+    vexpress_binfo.kernel_filename = kernel_filename;
+    vexpress_binfo.kernel_cmdline = kernel_cmdline;
+    vexpress_binfo.initrd_filename = initrd_filename;
+    vexpress_binfo.nb_cpus = smp_cpus;
+    vexpress_binfo.board_id = VEXPRESS_BOARD_ID;
+    vexpress_binfo.loader_start = 0x60000000;
+    arm_load_kernel(first_cpu, &vexpress_binfo);
+}
+
+
+static QEMUMachine vexpress_a9_machine = {
+    .name = "vexpress-a9",
+    .desc = "ARM Versatile Express for Cortex-A9",
+    .init = vexpress_a9_init,
+    .use_scsi = 1,
+    .max_cpus = 4,
+};
+
+static void vexpress_machine_init(void)
+{
+    qemu_register_machine(&vexpress_a9_machine);
+}
+
+machine_init(vexpress_machine_init);
-- 
1.7.1

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Paolo Bonzini]

On 03/04/2011 09:34 PM, Peter Maydell wrote:
>       .name = "realview_sysctl",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT32(leds, arm_sysctl_state),
>           VMSTATE_UINT16(lockval, arm_sysctl_state),
> @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = {
>           VMSTATE_UINT32(flags, arm_sysctl_state),
>           VMSTATE_UINT32(nvflags, arm_sysctl_state),
>           VMSTATE_UINT32(resetlevel, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>           VMSTATE_END_OF_LIST()
>       }

You need to present the fields as version 2-only.

Paolo

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

On 5 March 2011 12:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/04/2011 09:34 PM, Peter Maydell wrote:
>>
>>      .name = "realview_sysctl",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(leds, arm_sysctl_state),
>>          VMSTATE_UINT16(lockval, arm_sysctl_state),
>> @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = {
>>          VMSTATE_UINT32(flags, arm_sysctl_state),
>>          VMSTATE_UINT32(nvflags, arm_sysctl_state),
>>          VMSTATE_UINT32(resetlevel, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>>          VMSTATE_END_OF_LIST()
>>      }
>
> You need to present the fields as version 2-only.

Can you give an example/explanation? docs/migration.txt doesn't
seem to cover this...

thanks
-- PMM

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Paolo Bonzini]

On 03/05/2011 01:34 PM, Peter Maydell wrote:
>>> >>  +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
>>> >>  +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
>>> >>  +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>>> >>            VMSTATE_END_OF_LIST()
>>> >>        }
>> >
>> >  You need to present the fields as version 2-only.
> Can you give an example/explanation? docs/migration.txt doesn't
> seem to cover this...

Sure, sorry for being terse. It simply needs to be:

         VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2),
         VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2),
         VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2),

Also, minimum_version_id needs to remain 1 since you do support loading 
version 1 saved virtual machines.

Paolo

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/05/2011 01:34 PM, Peter Maydell wrote:
>> Can you give an example/explanation? docs/migration.txt doesn't
>> seem to cover this...
>
> Sure, sorry for being terse. It simply needs to be:
>
>        VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2),
>        VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2),
>        VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2),
>
> Also, minimum_version_id needs to remain 1 since you do support loading
> version 1 saved virtual machines.

Ah. I was just following Juan's suggestion:
> - if you don't care about backward compatibility, just add +1 to all the
> version fields and you are done.

but I guess if it's that trivial we might as well support it.
(My guess is that basically nobody is doing any kind of
migration of ARM TCG VMs, so I think it's all a bit moot.)

What do the new fields get set to when you do a load from
a v1 snapshot?

Other random snapshot/vmstate questions while I'm here:

(1) Is there supposed to be any kind of guard on trying to
do a vmsave on a system with devices that don't implement
save/load? IME it just produces a snapshot which doesn't
work when you reload it...
(2) How do you track incompatible changes at the machine
level? For instance, suppose we accidentally forgot to
model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
to the machine init function. None of the devices have
changed, but you can't load the state of an old version
of the machine into a new version, because then the
devices on either end of the inverter would be inconsistent
about the state of the line. But there's no version number
for a machine as far as I can see.


I've appended a draft of a suggested extra section for
docs/migration.txt so you can tell me if I've misunderstood
it all :-)

---begin---
=== Adding state fields to a device ===

If you make a bugfix or enhancement to a device which requires the
addition of extra state, you need to add these new state fields
to the VMStateDescription so that:
 (1) they are saved and loaded correctly
 (2) migration between the new and old versions either works
     or fails cleanly.

If the change is such that migration between versions would not
work anyway, you can just add the new fields using the usual
VMSTATE_* macros, increment the version_id and set the
minimum_version_id to be the same as the version_id.

Migration from the old version to the new version can be supported
if it is OK for the new fields to remain in their default state
[XXX is this right? are they zeroed, or do they get the value
the device's reset function sets them to, or something else?]
when the state of an old-version snapshot is loaded. To implement
this you need to use the VMSTATE_*_V macros which let you specify
the version in which a field was introduced, for instance:

 VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)

for a field introduced in version 2. You should also increment
the version_id, but leave the minimum_version_id unchanged.
Newly added VMSTATE_*_V fields should go at the end of the
VMState description.

---endit---

thanks
-- PMM

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Paolo Bonzini]

On 03/05/2011 05:50 PM, Peter Maydell wrote:
> (1) Is there supposed to be any kind of guard on trying to
> do a vmsave on a system with devices that don't implement
> save/load? IME it just produces a snapshot which doesn't
> work when you reload it...

I think you're right, devices currently have to call 
register_device_unmigratable manually.  I guess you could add support to 
qdev, so that qdev-ified devices could specify a special "forbid 
migration" value for the vmsd field.

Alternatively, you could have NULL mean "forbid migration" and a special 
value for "do not save anything, it will just work".

> (2) How do you track incompatible changes at the machine
> level? For instance, suppose we accidentally forgot to
> model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
> to the machine init function. None of the devices have
> changed, but you can't load the state of an old version
> of the machine into a new version, because then the
> devices on either end of the inverter would be inconsistent
> about the state of the line. But there's no version number
> for a machine as far as I can see.

You can change the machine model and keep the incompatible machine as a 
separate model.  A machine can specify compatibility properties that are 
meant exactly for this kind of bug-compatible behavior.  Reloading the 
VM must be done with the correct -M switch for the old model.

Examples of how to do this are in hw/pc_piix.c (which will provide good 
ideas for further grepping, too :)).

Paolo

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

On 5 March 2011 17:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/05/2011 05:50 PM, Peter Maydell wrote:
>>
>> (1) Is there supposed to be any kind of guard on trying to
>> do a vmsave on a system with devices that don't implement
>> save/load? IME it just produces a snapshot which doesn't
>> work when you reload it...
>
> I think you're right, devices currently have to call
> register_device_unmigratable manually.

That's a shame, since there are still plenty of devices in
the tree which just don't implement save/restore. It would
be nice if trying to vmsave one of those boards produced
an error listing all the devices that would need support
added for it to work.

> I guess you could add support to
> qdev, so that qdev-ified devices could specify a special "forbid migration"
> value for the vmsd field.

> Alternatively, you could have NULL mean "forbid migration" and a special
> value for "do not save anything, it will just work".

You definitely want the default to be "save/load support status
unknown, forbid migration" (whether the device is qdev or not),
and then you can whitelist devices where somebody's actually
checked the code and confirmed that saving nothing is OK.

-- PMM

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Jan Kiszka]

On 2011-03-07 12:45, Peter Maydell wrote:
> On 5 March 2011 17:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 03/05/2011 05:50 PM, Peter Maydell wrote:
>>>
>>> (1) Is there supposed to be any kind of guard on trying to
>>> do a vmsave on a system with devices that don't implement
>>> save/load? IME it just produces a snapshot which doesn't
>>> work when you reload it...
>>
>> I think you're right, devices currently have to call
>> register_device_unmigratable manually.
> 
> That's a shame, since there are still plenty of devices in
> the tree which just don't implement save/restore. It would
> be nice if trying to vmsave one of those boards produced
> an error listing all the devices that would need support
> added for it to work.

register_device_unmigratable is for devices that are conceptually
unmigratable (like assigned physical devices where we can't
extract/restore the state). It's not for devices that simply lack
vmstate support because no one bothered yet. Once we have device state
visualization (it's making progress again), I hope the motivation to
convert the remaining devices will increase further.

But I agree (and pointed this out when register_device_unmigratable was
introduced) that it should become a qdev flag. ivshmem could simple
register two devices, master as migratable, peer as not.

We could provide a void vmsd that qdev devices could use to declare "I'm
migratable", either because they still do legacy vmstate registration
(fortunately, only few are left) or because they have no state. Then the
absence of a vmsd would imply "unmigratable".

> 
>> I guess you could add support to
>> qdev, so that qdev-ified devices could specify a special "forbid migration"
>> value for the vmsd field.
> 
>> Alternatively, you could have NULL mean "forbid migration" and a special
>> value for "do not save anything, it will just work".
> 
> You definitely want the default to be "save/load support status
> unknown, forbid migration" (whether the device is qdev or not),
> and then you can whitelist devices where somebody's actually
> checked the code and confirmed that saving nothing is OK.

Part of the problem is that there are still non-qdev devices that are
basically outside any radar (except the one that scans code...).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

On 5 March 2011 16:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> I've appended a draft of a suggested extra section for
> docs/migration.txt so you can tell me if I've misunderstood
> it all :-)

Ping? In particular if somebody can fill in the [XXX] bit for
me (and correct any other mistakes!) I'll submit this as a proper
patch to the docs.

> ---begin---
> === Adding state fields to a device ===
>
> If you make a bugfix or enhancement to a device which requires the
> addition of extra state, you need to add these new state fields
> to the VMStateDescription so that:
>  (1) they are saved and loaded correctly
>  (2) migration between the new and old versions either works
>     or fails cleanly.
>
> If the change is such that migration between versions would not
> work anyway, you can just add the new fields using the usual
> VMSTATE_* macros, increment the version_id and set the
> minimum_version_id to be the same as the version_id.
>
> Migration from the old version to the new version can be supported
> if it is OK for the new fields to remain in their default state
> [XXX is this right? are they zeroed, or do they get the value
> the device's reset function sets them to, or something else?]
> when the state of an old-version snapshot is loaded. To implement
> this you need to use the VMSTATE_*_V macros which let you specify
> the version in which a field was introduced, for instance:
>
>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>
> for a field introduced in version 2. You should also increment
> the version_id, but leave the minimum_version_id unchanged.
> Newly added VMSTATE_*_V fields should go at the end of the
> VMState description.
>
> ---endit---

thanks
-- PMM

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 03/05/2011 01:34 PM, Peter Maydell wrote:

sorry, I miss this email before.

>
> Ah. I was just following Juan's suggestion:
>> - if you don't care about backward compatibility, just add +1 to all the
>> version fields and you are done.
>
> but I guess if it's that trivial we might as well support it.
> (My guess is that basically nobody is doing any kind of
> migration of ARM TCG VMs, so I think it's all a bit moot.)
>
> What do the new fields get set to when you do a load from
> a v1 snapshot?
>
> Other random snapshot/vmstate questions while I'm here:
>
> (1) Is there supposed to be any kind of guard on trying to
> do a vmsave on a system with devices that don't implement
> save/load? IME it just produces a snapshot which doesn't
> work when you reload it...

only real way to do it is to set a device that is not migratable.  Only
having devices that don't have a savevm section would not work, because
for instance usb devices don't need a savevm section.

> (2) How do you track incompatible changes at the machine
> level? For instance, suppose we accidentally forgot to
> model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
> to the machine init function. None of the devices have
> changed, but you can't load the state of an old version
> of the machine into a new version, because then the
> devices on either end of the inverter would be inconsistent
> about the state of the line. But there's no version number
> for a machine as far as I can see.

Dunno.  I have only worked to x86*, no clue about integrated boards that
have "interesting" conections.  At some point we would have to "define"
the machine board better, just now it is not there.

> I've appended a draft of a suggested extra section for
> docs/migration.txt so you can tell me if I've misunderstood
> it all :-)
>
> ---begin---
> === Adding state fields to a device ===
>
> If you make a bugfix or enhancement to a device which requires the
> addition of extra state, you need to add these new state fields
> to the VMStateDescription so that:
>  (1) they are saved and loaded correctly
>  (2) migration between the new and old versions either works
>      or fails cleanly.
>
> If the change is such that migration between versions would not
> work anyway, you can just add the new fields using the usual
> VMSTATE_* macros, increment the version_id and set the
> minimum_version_id to be the same as the version_id.
>
> Migration from the old version to the new version can be supported
> if it is OK for the new fields to remain in their default state
> [XXX is this right? are they zeroed, or do they get the value
> the device's reset function sets them to, or something else?]

You can initialize in your init function at the value that you want, or
use foo_post_load() function (that receives the version as a parameter)
to assign to any correct values that you need.

> when the state of an old-version snapshot is loaded. To implement
> this you need to use the VMSTATE_*_V macros which let you specify
> the version in which a field was introduced, for instance:
>
>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>
> for a field introduced in version 2. You should also increment
> the version_id, but leave the minimum_version_id unchanged.
> Newly added VMSTATE_*_V fields should go at the end of the
> VMState description.

Just to make things more complicated, this has been "deprecated" O:-)

Ok, I am going to try to explain it myself.

We have one vmstate section.

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 0,
    .minimum_version_id = 0,
    .minimum_version_id_old = 0,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

And we then notice that we need another int (bar2) on the state.
Posibilities:

- We know that old device was wrong, and that there is no way we can
  load (reliabely) from version 0.  Then we just increase the version:

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 1,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32(bar2, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

  And we are done.

- We know that we can load from v1.  But that we want to always sent
  bar2 for migration, then we just increase versions to:


const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 2,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32_V(bar2, FOOState, 1),
        VMSTATE_END_OF_LIST()
    }
};

And we are done.  We are able to receive state 0 and 1, and we would
always sent version 1.

- We know that bar2 is only need sometimes, and we know what sometimes
  mean.  Then we use a new subsection.

/* this function returns true if bar2 is needed */
static bool foo_bar2_needed(void *opaque)
{
    FOOState *s = opaque;

    return ......;
}

const VMStateDescription vmstate_bar2 = {
    .name = "foo/bar2",
    .version_id = 0,
    .minimum_version_id = 0,
    .minimum_version_id_old = 0,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar2, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 2,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32_V(bar2, FOOState, 1),
        VMSTATE_END_OF_LIST()
    }
,
    .subsections = (VMStateSubsection []) {
        {
            .vmsd = &vmstate_bar2,
            .needed = foo_bar2_needed,
        }, {
            /* empty */
        }
    }
};

Why do we want to go to all this trouble? To be able to do a migration
from a new version to one old version.  If bar2 is not needed, we know
that we can _not_ send bar2 subsection. If bar2 is needed, we just sent
it and we fail the migration to the old version.

Notice that this only makes sense if foo_bar2_needed() normally returns
false, if it returns always true, it is just as good to just increase
the version.   An example on when this is useful is with ide.  When we
defined vmstate_ide_drive, we forgot the pio state.  So, it always
worked well execpt if we were in the middle of a pio operation.  PIO
operations are only used (normally) during boot.   Adding a new
subsection means that we normally don't sent the pio state, except if it
is really needed.

Have I manage to explain myself a little bit?

Later, Juan.

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Peter Maydell]

On 22 March 2011 19:53, Juan Quintela <quintela@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> Migration from the old version to the new version can be supported
>> if it is OK for the new fields to remain in their default state
>> [XXX is this right? are they zeroed, or do they get the value
>> the device's reset function sets them to, or something else?]
>
> You can initialize in your init function at the value that you want, or
> use foo_post_load() function (that receives the version as a parameter)
> to assign to any correct values that you need.

To check I understand this, this means an incoming migration is
always done to a fresh, never-been-used-before device that has had
its init called but not its reset?

>> when the state of an old-version snapshot is loaded. To implement
>> this you need to use the VMSTATE_*_V macros which let you specify
>> the version in which a field was introduced, for instance:
>>
>>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>>
>> for a field introduced in version 2. You should also increment
>> the version_id, but leave the minimum_version_id unchanged.
>> Newly added VMSTATE_*_V fields should go at the end of the
>> VMState description.
>
> Just to make things more complicated, this has been "deprecated" O:-)

It has? Your examples below still use it...

> - We know that old device was wrong, and that there is no way we can
>  load (reliabely) from version 0.  Then we just increase the version:

If you're increasing the version can you also clean up by
converting any old VMSTATE_*_V() into plain VMSTATE_*() at this
point, since we can't migrate from those old versions any more?

> - We know that we can load from v1.  But that we want to always sent
>  bar2 for migration, then we just increase versions to:
>
>
> const VMStateDescription vmstate_foo = {
>    .name = "foo",
>    .version_id = 2,
>    .minimum_version_id = 1,
>    .minimum_version_id_old = 1,
>    .fields      = (VMStateField []) {
>        VMSTATE_INT32(bar, FOOState),
>        VMSTATE_INT32_V(bar2, FOOState, 1),
>        VMSTATE_END_OF_LIST()
>    }
> };
>
> And we are done.  We are able to receive state 0 and 1, and we would
> always sent version 1.

Your numbers in the struct and the text don't seem to match?
My guess is you meant to write version_id = 1, minimum_version* = 0 ?

> Have I manage to explain myself a little bit?

Yes, thanks, that's very helpful.

-- PMM

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Paolo Bonzini]

On 03/22/2011 09:32 PM, Peter Maydell wrote:
>> >  Just to make things more complicated, this has been "deprecated"O:-)
>
> It has? Your examples below still use it...

The case in which the "subsection needed" function returns true should 
be rare, so the version number should rarely need to be bumped.  In this 
sense, using _V is discouraged/deprecated.

In fact, some people would prefer the version number not to be bumped 
anymore, and subsections to be always used instead.  So far, every time 
the above argument was brought up in the list, people always found a way 
to define the "subsection needed" function so that it didn't return 
true, and the decision on deprecation of _V was postponed.

Subsections make it easier for downstream versions to backport features 
arbitrarily.  Suppose you release QEMU with a device at version 9.  The 
next version adds feature A as version 10 and feature B as version 11. 
For a downstream vendor, backporting just feature B is difficult because 
they would have three choices:

- the good, but also the hardest: bump to version 11, and save some 
dummy (but valid) value for fields related to feature A.  This 
introduces undesired differences from upstream, and may be difficult.

- the bad: bump to version 10, and have a migration format that is 
incompatible with upstream version 10.

- the ugly: keep version 9, and convert the migration data for feature B 
to a subsection.  This introduces differences from upstream and makes 
the migration format incompatible with upstream version, but avoids that 
the same version number means different things in different distributions.

So, those people say that subsections are a bit more friendly to 
downstream vendors.  So they suggest that upstream should use the third 
option to begin with, and even use subsections even if the "subsection 
needed" function returns true.  This makes the backport easier and more 
straightforward.  The argument is good but, as I said, so far there has 
never been an actual need to apply it.

So, Juan's mail documents what QEMU is doing right now accurately, but 
there isn't 100% agreement that it should be that way in the future. 
Just note that you are encouraged to use subsections (and thus devise a 
way to make the subsection optional) whenever possible and whenever it 
makes sense to help such downstream distributors.

Paolo

[Qemu-devel] Re: [PATCH v2 1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 March 2011 19:53, Juan Quintela <quintela@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Migration from the old version to the new version can be supported
>>> if it is OK for the new fields to remain in their default state
>>> [XXX is this right? are they zeroed, or do they get the value
>>> the device's reset function sets them to, or something else?]
>>
>> You can initialize in your init function at the value that you want, or
>> use foo_post_load() function (that receives the version as a parameter)
>> to assign to any correct values that you need.
>
> To check I understand this, this means an incoming migration is
> always done to a fresh, never-been-used-before device that has had
> its init called but not its reset?
>
>>> when the state of an old-version snapshot is loaded. To implement
>>> this you need to use the VMSTATE_*_V macros which let you specify
>>> the version in which a field was introduced, for instance:
>>>
>>>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>>>
>>> for a field introduced in version 2. You should also increment
>>> the version_id, but leave the minimum_version_id unchanged.
>>> Newly added VMSTATE_*_V fields should go at the end of the
>>> VMState description.
>>
>> Just to make things more complicated, this has been "deprecated" O:-)
>
> It has? Your examples below still use it...

as Paolo says, it should be rare that you need it.


>> - We know that old device was wrong, and that there is no way we can
>>  load (reliabely) from version 0.  Then we just increase the version:
>
> If you're increasing the version can you also clean up by
> converting any old VMSTATE_*_V() into plain VMSTATE_*() at this
> point, since we can't migrate from those old versions any more?

>From vl.c

    qemu_system_reset();
    if (loadvm) {
        if (load_vmstate(loadvm) < 0) {
            autostart = 0;
        }
    }

    if (incoming) {
        int ret = qemu_start_incoming_migration(incoming);
        if (ret < 0) {
            fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
                    incoming, ret);
            exit(ret);
        }
    } else if (autostart) {
        vm_start();
    }


reset is always called after init, before both incoming migration and
normal start.

>> - We know that we can load from v1.  But that we want to always sent
>>  bar2 for migration, then we just increase versions to:
>>
>>
>> const VMStateDescription vmstate_foo = {
>>    .name = "foo",
>>    .version_id = 2,
>>    .minimum_version_id = 1,
>>    .minimum_version_id_old = 1,
>>    .fields      = (VMStateField []) {
>>        VMSTATE_INT32(bar, FOOState),
>>        VMSTATE_INT32_V(bar2, FOOState, 1),
>>        VMSTATE_END_OF_LIST()
>>    }
>> };
>>
>> And we are done.  We are able to receive state 0 and 1, and we would
>> always sent version 1.
>
> Your numbers in the struct and the text don't seem to match?
> My guess is you meant to write version_id = 1, minimum_version* = 0 ?

My fault. copy paste :-(

>> Have I manage to explain myself a little bit?
>
> Yes, thanks, that's very helpful.

You are welcome.

Later, Juan.