diff mbox

virtio-mmio: Kill code duplication

Message ID 018a01d0da71$7a6f52a0$6f4df7e0$@samsung.com
State New
Headers show

Commit Message

Pavel Fedin Aug. 19, 2015, 11:23 a.m. UTC
Extract common code for virtio-mmio creation and FDT node addition and
put it into reusable functions. Use new functions in vexpress and virt
machines.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/sysbus-fdt.c             | 51 +++++++++++++++++++++++++++++++
 hw/arm/vexpress.c               | 55 ++++-----------------------------
 hw/arm/virt-acpi-build.c        | 13 ++++----
 hw/arm/virt.c                   | 68 +++++------------------------------------
 hw/virtio/virtio-mmio.c         | 41 ++++++++++++++++++++++++-
 include/hw/arm/sysbus-fdt.h     | 16 ++++++++++
 include/hw/virtio/virtio-mmio.h | 38 +++++++++++++++++++++++
 7 files changed, 165 insertions(+), 117 deletions(-)
 create mode 100644 include/hw/virtio/virtio-mmio.h

Comments

Peter Maydell Aug. 19, 2015, 12:32 p.m. UTC | #1
On 19 August 2015 at 12:23, Pavel Fedin <p.fedin@samsung.com> wrote:
> Extract common code for virtio-mmio creation and FDT node addition and
> put it into reusable functions. Use new functions in vexpress and virt
> machines.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/sysbus-fdt.c             | 51 +++++++++++++++++++++++++++++++
>  hw/arm/vexpress.c               | 55 ++++-----------------------------
>  hw/arm/virt-acpi-build.c        | 13 ++++----
>  hw/arm/virt.c                   | 68 +++++------------------------------------
>  hw/virtio/virtio-mmio.c         | 41 ++++++++++++++++++++++++-
>  include/hw/arm/sysbus-fdt.h     | 16 ++++++++++
>  include/hw/virtio/virtio-mmio.h | 38 +++++++++++++++++++++++
>  7 files changed, 165 insertions(+), 117 deletions(-)

Why bother? This is adding more code than it deletes, and
is implicitly tying together details of how the these
two boards are laid out -- there's no inherent reason
that they have to be the same.

-- PMM
Pavel Fedin Aug. 19, 2015, 12:45 p.m. UTC | #2
Hello!

> Why bother? This is adding more code than it deletes

 I just don't like code duplication, wanted to do this long time ago. Additionally this enables to add support for virtio-mmio to more machines. Actually it could be used not only by ARM with little modifications.

> and is implicitly tying together details of how the these
> two boards are laid out

 What exactly do you mean? The only thing assumed by new functions is that 
virtio-mmio devices are laid out in a single addresses window, one after another. Base address and irq do not have to be the same everywhere.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
diff mbox

Patch

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 9d28797..c35abdb 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -28,6 +28,7 @@ 
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/virtio/virtio-mmio.h"
 #include "hw/arm/fdt.h"
 
 /*
@@ -245,3 +246,53 @@  void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params)
     p->notifier.notify = platform_bus_fdt_notify;
     qemu_add_machine_init_done_notifier(&p->notifier);
 }
+
+int add_virtio_mmio_fdt_nodes(hwaddr addr, int irq, unsigned int num,
+                              void *fdt, int intc)
+{
+    uint32_t acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
+    uint32_t scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
+    int i;
+
+    /* We add dtb nodes in reverse order so that they appear in the finished
+     * device tree lowest address first.
+     *
+     * Note that this mapping is independent of the virtio_mmio_create(). That
+     * loop influences virtio device to virtio transport assignment, whereas
+     * this loop controls how virtio transports are laid out in the dtb.
+     */
+    for (i = num - 1; i >= 0; i--) {
+        hwaddr base = addr + VIRTIO_MMIO_SIZE * i;
+        char *nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
+        int rc;
+
+        /* Add a virtio_mmio node to the device tree blob:
+         *   virtio_mmio@ADDRESS {
+         *       compatible = "virtio,mmio";
+         *       reg = <ADDRESS, SIZE>;
+         *       interrupt-parent = <&intc>;
+         *       interrupts = <0, irq, 1>;
+         *   }
+         * (Note that the format of the interrupts property is dependent on the
+         * interrupt controller that interrupt-parent points to; these are for
+         * the ARM GIC and indicate an SPI interrupt, rising-edge-triggered.)
+         */
+        rc = qemu_fdt_add_subnode(fdt, nodename);
+        rc |= qemu_fdt_setprop_string(fdt, nodename,
+                                      "compatible", "virtio,mmio");
+        rc |= qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells, base,
+                                           scells, VIRTIO_MMIO_SIZE);
+        if (intc) {
+            qemu_fdt_setprop_cells(fdt, nodename, "interrupt-parent", intc);
+        }
+        qemu_fdt_setprop_cells(fdt, nodename, "interrupts",
+                               GIC_FDT_IRQ_TYPE_SPI, irq + i,
+                               GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+        g_free(nodename);
+        if (rc) {
+            return -1;
+        }
+    }
+
+    return 0;
+}
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index da21788..79aa02e 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -24,7 +24,9 @@ 
 #include "hw/sysbus.h"
 #include "hw/arm/arm.h"
 #include "hw/arm/primecell.h"
