[Qemu-devel] [PATCH v4 0/2] ARM: add 'virt' platform

[Qemu-devel] [PATCH v4 0/2] ARM: add 'virt' platform

[Peter Maydell]

This patch series adds a 'virt' platform which uses the
kernel's mach-virt (fully device-tree driven) support
to create a simple minimalist platform intended for
use for KVM VM guests. It's based on John Rigby's
patches, but I've overhauled it a lot:

 * renamed user-facing machine to just "virt"
 * removed the A9 support (it can't work since the A9 has no
   generic timers)
 * added virtio-mmio transports instead of random set of 'soc' devices
 * instead of updating io_base as we step through adding devices,
   define a memory map with an array (similar to vexpress)
 * folded in some minor fixes from John's aarch64-support patch
 * rather than explicitly doing endian-swapping on FDT cells,
   use fdt APIs that let us just pass in host-endian values
   and let the fdt layer take care of the swapping
 * miscellaneous minor code cleanups and style fixes

If you want to test this with TCG QEMU you'll also need the
generic-timers implementation patches I posted recently.
A branch with generic-timers plus these patches is here:
https://git.linaro.org/gitweb?p=people/pmaydell/qemu-arm.git;a=shortlog;h=refs/heads/mach-virt

(The kernel in pure mach-virt mode requires generic timers;
it can't deal with getting its clock source from an sp804
timer specified by the device tree. This might be fixed in
a future kernel, but dropping all the soc-device support
from mach-virt makes it simpler anyway.)

An obvious thing this machine does not provide is a serial
port. I would rather just use virtio-console (and we
should implement the 'emergency console/earlyprintk' bit of
the virtio spec).

John Rigby (2):
  ARM: Allow boards to provide an fdt blob
  ARM: Add 'virt' platform

 hw/arm/Makefile.objs |    2 +-
 hw/arm/boot.c        |   32 +++--
 hw/arm/virt.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/arm.h |    7 +
 4 files changed, 391 insertions(+), 13 deletions(-)
 create mode 100644 hw/arm/virt.c

-- 
1.7.9.5

[Qemu-devel] [PATCH v4 1/2] ARM: Allow boards to provide an fdt blob

[Peter Maydell]

From: John Rigby <john.rigby@linaro.org>

If no fdt is provided on command line and the new field
get_dtb in struct arm_boot_info is set then call it to
get a device tree blob.

