[Qemu-devel] [PATCH v6 0/2] hw/arm: add 'virt' platform

[Qemu-devel] [PATCH v6 0/2] hw/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).

Changes v4->v5:
 * removed outdated TODO remarks from commit messages
Changes v5->v6:
 * adjusted the memory map as per Anup's review comments
   (actually made the changes this time!)

John Rigby (2):
  hw/arm/boot: Allow boards to provide an fdt blob
  hw/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 v6 1/2] hw/arm/boot: 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 v6 2/2] hw/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.

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..6fd6a85
--- /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, 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] = { 0x1008000, 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 v6 0/2] hw/arm: add 'virt' platform

[Alexander Graf]

On 13.08.2013, at 14:03, 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:
> 
> * 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).

Are you sure about this? Not implementing a UART / something more standardized has been an absolute nightmare on s390. Device names diverge, so distributions get confused and there's this nasty bug somewhere in virtio-console that makes your input lag when you enter a lot of data quickly on the port.

I would really prefer to just define a UART for this machine. It will make life a lot easier.


Alex

Re: [Qemu-devel] [PATCH v6 2/2] hw/arm: Add 'virt' platform

[Alexander Graf]

On 13.08.2013, at 14:03, Peter Maydell 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.
> 
> 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..6fd6a85
> --- /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, 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] = { 0x1008000, 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

Do you need this #ifdef? Are the headers really not included when CONFIG_KVM is disabled?

> +    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

Sounds like a good point in time to implement a KVM compatible PSCI interface in TCG ;).

> +     * 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];

I don't see a check for smp_cpus <= 4, but the array is size 4?

> +    }
> +    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++) {

Where does that 64 come from? I presume from the GIC? Any chance this could be put into a global define?

> +        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,

Ah, this is where smp_cpus gets limited to 4.

Why is it limited to 4? And this really should be a define that you reuse on the IRQ map above :).


Alex

Re: [Qemu-devel] [PATCH v6 0/2] hw/arm: add 'virt' platform

[Peter Maydell]

On 14 August 2013 09:52, Alexander Graf <agraf@suse.de> wrote:
> On 13.08.2013, at 14:03, Peter Maydell wrote:
>> 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).
>
> Are you sure about this?

Nope, which is why I flagged it up in the commit message.
"No UART" is the idealistic approach, but maybe it won't work...

> Not implementing a UART / something more
> standardized has been an absolute nightmare on s390. Device names
> diverge, so distributions get confused

Device names are all over the shop for ARM serial ports anyway:
no two boards are the same. (This is a Linux kernel general
design flaw -- "first serial port" should be the same /dev/
name regardless of actual hardware/driver implementing it.)
That said, "we tried this on s390 and it was a mistake" is a
pretty good argument.

> and there's this nasty bug
> somewhere in virtio-console that makes your input lag when you enter
> a lot of data quickly on the port.
>
> I would really prefer to just define a UART for this machine. It
> will make life a lot easier.

UARTs are awkward because you have to get into defining clocks
for them in the device tree as far as I can see. And you still
have the problem that the kernel doesn't know where they are
for earlyprintk purposes.

-- PMM

Re: [Qemu-devel] [PATCH v6 2/2] hw/arm: Add 'virt' platform

[Peter Maydell]

On 14 August 2013 10:02, Alexander Graf <agraf@suse.de> wrote:
>
> On 13.08.2013, at 14:03, Peter Maydell wrote:
>
>> +    /* No PSCI for TCG yet */
>> +#ifdef CONFIG_KVM
>
> Do you need this #ifdef? Are the headers really not included when CONFIG_KVM is disabled?

Yes, the KVM_PSCI_* constants are defined in the linux-headers/
kernel include files, which are only pulled in when CONFIG_KVM is
defined. (Has to be that way, because the kernel headers aren't
guaranteed to be buildable on platforms other than Linux.)

>> +    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