+#include "hw/arm/sysbus-fdt.h"
 #include "hw/devices.h"
+#include "hw/virtio/virtio-mmio.h"
 #include "net/net.h"
 #include "sysemu/sysemu.h"
 #include "hw/boards.h"
@@ -427,38 +429,6 @@  static VEDBoardInfo a15_daughterboard = {
     .init = a15_daughterboard_init,
 };
 
-static int add_virtio_mmio_node(void *fdt, uint32_t acells, uint32_t scells,
-                                hwaddr addr, hwaddr size, uint32_t intc,
-                                int irq)
-{
-    /* Add a virtio_mmio node to the device tree blob:
-     *   virtio_mmio@ADDRESS {
-     *       compatible = "virtio,mmio";
-     *       reg = <ADDRESS, SIZE>;
-     *       interrupt-parent = <&intc>;
-     *       interrupts = <0, irq, 1>;
-     *   }
-     * (Note that the format of the interrupts property is dependent on the
-     * interrupt controller that interrupt-parent points to; these are for
-     * the ARM GIC and indicate an SPI interrupt, rising-edge-triggered.)
-     */
-    int rc;
-    char *nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, addr);
-
-    rc = qemu_fdt_add_subnode(fdt, nodename);
-    rc |= qemu_fdt_setprop_string(fdt, nodename,
-                                  "compatible", "virtio,mmio");
-    rc |= qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
-                                       acells, addr, scells, size);
-    qemu_fdt_setprop_cells(fdt, nodename, "interrupt-parent", intc);
-    qemu_fdt_setprop_cells(fdt, nodename, "interrupts", 0, irq, 1);
-    g_free(nodename);
-    if (rc) {
-        return -1;
-    }
-    return 0;
-}
-
 static uint32_t find_int_controller(void *fdt)
 {
     /* Find the FDT node corresponding to the interrupt controller
@@ -479,12 +449,9 @@  static uint32_t find_int_controller(void *fdt)
 
 static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
 {
-    uint32_t acells, scells, intc;
     const VEDBoardInfo *daughterboard = (const VEDBoardInfo *)info;
+    uint32 intc = find_int_controller(fdt);
 
-    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
-    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
-    intc = find_int_controller(fdt);
     if (!intc) {
         /* Not fatal, we just won't provide virtio. This will
          * happen with older device tree blobs.
@@ -492,17 +459,10 @@  static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
         fprintf(stderr, "QEMU: warning: couldn't find interrupt controller in "
                 "dtb; will not include virtio-mmio devices in the dtb.\n");
     } else {
-        int i;
         const hwaddr *map = daughterboard->motherboard_map;
 
-        /* We iterate backwards here because adding nodes
-         * to the dtb puts them in last-first.
-         */
-        for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
-            add_virtio_mmio_node(fdt, acells, scells,
-                                 map[VE_VIRTIO] + 0x200 * i,
-                                 0x200, intc, 40 + i);
-        }
+        add_virtio_mmio_fdt_nodes(map[VE_VIRTIO], 40, NUM_VIRTIO_TRANSPORTS,
+                                  fdt, intc);
     }
 }
 
@@ -694,10 +654,7 @@  static void vexpress_common_init(MachineState *machine)
      * (which will be automatically plugged in to the transports). If
      * no backend is created the transport will just sit harmlessly idle.
      */
-    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
-        sysbus_create_simple("virtio-mmio", map[VE_VIRTIO] + 0x200 * i,
-                             pic[40 + i]);
-    }
+    virtio_mmio_create(map[VE_VIRTIO], &pic[40], NUM_VIRTIO_TRANSPORTS);
 
     daughterboard->bootinfo.ram_size = machine->ram_size;
     daughterboard->bootinfo.kernel_filename = machine->kernel_filename;
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9088248..039c9dc 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -41,6 +41,7 @@ 
 #include "hw/acpi/aml-build.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
+#include "hw/virtio/virtio-mmio.h"
 
 #define ARM_SPI_BASE 32
 