Signed-off-by: John Rigby <john.rigby@linaro.org>
[PMM: minor tweaks and cleanup]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c        |   32 ++++++++++++++++++++------------
 include/hw/arm/arm.h |    7 +++++++
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 2cbeefd..9d790b7 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -228,23 +228,31 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 {
     void *fdt = NULL;
-    char *filename;
     int size, rc;
     uint32_t acells, scells;
 
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
-    if (!filename) {
-        fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
-        goto fail;
-    }
+    if (binfo->dtb_filename) {
+        char *filename;
+        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
+        if (!filename) {
+            fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
+            goto fail;
+        }
 
-    fdt = load_device_tree(filename, &size);
-    if (!fdt) {
-        fprintf(stderr, "Couldn't open dtb file %s\n", filename);
+        fdt = load_device_tree(filename, &size);
+        if (!fdt) {
+            fprintf(stderr, "Couldn't open dtb file %s\n", filename);
+            g_free(filename);
+            goto fail;
+        }
         g_free(filename);
-        goto fail;
+    } else if (binfo->get_dtb) {
+        fdt = binfo->get_dtb(binfo, &size);
+        if (!fdt) {
+            fprintf(stderr, "Board was unable to create a dtb blob\n");
+            goto fail;
+        }
     }
-    g_free(filename);
 
     acells = qemu_devtree_getprop_cell(fdt, "/", "#address-cells");
     scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
@@ -436,7 +444,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         /* for device tree boot, we pass the DTB directly in r2. Otherwise
          * we point to the kernel args.
          */
-        if (info->dtb_filename) {
+        if (info->dtb_filename || info->get_dtb) {
             /* Place the DTB after the initrd in memory. Note that some
              * kernels will trash anything in the 4K page the initrd
              * ends in, so make sure the DTB isn't caught up in that.
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index bae87c6..4e51a9e 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -55,6 +55,13 @@ struct arm_boot_info {
                                  const struct arm_boot_info *info);
     void (*secondary_cpu_reset_hook)(ARMCPU *cpu,
                                      const struct arm_boot_info *info);
+    /* if a board is able to create a dtb without a dtb file then it
+     * sets get_dtb. This will only be used if no dtb file is provided
+     * by the user. On success, sets *size to the length of the created
+     * dtb, and returns a pointer to it. (The caller must free this memory
+     * with g_free() when it has finished with it.) On failure, returns NULL.
+     */
+    void *(*get_dtb)(const struct arm_boot_info *info, int *size);
     /* if a board needs to be able to modify a device tree provided by
      * the user it should implement this hook.
      */
-- 
1.7.9.5

[Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Peter Maydell]

From: John Rigby <john.rigby@linaro.org>

Add 'virt' platform support corresponding to arch/arm/mach-virt
in the Linux kernel tree. This has no platform-specific code but
can use any device whose kernel driver is is able to work purely
from a device tree node. We use this to instantiate a minimal
set of devices: a GIC and some virtio-mmio transports.

TODO:
 * MAX_MEM for A15 could be higher if we shuffle things about
 * RAM or flash at addr 0??

Signed-off-by: John Rigby <john.rigby@linaro.org>
[PMM:
 Significantly overhauled:
 * renamed user-facing machine to just "virt"
 * removed the A9 support (it can't work since the A9 has no
   generic timers)
 * added virtio-mmio transports instead of random set of 'soc' devices
 * instead of updating io_base as we step through adding devices,
   define a memory map with an array (similar to vexpress)
 * folded in some minor fixes from John's aarch64-support patch
 * rather than explicitly doing endian-swapping on FDT cells,
   use fdt APIs that let us just pass in host-endian values
   and let the fdt layer take care of the swapping
 * miscellaneous minor code cleanups and style fixes
]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/Makefile.objs |    2 +-
 hw/arm/virt.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 364 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/virt.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 9e3a06f..744484f 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,7 +1,7 @@
 obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
 obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
 obj-y += omap_sx1.o palm.o pic_cpu.o realview.o spitz.o stellaris.o
-obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
+obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
 
 obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
 obj-y += omap1.o omap2.o strongarm.o
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
new file mode 100644
index 0000000..2a743b7
--- /dev/null
+++ b/hw/arm/virt.c
@@ -0,0 +1,363 @@
+/*
+ * ARM mach-virt emulation
+ *
+ * Copyright (c) 2013 Linaro
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/>.
+ *
+ * Emulate a virtual board which works by passing Linux all the information
+ * it needs about what devices are present via the device tree.
+ * There are some restrictions about what we can do here:
+ *  + we can only present devices whose Linux drivers will work based
+ *    purely on the device tree with no platform data at all
+ *  + we want to present a very stripped-down minimalist platform,
+ *    both because this reduces the security attack surface from the guest
+ *    and also because it reduces our exposure to being broken when
+ *    the kernel updates its device tree bindings and requires further
+ *    information in a device binding that we aren't providing.
+ * This is essentially the same approach kvmtool uses.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/arm/arm.h"
+#include "hw/arm/primecell.h"
+#include "hw/devices.h"
+#include "net/net.h"
+#include "sysemu/device_tree.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/kvm.h"
+#include "hw/boards.h"
+#include "exec/address-spaces.h"
+#include "qemu/bitops.h"
+#include "qemu/error-report.h"
+
+#define NUM_VIRTIO_TRANSPORTS 32
+
+#define GIC_FDT_IRQ_TYPE_SPI 0
+#define GIC_FDT_IRQ_TYPE_PPI 1
+
+#define GIC_FDT_IRQ_FLAGS_EDGE_LO_HI 1
+#define GIC_FDT_IRQ_FLAGS_EDGE_HI_LO 2
+#define GIC_FDT_IRQ_FLAGS_LEVEL_HI 4
+#define GIC_FDT_IRQ_FLAGS_LEVEL_LO 8
+
+#define GIC_FDT_IRQ_PPI_CPU_START 8
+#define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
+
+enum {
+    VIRT_FLASH,
+    VIRT_MEM,
+    VIRT_CPUPERIPHS,
+    VIRT_GIC_DIST,
+    VIRT_GIC_CPU,
+    VIRT_MMIO,
+};
+
+typedef struct MemMapEntry {
+    hwaddr base;
+    hwaddr size;
+} MemMapEntry;
+
+typedef struct VirtBoardInfo {
+    struct arm_boot_info bootinfo;
+    const char *cpu_model;
+    const char *cpu_compatible;
+    const char *qdevname;
+    const char *gic_compatible;
+    const MemMapEntry *memmap;
+    int smp_cpus;
+    void *fdt;
+    int fdt_size;
+} VirtBoardInfo;
+
+/* Addresses and sizes of our components.
+ * We leave the first 64K free for possible use later for
+ * flash (for running boot code such as UEFI); following
+ * that is I/O, and then everything else is RAM (which may
+ * happily spill over into the high memory region beyond 4GB).
+ */
+static const MemMapEntry a15memmap[] = {
+    [VIRT_FLASH] = { 0, 0x100000 },
+    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },
+    /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
+    [VIRT_GIC_DIST] = { 0x101000, 0x1000 },
+    [VIRT_GIC_CPU] = { 0x102000, 0x1000 },
+    [VIRT_MMIO] = { 0x108000, 0x200 },
+    /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
+    [VIRT_MEM] = { 0x8000000, 30ULL * 1024 * 1024 * 1024 },
+};
+
+static VirtBoardInfo machines[] = {
+    {
+        .cpu_model = "cortex-a15",
+        .cpu_compatible = "arm,cortex-a15",
+        .qdevname = "a15mpcore_priv",
+        .gic_compatible = "arm,cortex-a15-gic",
+        .memmap = a15memmap,
+    },
+};
+
+static VirtBoardInfo *find_machine_info(const char *cpu)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(machines); i++) {
+        if (strcmp(cpu, machines[i].cpu_model) == 0) {
+            return &machines[i];
+        }
+    }
+    return NULL;
+}
+
+static void create_fdt(VirtBoardInfo *vbi)
+{
+    void *fdt = create_device_tree(&vbi->fdt_size);
+
+    if (!fdt) {
+        error_report("create_device_tree() failed");
+        exit(1);
+    }
+
+    vbi->fdt = fdt;
+
+    /* Header */
+    qemu_devtree_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
+    qemu_devtree_setprop_cell(fdt, "/", "#address-cells", 0x2);
+    qemu_devtree_setprop_cell(fdt, "/", "#size-cells", 0x2);
+
+    /*
+     * /chosen and /memory nodes must exist for load_dtb
+     * to fill in necessary properties later
+     */
+    qemu_devtree_add_subnode(fdt, "/chosen");
+    qemu_devtree_add_subnode(fdt, "/memory");
+    qemu_devtree_setprop_string(fdt, "/memory", "device_type", "memory");
+
+    /* No PSCI for TCG yet */
+#ifdef CONFIG_KVM
+    if (kvm_enabled()) {
+        qemu_devtree_add_subnode(fdt, "/psci");
+        qemu_devtree_setprop_string(fdt, "/psci", "compatible", "arm,psci");
+        qemu_devtree_setprop_string(fdt, "/psci", "method", "hvc");
+        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_suspend",
+                                  KVM_PSCI_FN_CPU_SUSPEND);
+        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_off", KVM_PSCI_FN_CPU_OFF);
+        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_on", KVM_PSCI_FN_CPU_ON);
+        qemu_devtree_setprop_cell(fdt, "/psci", "migrate", KVM_PSCI_FN_MIGRATE);
+    }
+#endif
+}
+
+static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
+{
+    /* Note that on A15 h/w these interrupts are level-triggered,
+     * but for the GIC implementation provided by both QEMU and KVM
+     * they are edge-triggered.
+     */
+    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
+
+    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
+                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
+
+    qemu_devtree_add_subnode(vbi->fdt, "/timer");
+    qemu_devtree_setprop_string(vbi->fdt, "/timer",
+                                "compatible", "arm,armv7-timer");
+    qemu_devtree_setprop_cells(vbi->fdt, "/timer", "interrupts",
+                               GIC_FDT_IRQ_TYPE_PPI, 13, irqflags,
+                               GIC_FDT_IRQ_TYPE_PPI, 14, irqflags,
+                               GIC_FDT_IRQ_TYPE_PPI, 11, irqflags,
+                               GIC_FDT_IRQ_TYPE_PPI, 10, irqflags);
+}
+
+static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
+{
+    int cpu;
+
+    qemu_devtree_add_subnode(vbi->fdt, "/cpus");
+    qemu_devtree_setprop_cell(vbi->fdt, "/cpus", "#address-cells", 0x1);
+    qemu_devtree_setprop_cell(vbi->fdt, "/cpus", "#size-cells", 0x0);
+
+    for (cpu = 0; cpu < vbi->smp_cpus; cpu++) {
+        char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
+
+        qemu_devtree_add_subnode(vbi->fdt, nodename);
+        qemu_devtree_setprop_string(vbi->fdt, nodename, "device_type", "cpu");
+        qemu_devtree_setprop_string(vbi->fdt, nodename, "compatible",
+                                    vbi->cpu_compatible);
+
+        if (vbi->smp_cpus > 1) {
+            qemu_devtree_setprop_string(vbi->fdt, nodename,
+                                        "enable-method", "psci");
+        }
+
+        qemu_devtree_setprop_cell(vbi->fdt, nodename, "reg", cpu);
+        g_free(nodename);
+    }
+}
+
+static void fdt_add_gic_node(const VirtBoardInfo *vbi)
+{
+    uint32_t gic_phandle;
+
+    gic_phandle = qemu_devtree_alloc_phandle(vbi->fdt);
+    qemu_devtree_setprop_cell(vbi->fdt, "/", "interrupt-parent", gic_phandle);
+
+    qemu_devtree_add_subnode(vbi->fdt, "/intc");
+    qemu_devtree_setprop_string(vbi->fdt, "/intc", "compatible",
+                                vbi->gic_compatible);
+    qemu_devtree_setprop_cell(vbi->fdt, "/intc", "#interrupt-cells", 3);
+    qemu_devtree_setprop(vbi->fdt, "/intc", "interrupt-controller", NULL, 0);
+    qemu_devtree_setprop_sized_cells(vbi->fdt, "/intc", "reg",
+                                     2, vbi->memmap[VIRT_GIC_DIST].base,
+                                     2, vbi->memmap[VIRT_GIC_DIST].size,
+                                     2, vbi->memmap[VIRT_GIC_CPU].base,
+                                     2, vbi->memmap[VIRT_GIC_CPU].size);
+    qemu_devtree_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle);
+}
+
+static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    int i;
+    hwaddr base = vbi->memmap[VIRT_MMIO].base;
+    hwaddr size = vbi->memmap[VIRT_MMIO].size;
+
+    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
+        char *nodename;
+        int irq = i + 16;
+
+        sysbus_create_simple("virtio-mmio", base, pic[irq]);
+
+        nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
+        qemu_devtree_add_subnode(vbi->fdt, nodename);
+        qemu_devtree_setprop_string(vbi->fdt, nodename,
+                                    "compatible", "virtio,mmio");
+        qemu_devtree_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                         2, base, 2, size);
+        qemu_devtree_setprop_cells(vbi->fdt, nodename, "interrupts",
+                                   GIC_FDT_IRQ_TYPE_SPI, irq,
+                                   GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+        g_free(nodename);
+        base += size;
+    }
+}
+
+static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
+{
+    const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
+
+    *fdt_size = board->fdt_size;
+    return board->fdt;
+}
+
+static void machvirt_init(QEMUMachineInitArgs *args)
+{
+    qemu_irq pic[64];
+    MemoryRegion *sysmem = get_system_memory();
+    int n;
+    MemoryRegion *ram = g_new(MemoryRegion, 1);
+    qemu_irq cpu_irq[4];
+    DeviceState *dev;
+    SysBusDevice *busdev;
+    const char *cpu_model = args->cpu_model;
+    VirtBoardInfo *vbi;
+
+    if (!cpu_model) {
+        cpu_model = "cortex-a15";
+    }
+
+    vbi = find_machine_info(cpu_model);
+
+    if (!vbi) {
+        error_report("mach-virt: CPU %s not supported", cpu_model);
+        exit(1);
+    }
+
+    vbi->smp_cpus = smp_cpus;
+
+    /*
+     * Only supported method of starting secondary CPUs is PSCI and
+     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
+     * if we're not using KVM.
+     */
+    if (!kvm_enabled() && smp_cpus > 1) {
+        error_report("mach-virt: must enable KVM to use multiple CPUs");
+        exit(1);
+    }
+
+    if (args->ram_size > vbi->memmap[VIRT_MEM].size) {
+        error_report("mach-virt: cannot model more than 30GB RAM");
+        exit(1);
+    }
+
+    create_fdt(vbi);
+    fdt_add_timer_nodes(vbi);
+
+    for (n = 0; n < smp_cpus; n++) {
+        ARMCPU *cpu;
+        qemu_irq *irqp;
+
+        cpu = cpu_arm_init(cpu_model);
+        irqp = arm_pic_init_cpu(cpu);
+        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
+    }
+    fdt_add_cpu_nodes(vbi);
+
+    memory_region_init_ram(ram, NULL, "mach-virt.ram", args->ram_size);
+    vmstate_register_ram_global(ram);
+    memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
+
+    dev = qdev_create(NULL, vbi->qdevname);
+    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
+    qdev_init_nofail(dev);
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(busdev, 0, vbi->memmap[VIRT_CPUPERIPHS].base);
+    fdt_add_gic_node(vbi);
+    for (n = 0; n < smp_cpus; n++) {
+        sysbus_connect_irq(busdev, n, cpu_irq[n]);
+    }
+
+    for (n = 0; n < 64; n++) {
+        pic[n] = qdev_get_gpio_in(dev, n);
+    }
+
+    /* Create mmio transports, so the user can create virtio backends
+     * (which will be automatically plugged in to the transports). If
+     * no backend is created the transport will just sit harmlessly idle.
+     */
+    create_virtio_devices(vbi, pic);
+
+    vbi->bootinfo.ram_size = args->ram_size;
+    vbi->bootinfo.kernel_filename = args->kernel_filename;
+    vbi->bootinfo.kernel_cmdline = args->kernel_cmdline;
+    vbi->bootinfo.initrd_filename = args->initrd_filename;
+    vbi->bootinfo.nb_cpus = smp_cpus;
+    vbi->bootinfo.board_id = -1;
+    vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
+    vbi->bootinfo.get_dtb = machvirt_dtb;
+    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
+}
+
+static QEMUMachine machvirt_a15_machine = {
+    .name = "virt",
+    .desc = "ARM Virtual Machine",
+    .init = machvirt_init,
+    .max_cpus = 4,
+    DEFAULT_MACHINE_OPTIONS,
+};
+
+static void machvirt_machine_init(void)
+{
+    qemu_register_machine(&machvirt_a15_machine);
+}
+
+machine_init(machvirt_machine_init);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Peter Maydell]

