On Fri, 2016-06-17 at 11:36 -0400, Laine Stump wrote:
> On 06/17/2016 08:43 AM, Andrea Bolognani wrote:
> > There has been some progress lately in enabling virtio-pci on
> > aarch64 guests; however, guest OS support is still spotty at best,
> > so most guests are going to be using virtio-mmio instead.
> >
> > Currently, mach-virt guests are closely modeled after q35 guests,
> > and that includes always adding a dmi-to-pci-bridge that's just
> > impossible to get rid of.
>
> Yeah. Sorry my simple patches from yesterday didn't get you as far as
> you needed to go.
That's quite all right, you did a bunch of work and I merely
walked the last mile :)
> > * other than the pcie-root. This is so that there will be hot-pluggable
> > - * PCI slots available
> > + * PCI slots available.
> > + *
> > + * We skip this step for aarch64 mach-virt guests, where we want to
> > + * be able to have a pure virtio-mmio topology
> > */
> > if (virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1) < 0 &&
> > + !qemuDomainMachineIsVirt(def) &&
>
> You're assuming that the only virt* machinetypes will be aarch64, which
> may be reasonable now, but not in the future (periodically someone from
> qemu will mention the idea of a "virt" machinetype for x86, which is
> legacy-free and accepts only virtio devices). Wouldn't a more specific
> comparison be better here (and in the other places in this patch)?
I'll reply to this one in the sub-thread Martin started.
> > !virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1,
> > VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE))
> > goto cleanup;
>
> Okay, so pcie-root is still always "there", which isn't a problem since
> the presence of pcie-root in the config doesn't actually change anything
> in the qemu commandline or resulting virtual machine (it's a default
> device in qemu that can't be removed).
Exactly, that's why it *has* to be in the domain XML. Since
it can't be removed or disabled, it should always be displayed
to the user.
> > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> > index ca3adc0..883264a 100644
> > --- a/src/qemu/qemu_domain_address.c
> > +++ b/src/qemu/qemu_domain_address.c
> > @@ -1483,9 +1483,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> > }
> >
> > /* Reserve 1 extra slot for a (potential) bridge only if buses
> > - * are not fully reserved yet
> > + * are not fully reserved yet.
> > + *
> > + * We don't reserve the extra slot for aarch64 mach-virt guests
> > + * either because we want to be able to have pure virtio-mmio
> > + * guests, and reserving this slot would force us to add at least
> > + * a dmi-to-pci-bridge to an otherwise PCI-free topology
> > */
> > if (!buses_reserved &&
> > + !qemuDomainMachineIsVirt(def) &&
> > virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> > goto cleanup;
>
> You could save some time by just putting the whole thing from "for (i =
> 0; i < addrs->nbuses; i++)" down to that "goto cleanup" inside an "if
> (!qemuDomainMachineIsVirt(def)) { ... }". (maybe replacing
> qemuDomainMachineIsVirt() with something more specific, as noted above).
I didn't change that because it would have added one level
of nesting to the code, and qemuDomainPCIBusFullyReserved()
should be fairly cheap anyway. We can always revisit it
later.
> As we've discussed in IRC, eventually there should be a way to disable
> this for *every* platform, not just aarch64/virt. Possibly an
> "openPCISlots" attribute somewhere in the domain that tells how many
> slots should be left open for hotplug (with default being 1 for
> x86/440fx, and up for discussion on other platforms).
Agreed. We're not quite there yet, unfortunately, so for now
this mach-virt specific fix will have to do :)
> > <target dev='sda' bus='scsi'/>
> > <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> > </disk>
> > + <controller type='pci' index='0' model='pcie-root'/>
>
> This surprised me for a minute, since you're now explicitly adding
> pcie-root even though it's still implicitly added for you (and, for
> example, qemuxml2argvtest completes successfully without it). But I
> guess it doesn't hurt anything to have it in the file, as it makes it
> obvious that the dmi-to-pci-bridge isn't just being added on top of nothing.
Yeah, that was the pretty much my reasoning :)
> (Hopefully all of this will soon be a thing of the past - for *all*
> arches/machinetypes if you put in a device with <address type='pci'/>
> (or a device that can only be connected via PCI), then all the necessary
> pci controllers should just magically appear, otherwise not).
I think we're all looking forward to that brighter day! :)
> ACK.
Thanks, I've pushed it.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list