@@ -136,11 +137,10 @@  static void acpi_dsdt_add_flash(Aml *scope, const MemMapEntry *flash_memmap)
 
 static void acpi_dsdt_add_virtio(Aml *scope,
                                  const MemMapEntry *virtio_mmio_memmap,
-                                 int mmio_irq, int num)
+                                 int irq)
 {
     hwaddr base = virtio_mmio_memmap->base;
-    hwaddr size = virtio_mmio_memmap->size;
-    int irq = mmio_irq;
+    int num = virtio_mmio_memmap->size / VIRTIO_MMIO_SIZE;
     int i;
 
     for (i = 0; i < num; i++) {
@@ -149,13 +149,14 @@  static void acpi_dsdt_add_virtio(Aml *scope,
         aml_append(dev, aml_name_decl("_UID", aml_int(i)));
 
         Aml *crs = aml_resource_template();
-        aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
+        aml_append(crs, aml_memory32_fixed(base, VIRTIO_MMIO_SIZE,
+                                           AML_READ_WRITE));
         aml_append(crs,
                    aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
                                  AML_EXCLUSIVE, irq + i));
         aml_append(dev, aml_name_decl("_CRS", crs));
         aml_append(scope, dev);
-        base += size;
+        base += VIRTIO_MMIO_SIZE;
     }
 }
 
@@ -521,7 +522,7 @@  build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
                       (irqmap[VIRT_RTC] + ARM_SPI_BASE));
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
-                    (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
+                         irqmap[VIRT_MMIO] + ARM_SPI_BASE);
     acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
                       guest_info->use_highmem);
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4c02708..379e33f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -32,6 +32,7 @@ 
 #include "hw/arm/arm.h"
 #include "hw/arm/primecell.h"
 #include "hw/arm/virt.h"
+#include "hw/virtio/virtio-mmio.h"
 #include "hw/devices.h"
 #include "net/net.h"
 #include "sysemu/block-backend.h"
@@ -120,8 +121,7 @@  static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
-    [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
-    /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
+    [VIRT_MMIO] =               { 0x0a000000, 0x00004000 }, /* 32 transports */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
     [VIRT_PCIE_MMIO] =          { 0x10000000, 0x2eff0000 },
     [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
@@ -568,66 +568,12 @@  static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
 
 static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
-    int i;
-    hwaddr size = vbi->memmap[VIRT_MMIO].size;
-
-    /* We create the transports in forwards order. Since qbus_realize()
-     * prepends (not appends) new child buses, the incrementing loop below will
-     * create a list of virtio-mmio buses with decreasing base addresses.
-     *
-     * When a -device option is processed from the command line,
-     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
-     * order. The upshot is that -device options in increasing command line
-     * order are mapped to virtio-mmio buses with decreasing base addresses.
-     *
-     * When this code was originally written, that arrangement ensured that the
-     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
-     * the first -device on the command line. (The end-to-end order is a
-     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
-     * guest kernel's name-to-address assignment strategy.)
-     *
-     * Meanwhile, the kernel's traversal seems to have been reversed; see eg.
-     * the message, if not necessarily the code, of commit 70161ff336.
-     * Therefore the loop now establishes the inverse of the original intent.
-     *
-     * Unfortunately, we can't counteract the kernel change by reversing the
-     * loop; it would break existing command lines.
-     *
-     * In any case, the kernel makes no guarantee about the stability of
-     * enumeration order of virtio devices (as demonstrated by it changing
-     * between kernel versions). For reliable and stable identification
-     * of disks users must use UUIDs or similar mechanisms.
-     */
-    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
-        int irq = vbi->irqmap[VIRT_MMIO] + i;
-        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
-
-        sysbus_create_simple("virtio-mmio", base, pic[irq]);
-    }
+    unsigned int num = vbi->memmap[VIRT_MMIO].size / VIRTIO_MMIO_SIZE;
+    int irq = vbi->irqmap[VIRT_MMIO];
 
-    /* We add dtb nodes in reverse order so that they appear in the finished
-     * device tree lowest address first.
-     *
-     * Note that this mapping is independent of the loop above. The previous
-     * loop influences virtio device to virtio transport assignment, whereas
-     * this loop controls how virtio transports are laid out in the dtb.
-     */
-    for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
-        char *nodename;
-        int irq = vbi->irqmap[VIRT_MMIO] + i;
-        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
-
-        nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
-        qemu_fdt_add_subnode(vbi->fdt, nodename);
-        qemu_fdt_setprop_string(vbi->fdt, nodename,
-                                "compatible", "virtio,mmio");
-        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
-                                     2, base, 2, size);
-        qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
-                               GIC_FDT_IRQ_TYPE_SPI, irq,
-                               GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
-        g_free(nodename);
-    }
+    virtio_mmio_create(vbi->memmap[VIRT_MMIO].base, &pic[irq], num);
+    add_virtio_mmio_fdt_nodes(vbi->memmap[VIRT_MMIO].base, irq, num,
+                              vbi->fdt, 0);
 }
 
 static void create_one_flash(const char *name, hwaddr flashbase,
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 4ad56ca..858631e 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -22,8 +22,10 @@ 
 #include "hw/sysbus.h"
 #include "hw/virtio/virtio.h"
 #include "qemu/host-utils.h"
+#include "sysemu/device_tree.h"
 #include "sysemu/kvm.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-mmio.h"
 #include "qemu/error-report.h"
 
 #define DEBUG_VIRTIO_MMIO
@@ -531,7 +533,7 @@  static void virtio_mmio_realizefn(DeviceState *d, Error **errp)
                         d, NULL);
     sysbus_init_irq(sbd, &proxy->irq);
     memory_region_init_io(&proxy->iomem, OBJECT(d), &virtio_mem_ops, proxy,
-                          TYPE_VIRTIO_MMIO, 0x200);
+                          TYPE_VIRTIO_MMIO, VIRTIO_MMIO_SIZE);
     sysbus_init_mmio(sbd, &proxy->iomem);
 }
 