On 5 August 2013 12:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: John Rigby <john.rigby@linaro.org>
>
> Add 'virt' platform support corresponding to arch/arm/mach-virt
> in the Linux kernel tree. This has no platform-specific code but
> can use any device whose kernel driver is is able to work purely
> from a device tree node. We use this to instantiate a minimal
> set of devices: a GIC and some virtio-mmio transports.
>
> TODO:
>  * MAX_MEM for A15 could be higher if we shuffle things about
>  * RAM or flash at addr 0??

Doh. I addressed these TODO items in the code but forgot to
remove them from the commit message. Please ignore for
purposes of code review :-)

-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Anup Patel]

On Mon, Aug 5, 2013 at 4:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: John Rigby <john.rigby@linaro.org>
>
> Add 'virt' platform support corresponding to arch/arm/mach-virt
> in the Linux kernel tree. This has no platform-specific code but
> can use any device whose kernel driver is is able to work purely
> from a device tree node. We use this to instantiate a minimal
> set of devices: a GIC and some virtio-mmio transports.
>
> TODO:
>  * MAX_MEM for A15 could be higher if we shuffle things about
>  * RAM or flash at addr 0??
>
> Signed-off-by: John Rigby <john.rigby@linaro.org>
> [PMM:
>  Significantly overhauled:
>  * renamed user-facing machine to just "virt"
>  * removed the A9 support (it can't work since the A9 has no
>    generic timers)
>  * added virtio-mmio transports instead of random set of 'soc' devices
>  * instead of updating io_base as we step through adding devices,
>    define a memory map with an array (similar to vexpress)
>  * folded in some minor fixes from John's aarch64-support patch
>  * rather than explicitly doing endian-swapping on FDT cells,
>    use fdt APIs that let us just pass in host-endian values
>    and let the fdt layer take care of the swapping
>  * miscellaneous minor code cleanups and style fixes
> ]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/Makefile.objs |    2 +-
>  hw/arm/virt.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 364 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/virt.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 9e3a06f..744484f 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -1,7 +1,7 @@
>  obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>  obj-y += omap_sx1.o palm.o pic_cpu.o realview.o spitz.o stellaris.o
> -obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
> +obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>
>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>  obj-y += omap1.o omap2.o strongarm.o
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> new file mode 100644
> index 0000000..2a743b7
> --- /dev/null
> +++ b/hw/arm/virt.c
> @@ -0,0 +1,363 @@
> +/*
> + * ARM mach-virt emulation
> + *
> + * Copyright (c) 2013 Linaro
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/>.
> + *
> + * Emulate a virtual board which works by passing Linux all the information
> + * it needs about what devices are present via the device tree.
> + * There are some restrictions about what we can do here:
> + *  + we can only present devices whose Linux drivers will work based
> + *    purely on the device tree with no platform data at all
> + *  + we want to present a very stripped-down minimalist platform,
> + *    both because this reduces the security attack surface from the guest
> + *    and also because it reduces our exposure to being broken when
> + *    the kernel updates its device tree bindings and requires further
> + *    information in a device binding that we aren't providing.
> + * This is essentially the same approach kvmtool uses.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/arm/arm.h"
> +#include "hw/arm/primecell.h"
> +#include "hw/devices.h"
> +#include "net/net.h"
> +#include "sysemu/device_tree.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/kvm.h"
> +#include "hw/boards.h"
> +#include "exec/address-spaces.h"
> +#include "qemu/bitops.h"
> +#include "qemu/error-report.h"
> +
> +#define NUM_VIRTIO_TRANSPORTS 32
> +
> +#define GIC_FDT_IRQ_TYPE_SPI 0
> +#define GIC_FDT_IRQ_TYPE_PPI 1
> +
> +#define GIC_FDT_IRQ_FLAGS_EDGE_LO_HI 1
> +#define GIC_FDT_IRQ_FLAGS_EDGE_HI_LO 2
> +#define GIC_FDT_IRQ_FLAGS_LEVEL_HI 4
> +#define GIC_FDT_IRQ_FLAGS_LEVEL_LO 8
> +
> +#define GIC_FDT_IRQ_PPI_CPU_START 8
> +#define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
> +
> +enum {
> +    VIRT_FLASH,
> +    VIRT_MEM,
> +    VIRT_CPUPERIPHS,
> +    VIRT_GIC_DIST,
> +    VIRT_GIC_CPU,
> +    VIRT_MMIO,
> +};
> +
> +typedef struct MemMapEntry {
> +    hwaddr base;
> +    hwaddr size;
> +} MemMapEntry;
> +
> +typedef struct VirtBoardInfo {
> +    struct arm_boot_info bootinfo;
> +    const char *cpu_model;
> +    const char *cpu_compatible;
> +    const char *qdevname;
> +    const char *gic_compatible;
> +    const MemMapEntry *memmap;
> +    int smp_cpus;
> +    void *fdt;
> +    int fdt_size;
> +} VirtBoardInfo;
> +
> +/* Addresses and sizes of our components.
> + * We leave the first 64K free for possible use later for
> + * flash (for running boot code such as UEFI); following
> + * that is I/O, and then everything else is RAM (which may
> + * happily spill over into the high memory region beyond 4GB).
> + */
> +static const MemMapEntry a15memmap[] = {
> +    [VIRT_FLASH] = { 0, 0x100000 },

IMHO, 1 MB of flash is small for possible future expansion. If mach-virt
becomes popular then we can expect people running UBoot or UEFI or
.... from this flash.

I think having 16 MB of flash would be good because it is multiple of
2 MB hence we can also create section entries for it in Stage2 TTBL.

> +    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },

I would suggest to start peripheral space at 2 MB boundary and also
have its total size in multiple of 2 MB. This will enable us to create 2 MB
entries in Stage2 TTBL for trapping which in-turn can help in performance
by reducing Stage2 TTBL walks.

I am sure there won't be too many peripherals in mach-virt so, it would
even better if we can have base address of each peripheral to be at
2 MB boundary.

> +    /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
> +    [VIRT_GIC_DIST] = { 0x101000, 0x1000 },
> +    [VIRT_GIC_CPU] = { 0x102000, 0x1000 },
> +    [VIRT_MMIO] = { 0x108000, 0x200 },
> +    /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> +    [VIRT_MEM] = { 0x8000000, 30ULL * 1024 * 1024 * 1024 },
> +};
> +
> +static VirtBoardInfo machines[] = {
> +    {
> +        .cpu_model = "cortex-a15",
> +        .cpu_compatible = "arm,cortex-a15",
> +        .qdevname = "a15mpcore_priv",
> +        .gic_compatible = "arm,cortex-a15-gic",
> +        .memmap = a15memmap,
> +    },
> +};
> +
> +static VirtBoardInfo *find_machine_info(const char *cpu)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(machines); i++) {
> +        if (strcmp(cpu, machines[i].cpu_model) == 0) {
> +            return &machines[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void create_fdt(VirtBoardInfo *vbi)
> +{
> +    void *fdt = create_device_tree(&vbi->fdt_size);
> +
> +    if (!fdt) {
> +        error_report("create_device_tree() failed");
> +        exit(1);
> +    }
> +
> +    vbi->fdt = fdt;
> +
> +    /* Header */
> +    qemu_devtree_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
> +    qemu_devtree_setprop_cell(fdt, "/", "#address-cells", 0x2);
> +    qemu_devtree_setprop_cell(fdt, "/", "#size-cells", 0x2);
> +
> +    /*
> +     * /chosen and /memory nodes must exist for load_dtb
> +     * to fill in necessary properties later
> +     */
> +    qemu_devtree_add_subnode(fdt, "/chosen");
> +    qemu_devtree_add_subnode(fdt, "/memory");
> +    qemu_devtree_setprop_string(fdt, "/memory", "device_type", "memory");
> +
> +    /* No PSCI for TCG yet */
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled()) {
> +        qemu_devtree_add_subnode(fdt, "/psci");
> +        qemu_devtree_setprop_string(fdt, "/psci", "compatible", "arm,psci");
> +        qemu_devtree_setprop_string(fdt, "/psci", "method", "hvc");
> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_suspend",
> +                                  KVM_PSCI_FN_CPU_SUSPEND);
> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_off", KVM_PSCI_FN_CPU_OFF);
> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_on", KVM_PSCI_FN_CPU_ON);
> +        qemu_devtree_setprop_cell(fdt, "/psci", "migrate", KVM_PSCI_FN_MIGRATE);
> +    }
> +#endif
> +}
> +
> +static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
> +{
> +    /* Note that on A15 h/w these interrupts are level-triggered,
> +     * but for the GIC implementation provided by both QEMU and KVM
> +     * they are edge-triggered.
> +     */
> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
> +
> +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
> +
> +    qemu_devtree_add_subnode(vbi->fdt, "/timer");
> +    qemu_devtree_setprop_string(vbi->fdt, "/timer",
> +                                "compatible", "arm,armv7-timer");
> +    qemu_devtree_setprop_cells(vbi->fdt, "/timer", "interrupts",
> +                               GIC_FDT_IRQ_TYPE_PPI, 13, irqflags,
> +                               GIC_FDT_IRQ_TYPE_PPI, 14, irqflags,
> +                               GIC_FDT_IRQ_TYPE_PPI, 11, irqflags,
> +                               GIC_FDT_IRQ_TYPE_PPI, 10, irqflags);
> +}
> +
> +static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> +{
> +    int cpu;
> +
> +    qemu_devtree_add_subnode(vbi->fdt, "/cpus");
> +    qemu_devtree_setprop_cell(vbi->fdt, "/cpus", "#address-cells", 0x1);
> +    qemu_devtree_setprop_cell(vbi->fdt, "/cpus", "#size-cells", 0x0);
> +
> +    for (cpu = 0; cpu < vbi->smp_cpus; cpu++) {
> +        char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> +
> +        qemu_devtree_add_subnode(vbi->fdt, nodename);
> +        qemu_devtree_setprop_string(vbi->fdt, nodename, "device_type", "cpu");
> +        qemu_devtree_setprop_string(vbi->fdt, nodename, "compatible",
> +                                    vbi->cpu_compatible);
> +
> +        if (vbi->smp_cpus > 1) {
> +            qemu_devtree_setprop_string(vbi->fdt, nodename,
> +                                        "enable-method", "psci");
> +        }
> +
> +        qemu_devtree_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> +        g_free(nodename);
> +    }
> +}
> +
> +static void fdt_add_gic_node(const VirtBoardInfo *vbi)
> +{
> +    uint32_t gic_phandle;
> +
> +    gic_phandle = qemu_devtree_alloc_phandle(vbi->fdt);
> +    qemu_devtree_setprop_cell(vbi->fdt, "/", "interrupt-parent", gic_phandle);
> +
> +    qemu_devtree_add_subnode(vbi->fdt, "/intc");
> +    qemu_devtree_setprop_string(vbi->fdt, "/intc", "compatible",
> +                                vbi->gic_compatible);
> +    qemu_devtree_setprop_cell(vbi->fdt, "/intc", "#interrupt-cells", 3);
> +    qemu_devtree_setprop(vbi->fdt, "/intc", "interrupt-controller", NULL, 0);
> +    qemu_devtree_setprop_sized_cells(vbi->fdt, "/intc", "reg",
> +                                     2, vbi->memmap[VIRT_GIC_DIST].base,
> +                                     2, vbi->memmap[VIRT_GIC_DIST].size,
> +                                     2, vbi->memmap[VIRT_GIC_CPU].base,
> +                                     2, vbi->memmap[VIRT_GIC_CPU].size);
> +    qemu_devtree_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle);
> +}
> +
> +static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    int i;
> +    hwaddr base = vbi->memmap[VIRT_MMIO].base;
> +    hwaddr size = vbi->memmap[VIRT_MMIO].size;
> +
> +    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
> +        char *nodename;
> +        int irq = i + 16;
> +
> +        sysbus_create_simple("virtio-mmio", base, pic[irq]);
> +
> +        nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
> +        qemu_devtree_add_subnode(vbi->fdt, nodename);
> +        qemu_devtree_setprop_string(vbi->fdt, nodename,
> +                                    "compatible", "virtio,mmio");
> +        qemu_devtree_setprop_sized_cells(vbi->fdt, nodename, "reg",
> +                                         2, base, 2, size);
> +        qemu_devtree_setprop_cells(vbi->fdt, nodename, "interrupts",
> +                                   GIC_FDT_IRQ_TYPE_SPI, irq,
> +                                   GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> +        g_free(nodename);
> +        base += size;
> +    }
> +}
> +
> +static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> +{
> +    const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
> +
> +    *fdt_size = board->fdt_size;
> +    return board->fdt;
> +}
> +
> +static void machvirt_init(QEMUMachineInitArgs *args)
> +{
> +    qemu_irq pic[64];
> +    MemoryRegion *sysmem = get_system_memory();
> +    int n;
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    qemu_irq cpu_irq[4];
> +    DeviceState *dev;
> +    SysBusDevice *busdev;
> +    const char *cpu_model = args->cpu_model;
> +    VirtBoardInfo *vbi;
> +
> +    if (!cpu_model) {
> +        cpu_model = "cortex-a15";
> +    }
> +
> +    vbi = find_machine_info(cpu_model);
> +
> +    if (!vbi) {
> +        error_report("mach-virt: CPU %s not supported", cpu_model);
> +        exit(1);
> +    }
> +
> +    vbi->smp_cpus = smp_cpus;
> +
> +    /*
> +     * Only supported method of starting secondary CPUs is PSCI and
> +     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
> +     * if we're not using KVM.
> +     */
> +    if (!kvm_enabled() && smp_cpus > 1) {
> +        error_report("mach-virt: must enable KVM to use multiple CPUs");
> +        exit(1);
> +    }
> +
> +    if (args->ram_size > vbi->memmap[VIRT_MEM].size) {
> +        error_report("mach-virt: cannot model more than 30GB RAM");
> +        exit(1);
> +    }
> +
> +    create_fdt(vbi);
> +    fdt_add_timer_nodes(vbi);
> +
> +    for (n = 0; n < smp_cpus; n++) {
> +        ARMCPU *cpu;
> +        qemu_irq *irqp;
> +
> +        cpu = cpu_arm_init(cpu_model);
> +        irqp = arm_pic_init_cpu(cpu);
> +        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
> +    }
> +    fdt_add_cpu_nodes(vbi);
> +
> +    memory_region_init_ram(ram, NULL, "mach-virt.ram", args->ram_size);
> +    vmstate_register_ram_global(ram);
> +    memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
> +
> +    dev = qdev_create(NULL, vbi->qdevname);
> +    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
> +    qdev_init_nofail(dev);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(busdev, 0, vbi->memmap[VIRT_CPUPERIPHS].base);
> +    fdt_add_gic_node(vbi);
> +    for (n = 0; n < smp_cpus; n++) {
> +        sysbus_connect_irq(busdev, n, cpu_irq[n]);
> +    }
> +
> +    for (n = 0; n < 64; n++) {
> +        pic[n] = qdev_get_gpio_in(dev, n);
> +    }
> +
> +    /* Create mmio transports, so the user can create virtio backends
> +     * (which will be automatically plugged in to the transports). If
> +     * no backend is created the transport will just sit harmlessly idle.
> +     */
> +    create_virtio_devices(vbi, pic);
> +
> +    vbi->bootinfo.ram_size = args->ram_size;
> +    vbi->bootinfo.kernel_filename = args->kernel_filename;
> +    vbi->bootinfo.kernel_cmdline = args->kernel_cmdline;
> +    vbi->bootinfo.initrd_filename = args->initrd_filename;
> +    vbi->bootinfo.nb_cpus = smp_cpus;
> +    vbi->bootinfo.board_id = -1;
> +    vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
> +    vbi->bootinfo.get_dtb = machvirt_dtb;
> +    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
> +}
> +
> +static QEMUMachine machvirt_a15_machine = {
> +    .name = "virt",
> +    .desc = "ARM Virtual Machine",
> +    .init = machvirt_init,
> +    .max_cpus = 4,
> +    DEFAULT_MACHINE_OPTIONS,
> +};
> +
> +static void machvirt_machine_init(void)
> +{
> +    qemu_register_machine(&machvirt_a15_machine);
> +}
> +
> +machine_init(machvirt_machine_init);
> --
> 1.7.9.5
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

