Libguestfs - Oct 2016 - [PATCH] aarch64: Enable virtio-pci, replacing virtio-mmio.

This patch causes aarch64 to use virtio-pci instead of virtio-mmio.
Virtio-pci is considerably faster than virtio-mmio, it's more like how
other architectures work, and it supports hotplugging (although it's
not likely we'd use the latter feature).

I'm not necessarily suggesting that we apply this.  Laine (CC'd) has
some further patches to libvirt lined up which AIUI would make it
simpler to do this.

I have given this a little bit of testing and it works fine for simple
operations on aarch64.  I'm currently running the in-depth libguestfs
test suite.

Rich.

Thanks: Laine Stump, Andrea Bolognani, Marcel Apfelbaum.
---
 src/guestfs-internal.h |  6 +++---
 src/launch-libvirt.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index d437b9a..428da7f 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -137,17 +137,17 @@
 /* Differences in device names on ARM (virtio-mmio) vs normal
  * hardware with PCI.
  */
-#if !defined(__arm__) && !defined(__aarch64__)
+#if !defined(__arm__)
 #define VIRTIO_BLK "virtio-blk-pci"
 #define VIRTIO_SCSI "virtio-scsi-pci"
 #define VIRTIO_SERIAL "virtio-serial-pci"
 #define VIRTIO_NET "virtio-net-pci"
-#else /* ARM */
+#else /* ARMv7 */
 #define VIRTIO_BLK "virtio-blk-device"
 #define VIRTIO_SCSI "virtio-scsi-device"
 #define VIRTIO_SERIAL "virtio-serial-device"
 #define VIRTIO_NET "virtio-net-device"
-#endif /* ARM */
+#endif /* ARMv7 */
 
 /* Machine types. */
 #ifdef __arm__
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index d8479dc..1ac5604 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -950,6 +950,13 @@ static int construct_libvirt_xml_disk_source_hosts
(guestfs_h *g, xmlTextWriterP
 static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const
struct backend_libvirt_data *data, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_appliance (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
 
+static int construct_libvirt_xml_virtio_pci_address (guestfs_h *g,
xmlTextWriterPtr xo, int slot);
+/* Don't use slot 1, since can be used by video. */
+#define VIRTIO_PCI_SLOT_RNG     2
+#define VIRTIO_PCI_SLOT_SCSI    3
+#define VIRTIO_PCI_SLOT_SERIAL  4
+#define VIRTIO_PCI_SLOT_NETWORK 5
+
 /* These macros make it easier to write XML, but they also make a lot
  * of assumptions:
  *
@@ -1337,6 +1344,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
           attribute ("model", "random");
           string ("/dev/urandom");
         } end_element ();
+        if (construct_libvirt_xml_virtio_pci_address (g, xo,
+                                                      VIRTIO_PCI_SLOT_RNG) ==
-1)
+          return -1;
       } end_element ();
     }
 
@@ -1345,6 +1355,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
       attribute ("type", "scsi");
       attribute ("index", "0");
       attribute ("model", "virtio-scsi");
+      if (construct_libvirt_xml_virtio_pci_address (g, xo,
+                                                    VIRTIO_PCI_SLOT_SCSI) ==
-1)
+        return -1;
     } end_element ();
 
     /* Disks. */
@@ -1382,6 +1395,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
         attribute ("type", "virtio");
         attribute ("name", "org.libguestfs.channel.0");
       } end_element ();
+      if (construct_libvirt_xml_virtio_pci_address (g, xo,
+                                                    VIRTIO_PCI_SLOT_SERIAL) ==
-1)
+        return -1;
     } end_element ();
 
     /* Connect to libvirt bridge (see: RHBZ#1148012). */
@@ -1394,6 +1410,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
         start_element ("model") {
           attribute ("type", "virtio");
         } end_element ();
+        if (construct_libvirt_xml_virtio_pci_address (g, xo,
+                                                      VIRTIO_PCI_SLOT_NETWORK)
== -1)
+          return -1;
       } end_element ();
     }
 
@@ -1986,6 +2005,28 @@ find_secret (guestfs_h *g,
   return 0;
 }
 
+/**
+ * On aarch64 only, to force libvirt to use virtio-pci instead of
+ * virtio-mmio, we assign every virtio device to a unique function
+ * within the (implicitly created) pcie-root bus.  Every virtio device
+ * must have a unique slot number.
+ */
+static int
+construct_libvirt_xml_virtio_pci_address (guestfs_h *g,
+                                          xmlTextWriterPtr xo,
+                                          int slot)
+{
+#if defined(__aarch64__)
+  start_element ("address") {
+    attribute ("type", "pci");
+    attribute ("bus", "0");
+    attribute_format ("slot", "%d", slot);
+  } end_element ();
+#endif
+
+  return 0;
+}
+
 static int
 is_blk (const char *path)
 {
-- 
2.9.3

On 10/10/2016 11:47 AM, Richard W.M. Jones wrote:
> This patch causes aarch64 to use virtio-pci instead of virtio-mmio.
> Virtio-pci is considerably faster than virtio-mmio, it's more like how
> other architectures work, and it supports hotplugging (although it's
> not likely we'd use the latter feature).
>
> I'm not necessarily suggesting that we apply this.  Laine (CC'd)
has
> some further patches to libvirt lined up which AIUI would make it
> simpler to do this.
Since you're placing the devices directly on pcie-root, my libvirt 
patches won't make it any easier. (If, on the other hand, you were 
willing to have each device on its own pcie-root-port, then my patches 
would mean that you wouldn't have to do anything special at all). So I 
don't see a problem with pushing this. (It's possible that in the future
we might further simplify by making it possible to specify
"bus='0'"
without needing to specify exactly which slot, but that's very iffy, and 
not likely to happen soon even if it does)


