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