--Anup

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Peter Maydell]

On 5 August 2013 12:48, Anup Patel <anup@brainfault.org> wrote:
>> +static const MemMapEntry a15memmap[] = {
>> +    [VIRT_FLASH] = { 0, 0x100000 },
>
> IMHO, 1 MB of flash is small for possible future expansion. If mach-virt
> becomes popular then we can expect people running UBoot or UEFI or
> .... from this flash.
>
> I think having 16 MB of flash would be good because it is multiple of
> 2 MB hence we can also create section entries for it in Stage2 TTBL.

Seems reasonable.

>> +    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },
>
> I would suggest to start peripheral space at 2 MB boundary and also
> have its total size in multiple of 2 MB.

The total size here is fixed by the CPU hardware -- an A15's
private peripheral space is only 0x8000 in size.

> This will enable us to create 2 MB
> entries in Stage2 TTBL for trapping which in-turn can help in performance
> by reducing Stage2 TTBL walks.
>
> I am sure there won't be too many peripherals in mach-virt so, it would
> even better if we can have base address of each peripheral to be at
> 2 MB boundary.

Why does each individual peripheral have to be at a 2MB boundary?
I would expect the kernel to just have a single "nothing mapped"
here stage 2 table entry (or entries) covering the whole of the
mmio peripheral region regardless of how many devices happen
to be inside it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Anup Patel]