>> +    /*
>> +     * Only supported method of starting secondary CPUs is PSCI and
>> +     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
>
> Sounds like a good point in time to implement a KVM compatible PSCI interface in TCG ;).

It is certainly on our TODO list, but since it requires tackling the
"SMC in a non-trustzone QEMU" issue I preferred not to tangle it up
with this patchset.

>> +     * 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];
>
> I don't see a check for smp_cpus <= 4, but the array is size 4?

This array will probably go away once this is rebased on my
"get rid of arm_pic" patchset that I posted last week.

>> +    }
>> +    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++) {
>
> Where does that 64 come from? I presume from the GIC? Any chance this could be put into a global define?

Nope, it's just a decision by this board to have 64 external
GIC interrupts. We could make it 96 or 128 if we chose (and
set a property on the GIC device to match). You could argue
that we shouldn't rely on knowing what the default value for
the GIC's num-irqs property is though.

>> +static QEMUMachine machvirt_a15_machine = {
>> +    .name = "virt",
>> +    .desc = "ARM Virtual Machine",
>> +    .init = machvirt_init,
>> +    .max_cpus = 4,
>
> Ah, this is where smp_cpus gets limited to 4.
>
> Why is it limited to 4? And this really should be a define that you
> reuse on the IRQ map above :).

An A15 can only have 4 CPU cores maximum -- hardware limit.
This will have to be adjusted when virt supports non-a15 CPUs,
though.

-- PMM

Re: [Qemu-devel] [PATCH v6 2/2] hw/arm: Add 'virt' platform

[Alexander Graf]

On 14.08.2013, at 11:19, Peter Maydell wrote:

> On 14 August 2013 10:02, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 13.08.2013, at 14:03, Peter Maydell wrote:
>> 
>>> +    /* No PSCI for TCG yet */
>>> +#ifdef CONFIG_KVM
>> 
>> Do you need this #ifdef? Are the headers really not included when CONFIG_KVM is disabled?
> 
> Yes, the KVM_PSCI_* constants are defined in the linux-headers/
> kernel include files, which are only pulled in when CONFIG_KVM is
> defined. (Has to be that way, because the kernel headers aren't
> guaranteed to be buildable on platforms other than Linux.)

What a shame. But then again once you implement a compatible interface in QEMU, the defines will be available regardless so you can get rid of the #ifdef again :).

> 
>>> +    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
> 
>>> +    /*
>>> +     * Only supported method of starting secondary CPUs is PSCI and
>>> +     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
>> 
>> Sounds like a good point in time to implement a KVM compatible PSCI interface in TCG ;).
> 
> It is certainly on our TODO list, but since it requires tackling the
> "SMC in a non-trustzone QEMU" issue I preferred not to tangle it up
> with this patchset.

I agree.

> 
>>> +     * 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];
>> 
>> I don't see a check for smp_cpus <= 4, but the array is size 4?
> 
> This array will probably go away once this is rebased on my
> "get rid of arm_pic" patchset that I posted last week.

Can you just use smp_cpus as array length for the time being?

> 
>>> +    }
>>> +    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++) {
>> 
>> Where does that 64 come from? I presume from the GIC? Any chance this could be put into a global define?
> 
> Nope, it's just a decision by this board to have 64 external
> GIC interrupts. We could make it 96 or 128 if we chose (and
> set a property on the GIC device to match). You could argue
> that we shouldn't rely on knowing what the default value for
> the GIC's num-irqs property is though.

We shouldn't, no :).

> 
>>> +static QEMUMachine machvirt_a15_machine = {
>>> +    .name = "virt",
>>> +    .desc = "ARM Virtual Machine",
>>> +    .init = machvirt_init,
>>> +    .max_cpus = 4,
>> 
>> Ah, this is where smp_cpus gets limited to 4.
>> 
>> Why is it limited to 4? And this really should be a define that you
>> reuse on the IRQ map above :).
> 
> An A15 can only have 4 CPU cores maximum -- hardware limit.
> This will have to be adjusted when virt supports non-a15 CPUs,
> though.

Ah, ok. This is the 15 machine description, so I guess imposing a15 limits is ok.


Alex