BTW, you can use *exactly* the same device config for a Q35 machine.

On 10/10/2016 11:47 AM, Richard W.M. Jones wrote:
> Thanks: Laine Stump, Andrea Bolognani, Marcel Apfelbaum.
> ---
>   src/guestfs-internal.h |  6 +++---
>   src/launch-libvirt.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
> index d437b9a..428da7f 100644
> --- a/src/guestfs-internal.h
> +++ b/src/guestfs-internal.h
> @@ -137,17 +137,17 @@
>   /* Differences in device names on ARM (virtio-mmio) vs normal
>    * hardware with PCI.
>    */
> -#if !defined(__arm__) && !defined(__aarch64__)
> +#if !defined(__arm__)
>   #define VIRTIO_BLK "virtio-blk-pci"
>   #define VIRTIO_SCSI "virtio-scsi-pci"
>   #define VIRTIO_SERIAL "virtio-serial-pci"
>   #define VIRTIO_NET "virtio-net-pci"
> -#else /* ARM */
> +#else /* ARMv7 */
>   #define VIRTIO_BLK "virtio-blk-device"
>   #define VIRTIO_SCSI "virtio-scsi-device"
>   #define VIRTIO_SERIAL "virtio-serial-device"
>   #define VIRTIO_NET "virtio-net-device"
> -#endif /* ARM */
> +#endif /* ARMv7 */
>   
>   /* Machine types. */
>   #ifdef __arm__
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index d8479dc..1ac5604 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> @@ -950,6 +950,13 @@ static int construct_libvirt_xml_disk_source_hosts
(guestfs_h *g, xmlTextWriterP
>   static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g,
const struct backend_libvirt_data *data, xmlTextWriterPtr xo);
>   static int construct_libvirt_xml_appliance (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
>   
> +static int construct_libvirt_xml_virtio_pci_address (guestfs_h *g,
xmlTextWriterPtr xo, int slot);
> +/* Don't use slot 1, since can be used by video. */
> +#define VIRTIO_PCI_SLOT_RNG     2
> +#define VIRTIO_PCI_SLOT_SCSI    3
> +#define VIRTIO_PCI_SLOT_SERIAL  4
> +#define VIRTIO_PCI_SLOT_NETWORK 5
> +
>   /* These macros make it easier to write XML, but they also make a lot
>    * of assumptions:
>    *
> @@ -1337,6 +1344,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
>             attribute ("model", "random");
>             string ("/dev/urandom");
>           } end_element ();
> +        if (construct_libvirt_xml_virtio_pci_address (g, xo,
> +                                                      VIRTIO_PCI_SLOT_RNG)
== -1)
> +          return -1;
>         } end_element ();
>       }
>   
> @@ -1345,6 +1355,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
>         attribute ("type", "scsi");
>         attribute ("index", "0");
>         attribute ("model", "virtio-scsi");
> +      if (construct_libvirt_xml_virtio_pci_address (g, xo,
> +                                                    VIRTIO_PCI_SLOT_SCSI)
== -1)
> +        return -1;
>       } end_element ();
>   
>       /* Disks. */
> @@ -1382,6 +1395,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
>           attribute ("type", "virtio");
>           attribute ("name",
"org.libguestfs.channel.0");
>         } end_element ();
> +      if (construct_libvirt_xml_virtio_pci_address (g, xo,
> +                                                   
VIRTIO_PCI_SLOT_SERIAL) == -1)
> +        return -1;
>       } end_element ();
>   
>       /* Connect to libvirt bridge (see: RHBZ#1148012). */
> @@ -1394,6 +1410,9 @@ construct_libvirt_xml_devices (guestfs_h *g,
>           start_element ("model") {
>             attribute ("type", "virtio");
>           } end_element ();
> +        if (construct_libvirt_xml_virtio_pci_address (g, xo,
> +                                                     
VIRTIO_PCI_SLOT_NETWORK) == -1)
> +          return -1;
>         } end_element ();
>       }
>   
> @@ -1986,6 +2005,28 @@ find_secret (guestfs_h *g,
>     return 0;
>   }
>   
> +/**
> + * On aarch64 only, to force libvirt to use virtio-pci instead of
> + * virtio-mmio, we assign every virtio device to a unique function
s/function/slot/
> + * within the (implicitly created) pcie-root bus.  Every virtio device
> + * must have a unique slot number.

Not exactly true. It would also be possible to have multiple devices on 
the same slot, as long as each was on a different function within that 
slot (the reason it didn't work when you tried to assign them to 
different functions on the same slot was that you were using slot 0 of 
pcie-root, which is reserved. If you had used slot='1' that would have 
worked to.

So anyway, if you wanted to be pendantic, you could say that each device 
must have a unique address, and that you make the addresses unique by 
giving each a different slot.

If I knew the libguestfs code at all, I would ACK this. Since I don't, 
I'll give an ACK to the resulting config, for whatever that's worth :-)

> + */
> +static int
> +construct_libvirt_xml_virtio_pci_address (guestfs_h *g,
> +                                          xmlTextWriterPtr xo,
> +                                          int slot)
> +{
> +#if defined(__aarch64__)
> +  start_element ("address") {
> +    attribute ("type", "pci");
> +    attribute ("bus", "0");
> +    attribute_format ("slot", "%d", slot);
> +  } end_element ();
> +#endif
> +
> +  return 0;
> +}
> +
>   static int
>   is_blk (const char *path)
>   {