On Mon, Aug 5, 2013 at 5:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 August 2013 12:48, Anup Patel <anup@brainfault.org> wrote:
>>> +static const MemMapEntry a15memmap[] = {
>>> +    [VIRT_FLASH] = { 0, 0x100000 },
>>
>> IMHO, 1 MB of flash is small for possible future expansion. If mach-virt
>> becomes popular then we can expect people running UBoot or UEFI or
>> .... from this flash.
>>
>> I think having 16 MB of flash would be good because it is multiple of
>> 2 MB hence we can also create section entries for it in Stage2 TTBL.
>
> Seems reasonable.
>
>>> +    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },
>>
>> I would suggest to start peripheral space at 2 MB boundary and also
>> have its total size in multiple of 2 MB.
>
> The total size here is fixed by the CPU hardware -- an A15's
> private peripheral space is only 0x8000 in size.

Does this mean, mach-virt address space is Cortex-A15 specific ?
What about Cortex-A7 and Cortex-A12  ?

If above is a mandatory constraint then we can have peripheral space divide
into two parts:
1. Essential (or MPCore) peripherals: For now only GIC belongs here. Other
things that fit here is Watchdog and Local timers but this are redundant for
mach-virt. This space can be only 0x8000 in size as required by Cortex-A15.
2. General peripherals: All VirtIO devices would belong here. In addition,
users can also add devices to this space using QEMU command line.

>
>> This will enable us to create 2 MB
>> entries in Stage2 TTBL for trapping which in-turn can help in performance
>> by reducing Stage2 TTBL walks.
>>
>> I am sure there won't be too many peripherals in mach-virt so, it would
>> even better if we can have base address of each peripheral to be at
>> 2 MB boundary.
>
> Why does each individual peripheral have to be at a 2MB boundary?
> I would expect the kernel to just have a single "nothing mapped"
> here stage 2 table entry (or entries) covering the whole of the
> mmio peripheral region regardless of how many devices happen
> to be inside it.

Yes, this seem a little over-kill but we can atleast have peripheral space to
be 2MB aligned with total size in multiple of 2MB.

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Peter Maydell]

On 5 August 2013 13:22, Anup Patel <anup@brainfault.org> wrote:
> On Mon, Aug 5, 2013 at 5:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 August 2013 12:48, Anup Patel <anup@brainfault.org> wrote:
>>>> +static const MemMapEntry a15memmap[] = {
>>>> +    [VIRT_FLASH] = { 0, 0x100000 },
>>>
>>> IMHO, 1 MB of flash is small for possible future expansion. If mach-virt
>>> becomes popular then we can expect people running UBoot or UEFI or
>>> .... from this flash.
>>>
>>> I think having 16 MB of flash would be good because it is multiple of
>>> 2 MB hence we can also create section entries for it in Stage2 TTBL.
>>
>> Seems reasonable.
>>
>>>> +    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },
>>>
>>> I would suggest to start peripheral space at 2 MB boundary and also
>>> have its total size in multiple of 2 MB.
>>
>> The total size here is fixed by the CPU hardware -- an A15's
>> private peripheral space is only 0x8000 in size.
>
> Does this mean, mach-virt address space is Cortex-A15 specific ?

The patch has support for an array of VirtBoardInfo structs,
each of which has its own memmap. The only entry at the
moment is for the A15.

> What about Cortex-A7 and Cortex-A12  ?

QEMU supports neither of these CPUs. The A7's the same as
the A15, though.

> If above is a mandatory constraint then we can have peripheral space divide
> into two parts:
> 1. Essential (or MPCore) peripherals: For now only GIC belongs here. Other
> things that fit here is Watchdog and Local timers but this are redundant for
> mach-virt. This space can be only 0x8000 in size as required by Cortex-A15.
> 2. General peripherals: All VirtIO devices would belong here. In addition,
> users can also add devices to this space using QEMU command line.

Er, this is what the patch already does. The "CPUPERIPHS"
bit is specifically for the CPU's private peripheral region.

You can't add an MMIO device on the QEMU command line, there's
no way to wire up the memory regions and IRQs.

>> Why does each individual peripheral have to be at a 2MB boundary?
>> I would expect the kernel to just have a single "nothing mapped"
>> here stage 2 table entry (or entries) covering the whole of the
>> mmio peripheral region regardless of how many devices happen
>> to be inside it.
>
> Yes, this seem a little over-kill but we can atleast have peripheral space to
> be 2MB aligned with total size in multiple of 2MB.

I really don't want to eat 2MB for each virtio-mmio transport
in a 32 bit address space, it seems hugely wasteful unless
there's a good reason to do it.

-- PMM

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Anup Patel]