@@ -581,3 +583,40 @@  static void virtio_mmio_register_types(void)
 }
 
 type_init(virtio_mmio_register_types)
+
+void virtio_mmio_create(hwaddr addr, qemu_irq *irqs, unsigned int num)
+{
+    unsigned int i;
+
+    /* We create the transports in forwards order. Since qbus_realize()
+     * prepends (not appends) new child buses, the incrementing loop below will
+     * create a list of virtio-mmio buses with decreasing base addresses.
+     *
+     * When a -device option is processed from the command line,
+     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
+     * order. The upshot is that -device options in increasing command line
+     * order are mapped to virtio-mmio buses with decreasing base addresses.
+     *
+     * When this code was originally written, that arrangement ensured that the
+     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
+     * the first -device on the command line. (The end-to-end order is a
+     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
+     * guest kernel's name-to-address assignment strategy.)
+     *
+     * Meanwhile, the kernel's traversal seems to have been reversed; see eg.
+     * the message, if not necessarily the code, of commit 70161ff336.
+     * Therefore the loop now establishes the inverse of the original intent.
+     *
+     * Unfortunately, we can't counteract the kernel change by reversing the
+     * loop; it would break existing command lines.
+     *
+     * In any case, the kernel makes no guarantee about the stability of
+     * enumeration order of virtio devices (as demonstrated by it changing
+     * between kernel versions). For reliable and stable identification
+     * of disks users must use UUIDs or similar mechanisms.
+     */
+    for (i = 0; i < num; i++) {
+        sysbus_create_simple(TYPE_VIRTIO_MMIO,
+                             addr + VIRTIO_MMIO_SIZE * i, irqs[i]);
+    }
+}
diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
index e15bb81..5646d85 100644
--- a/include/hw/arm/sysbus-fdt.h
+++ b/include/hw/arm/sysbus-fdt.h
@@ -57,4 +57,20 @@  typedef struct {
  */
 void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params);
 
+/**
+ * add_virtio_mmio_fdt_nodes:
+ * @addr: Starting physical address of virtio region
+ * @irq: IRQ number for the first device in a row
+ * @num: Number of virtio devices
+ * @fdt: Opaque pointer to the device tree
+ * @intc: Optional handle to put as a value of interrupt-parent
+ * property
+ *
+ * Insert description of virtio-mmio devices into device tree
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int add_virtio_mmio_fdt_nodes(hwaddr addr, int irq, unsigned int num,
+                              void *fdt, int intc);
+
 #endif
diff --git a/include/hw/virtio/virtio-mmio.h b/include/hw/virtio/virtio-mmio.h
new file mode 100644
index 0000000..d54ecc3
--- /dev/null
+++ b/include/hw/virtio/virtio-mmio.h
@@ -0,0 +1,38 @@ 
+/*
+ * Virtio-mmio public utilities
+ *
+ * Copyright (c) 2015 Samsung Electronics Co. Ltd.
+ *
+ * Author:
+ *  Pavel Fedin <p.fedin@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _QEMU_VIRTIO_MMIO_H
+#define _QEMU_VIRTIO_MMIO_H
+
+#include "hw/irq.h"
+
+#define VIRTIO_MMIO_SIZE 0x0200
+
+/**
+ * virtio_mmio_create:
+ * @addr: Starting physical address of virtio region
+ * @irqs: Array of IRQ objects, one per virtio device
+ * @num: Number of virtio devices to create
+ *
+ * Create a bunch of virtio-mmio devices
+ */
+void virtio_mmio_create(hwaddr addr, qemu_irq *irqs, unsigned int num);
+
+#endif