[libvirt] [PATCH 8/9] qemu: domain: Reflect USB controller model in guest XML

[Andrea Bolognani]

On Mon, 2016-08-08 at 18:30 +0200, Ján Tomko wrote:
> > I think we still want to make a last-ditch effort to provide
> > some sort, any sort, of USB controller. Or rather, that we
> > have to. Honestly, I'd rather just drop the -usb handling
> > altogether and expect users not to mix modern-day libvirt
> > with QEMU versions from 5+ years ago.
> 
> It seems QEMU_CAPS_PIIX3_USB_UHCI was introduced by this QEMU commit:
> 
> commit 556cd09885bec3f69ba78228fe4e46dc1dad145b
> Author:     Markus Armbruster <armbru at redhat.com>
> AuthorDate: 2009-12-09 17:07:53 +0100
> Commit:     Anthony Liguori <aliguori at us.ibm.com>
> CommitDate: 2009-12-12 07:59:38 -0600
> 
>     qdev: Replace device names containing whitespace
> 
>     Device names with whitespace require quoting in the shell and in the
>     monitor.  Some of the offenders are also overly long.  Some have a
>     more convenient alias, some don't.
> 
>     The place for verbose device names is DeviceInfo member desc.  The
>     name should be short & sweet.
> 
>     Signed-off-by: Markus Armbruster <armbru at redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori at us.ibm.com>
> 
> contains: v0.13.0-rc0~2041
> 
> While I think we could safely drop support for that for x86_64,
> I would prefer if it would be accompanied with an exact version that is
> dropped, just like we did with 0.12.0. That way we could possibly drop
> more code.

0.12.0 was chosen as our cut-off version because it aligns
with currently-supported enterprise Linux releases, eg.
RHEL/CentOS 6, Ubuntu 12.04 and SLES 11. I don't think we
should drop newer versions while the OS vendors keep
supporting them. So we'll be stuck with 0.12.0 for a while :)

> But this is also executed for non-x86 architectures.
> We either do or do not add the <controller>, but emit -usb anyway.
> This could be broken by patch 9/9.
> 
> > > Of course, this breaks migration to pre-0.9.x libvirt which did not
> > > support USB controllers. I am okay with that, but you might want to get
> > > a second opinion on that.
> > 
> > How far back can you reasonably expect to migrate a guest?
> 
> I would expect us not to break it if we can avoid it.
> 
> Since we now drop the controller only for model =-1 and i440fx machine,
> changing the condition to also accept the i440fx model would allow us
> to migrate from newest libvirt to ancient libvirt not supporting USB
> controllers. When migrating back, we would auto-add the correct
> model since it happens at parse time and ABIStabilityCheck would be
> happy.
> 
> Please extend that condition to include PIIX3 model. If we ever want
> to drop it, we can just delete the whole thing from qemuDomainDefFormatBuf.

Okay, that would work since we make sure that piix3-usb-uhci
is always assigned a PCI address of 00:01.2 and -usb will use
that address.

> > How
> > would you even migrate a guest that uses USB controllers to a
> > version of libvirt that doesn't support USB controllers?
> 
> We were autoadding -usb to the command line even if libvirt did not
> support <controller type='usb'>. So passing it in XML unconditionally
> fails when being parsed on the destination, but if it's dropped, the
> destination libvirt just assumes it's there and emits -usb. (might even
> be -device .., I did not look back that far).
> 
> See:
> commit 409b5f549530e7b3a33f4505f2cad2e26896107c
>     qemu: Emit compatible XML when migrating a domain
> contains: v0.9.12-rc2~22
> 
> So, ACK to this patch if you either:
> * extend qemuDomainDefFormatBuf to also drop PIIX3 from migratable XML

I see. I will do that.

> * or get someone else to say: "Let's break this!"
> 
> For 9/9 I think we should not error out on controller model -1 just yet,
> since we do not pick it on XML parsing in all cases.

I think you're referring to this chunk:

@@ -2396,10 +2395,9 @@ qemuBuildUSBControllerDevStr(const virDomainDef *domainDef,
     model = def->model;
 
     if (model == -1) {
-        if ARCH_IS_PPC64(domainDef->os.arch)
-            model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
-        else
-            model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       "%s", _("no model provided for USB controller"));
+        return -1;
     }

Note that the previous code didn't really add anything but
complexity: immediately afterwards, the QEMU capabilities
are checked to see whether pci-ohci or piix3-usb-uhci are
available - but if they are, then we'd already have set the
model accordingly in qemuDomainDeviceDefPostParse().

I'll push both a fixed 8/9 and 9/9 in a while.

-- 
Andrea Bolognani / Red Hat / Virtualization