On Mon, Aug 5, 2013 at 5:58 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 August 2013 13:22, Anup Patel <anup@brainfault.org> wrote:
>> On Mon, Aug 5, 2013 at 5:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 5 August 2013 12:48, Anup Patel <anup@brainfault.org> wrote:
>>>>> +static const MemMapEntry a15memmap[] = {
>>>>> +    [VIRT_FLASH] = { 0, 0x100000 },
>>>>
>>>> IMHO, 1 MB of flash is small for possible future expansion. If mach-virt
>>>> becomes popular then we can expect people running UBoot or UEFI or
>>>> .... from this flash.
>>>>
>>>> I think having 16 MB of flash would be good because it is multiple of
>>>> 2 MB hence we can also create section entries for it in Stage2 TTBL.
>>>
>>> Seems reasonable.
>>>
>>>>> +    [VIRT_CPUPERIPHS] = { 0x100000, 0x8000 },
>>>>
>>>> I would suggest to start peripheral space at 2 MB boundary and also
>>>> have its total size in multiple of 2 MB.
>>>
>>> The total size here is fixed by the CPU hardware -- an A15's
>>> private peripheral space is only 0x8000 in size.
>>
>> Does this mean, mach-virt address space is Cortex-A15 specific ?
>
> The patch has support for an array of VirtBoardInfo structs,
> each of which has its own memmap. The only entry at the
> moment is for the A15.
>
>> What about Cortex-A7 and Cortex-A12  ?
>
> QEMU supports neither of these CPUs. The A7's the same as
> the A15, though.
>
>> If above is a mandatory constraint then we can have peripheral space divide
>> into two parts:
>> 1. Essential (or MPCore) peripherals: For now only GIC belongs here. Other
>> things that fit here is Watchdog and Local timers but this are redundant for
>> mach-virt. This space can be only 0x8000 in size as required by Cortex-A15.
>> 2. General peripherals: All VirtIO devices would belong here. In addition,
>> users can also add devices to this space using QEMU command line.
>
> Er, this is what the patch already does. The "CPUPERIPHS"
> bit is specifically for the CPU's private peripheral region.
>
> You can't add an MMIO device on the QEMU command line, there's
> no way to wire up the memory regions and IRQs.
>
>>> Why does each individual peripheral have to be at a 2MB boundary?
>>> I would expect the kernel to just have a single "nothing mapped"
>>> here stage 2 table entry (or entries) covering the whole of the
>>> mmio peripheral region regardless of how many devices happen
>>> to be inside it.
>>
>> Yes, this seem a little over-kill but we can atleast have peripheral space to
>> be 2MB aligned with total size in multiple of 2MB.
>
> I really don't want to eat 2MB for each virtio-mmio transport
> in a 32 bit address space, it seems hugely wasteful unless
> there's a good reason to do it.

I am not suggesting to give 2MB space to each virtio-mmio transport.

What I really meant was to start VIRT_MMIO space (where all the
virtio-mmio transport would be added) to start at 2MB aligned address
and have total size (include all virtio-mmio transports) to be in-multiple
of 2MB so that we can trap access to all virtio-mmio transports using
1-2 2MB entries in Stage2.

>
> -- PMM

--Anup

Re: [Qemu-devel] [PATCH v4 2/2] ARM: Add 'virt' platform

[Peter Maydell]

On 5 August 2013 13:37, Anup Patel <anup@brainfault.org> wrote:
> On Mon, Aug 5, 2013 at 5:58 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I really don't want to eat 2MB for each virtio-mmio transport
>> in a 32 bit address space, it seems hugely wasteful unless
>> there's a good reason to do it.
>
> I am not suggesting to give 2MB space to each virtio-mmio transport.
>
> What I really meant was to start VIRT_MMIO space (where all the
> virtio-mmio transport would be added) to start at 2MB aligned address
> and have total size (include all virtio-mmio transports) to be in-multiple
> of 2MB so that we can trap access to all virtio-mmio transports using
> 1-2 2MB entries in Stage2.

Yes, that's fine. That would give us:

static const MemMapEntry a15memmap[] = {
    [VIRT_FLASH] = { 0, 0x1000000 },
    [VIRT_CPUPERIPHS] = { 0x1000000, 0x8000 },
    /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
    [VIRT_GIC_DIST] = { 0x1001000, 0x1000 },
    [VIRT_GIC_CPU] = { 0x1002000, 0x1000 },
    [VIRT_MMIO] = { 0x1200000, 0x200 },
    /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
    [VIRT_MEM] = { 0x8000000, 30ULL * 1024 * 1024 * 1024 },
};

-- PMM

[Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Daniel P. Berrange]

On Mon, Aug 05, 2013 at 12:18:10PM +0100, Peter Maydell wrote:
> This patch series adds a 'virt' platform which uses the
> kernel's mach-virt (fully device-tree driven) support
> to create a simple minimalist platform intended for
> use for KVM VM guests. It's based on John Rigby's
> patches, but I've overhauled it a lot:

On x86, we've long had versioned machine names, so that we can
make changes in future QEMU releases without breaking guest ABI
compatibility. AFAICT, the problem has basically been ignored
on non-x86 platforms in QEMU. Given the increased interest in
ARM in particular, should we use the addition of this new 'virt'
machine type, as an opportunity to introduce versioning for
ARM too. eg make this machine be called 'virt-1.0.6' and then
have 'virt' simply be an alias that points to the most recent
version.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Peter Maydell]

On 5 August 2013 13:49, Daniel P. Berrange <berrange@redhat.com> wrote:
> On x86, we've long had versioned machine names, so that we can
> make changes in future QEMU releases without breaking guest ABI
> compatibility. AFAICT, the problem has basically been ignored
> on non-x86 platforms in QEMU.

Yes; this is deliberate on the basis that starting to do this
is accepting a huge pile of maintenance workload (ie checking
for things which change, keeping around a pile of old version
machine models, retaining migration compatibility between
old and new versions). Which isn't to say I'm against it
but it means I'm not doing it until the pushback from users
that it's necessary is pretty strong.

> Given the increased interest in
> ARM in particular, should we use the addition of this new 'virt'
> machine type, as an opportunity to introduce versioning for
> ARM too. eg make this machine be called 'virt-1.0.6' and then
> have 'virt' simply be an alias that points to the most recent
> version.

I'm not convinced we're at the point where we need to do this
yet.

-- PMM

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Anthony Liguori]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Aug 05, 2013 at 12:18:10PM +0100, Peter Maydell wrote:
>> This patch series adds a 'virt' platform which uses the
>> kernel's mach-virt (fully device-tree driven) support
>> to create a simple minimalist platform intended for
>> use for KVM VM guests. It's based on John Rigby's
>> patches, but I've overhauled it a lot:
>
> On x86, we've long had versioned machine names, so that we can
> make changes in future QEMU releases without breaking guest ABI
> compatibility. AFAICT, the problem has basically been ignored
> on non-x86 platforms in QEMU. Given the increased interest in
> ARM in particular, should we use the addition of this new 'virt'
> machine type, as an opportunity to introduce versioning for
> ARM too. eg make this machine be called 'virt-1.0.6' and then
> have 'virt' simply be an alias that points to the most recent
> version.

I've been thinking about this for SPAPR too.  Like virt, I'm not sure
the platform is stable enough for this but I expect it to be soon.

However, unlike PC, I'd like to do linear versioning and avoid bumping
at every release.

IOW, spapr-1, spapr-2, spapr-3, etc.

I think virt ought to try to do the same.

Regards,

Anthony Liguori

>
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Daniel P. Berrange]

On Mon, Aug 05, 2013 at 08:28:50AM -0500, Anthony Liguori wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Mon, Aug 05, 2013 at 12:18:10PM +0100, Peter Maydell wrote:
> >> This patch series adds a 'virt' platform which uses the
> >> kernel's mach-virt (fully device-tree driven) support
> >> to create a simple minimalist platform intended for
> >> use for KVM VM guests. It's based on John Rigby's
> >> patches, but I've overhauled it a lot:
> >
> > On x86, we've long had versioned machine names, so that we can
> > make changes in future QEMU releases without breaking guest ABI
> > compatibility. AFAICT, the problem has basically been ignored
> > on non-x86 platforms in QEMU. Given the increased interest in
> > ARM in particular, should we use the addition of this new 'virt'
> > machine type, as an opportunity to introduce versioning for
> > ARM too. eg make this machine be called 'virt-1.0.6' and then
> > have 'virt' simply be an alias that points to the most recent
> > version.
> 
> I've been thinking about this for SPAPR too.  Like virt, I'm not sure
> the platform is stable enough for this but I expect it to be soon.

BTW, do you have any tests / scripts you use to validate the machine
type stability prior to cutting releases ?

Seems like it ought to be able to have some custom initrd which
just recorded contents of various sysfs/procfs files & perhaps PCI
device config space blobs. Then just boot the QEMU release candidate
binaries with this initrd & compare the results to previously recorded
data, for each machine type ?  If the records for each machine type
were checked into GIT, you could arguably do this as part of 'make test'
so developers can see if something they're doing is breaking ABI by
mistake.

> However, unlike PC, I'd like to do linear versioning and avoid bumping
> at every release.
> 
> IOW, spapr-1, spapr-2, spapr-3, etc.
> 
> I think virt ought to try to do the same.

Any particular reason why ? I kind of like the clarity of having the
version match the release version. Avoids needing to lookup a magic
decoder ring to figure out which QEMU version maps to which machine
version.

Would you say that stable release branches should not accept any
patch that changes guest ABI, to avoid this single component version
number turning into a 2 component version on stable branches ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Daniel P. Berrange]

On Mon, Aug 05, 2013 at 02:02:54PM +0100, Peter Maydell wrote:
> On 5 August 2013 13:49, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On x86, we've long had versioned machine names, so that we can
> > make changes in future QEMU releases without breaking guest ABI
> > compatibility. AFAICT, the problem has basically been ignored
> > on non-x86 platforms in QEMU.
> 
> Yes; this is deliberate on the basis that starting to do this
> is accepting a huge pile of maintenance workload (ie checking
> for things which change, keeping around a pile of old version
> machine models, retaining migration compatibility between
> old and new versions). Which isn't to say I'm against it
> but it means I'm not doing it until the pushback from users
> that it's necessary is pretty strong.
> 
> > Given the increased interest in
> > ARM in particular, should we use the addition of this new 'virt'
> > machine type, as an opportunity to introduce versioning for
> > ARM too. eg make this machine be called 'virt-1.0.6' and then
> > have 'virt' simply be an alias that points to the most recent
> > version.
> 
> I'm not convinced we're at the point where we need to do this
> yet.

Ok, fair enough. Something to consider in the future then.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Anthony Liguori]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Aug 05, 2013 at 08:28:50AM -0500, Anthony Liguori wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>> 
>> > On Mon, Aug 05, 2013 at 12:18:10PM +0100, Peter Maydell wrote:
>> >> This patch series adds a 'virt' platform which uses the
>> >> kernel's mach-virt (fully device-tree driven) support
>> >> to create a simple minimalist platform intended for
>> >> use for KVM VM guests. It's based on John Rigby's
>> >> patches, but I've overhauled it a lot:
>> >
>> > On x86, we've long had versioned machine names, so that we can
>> > make changes in future QEMU releases without breaking guest ABI
>> > compatibility. AFAICT, the problem has basically been ignored
>> > on non-x86 platforms in QEMU. Given the increased interest in
>> > ARM in particular, should we use the addition of this new 'virt'
>> > machine type, as an opportunity to introduce versioning for
>> > ARM too. eg make this machine be called 'virt-1.0.6' and then
>> > have 'virt' simply be an alias that points to the most recent
>> > version.
>> 
>> I've been thinking about this for SPAPR too.  Like virt, I'm not sure
>> the platform is stable enough for this but I expect it to be soon.
>
> BTW, do you have any tests / scripts you use to validate the machine
> type stability prior to cutting releases ?

I use qemu-test which has a finger printing test for this purpose.

>
> Seems like it ought to be able to have some custom initrd which
> just recorded contents of various sysfs/procfs files & perhaps PCI
> device config space blobs.

That's exactly what it does in fact.

> Then just boot the QEMU release candidate
> binaries with this initrd & compare the results to previously recorded
> data, for each machine type ?  If the records for each machine type
> were checked into GIT, you could arguably do this as part of 'make test'
> so developers can see if something they're doing is breaking ABI by
> mistake.
>
>> However, unlike PC, I'd like to do linear versioning and avoid bumping
>> at every release.
>> 
>> IOW, spapr-1, spapr-2, spapr-3, etc.
>> 
>> I think virt ought to try to do the same.
>
> Any particular reason why ? I kind of like the clarity of having the
> version match the release version. Avoids needing to lookup a magic
> decoder ring to figure out which QEMU version maps to which machine
> version.

(1) reduces testing matrix by having fewer versions (2) makes people
think more carefully about whether it's really necessary to break
compatibility.

> Would you say that stable release branches should not accept any
> patch that changes guest ABI, to avoid this single component version
> number turning into a 2 component version on stable branches ?

Yes.  This is already the policy FWIW.

Regards,

Anthony Liguori

>
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] Versioned machine types for ARM/non-x86 ? (Was Re: [PATCH v4 0/2] ARM: add 'virt' platform)

[Gerd Hoffmann]

  Hi,

>>> However, unlike PC, I'd like to do linear versioning and avoid bumping
>>> at every release.
>>>
>>> IOW, spapr-1, spapr-2, spapr-3, etc.
>>>
>>> I think virt ought to try to do the same.
>>
>> Any particular reason why ? I kind of like the clarity of having the
>> version match the release version. Avoids needing to lookup a magic
>> decoder ring to figure out which QEMU version maps to which machine
>> version.

+1, /me likes the version-based naming too.  It's also easier to handle
on source code level as it makes it easier to reuse the #defines we
already have for pc compat properties.

> (1) reduces testing matrix by having fewer versions

I doubt that is going to fly.  It's not like we do new pc-* machine
types just for the snake of creating them, there is no policy we have to
have a new one for each release.  Usually we create them in case there
is an actual need, i.e. a incompatible change which needs a compat
property.  Which so far was the case for (almost?) every release.

> (2) makes people
> think more carefully about whether it's really necessary to break
> compatibility.

Often it's not about incompatibilities but about new features which we
wanna have enabled by default.

cheers,
  Gerd