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

[Ján Tomko]

On Thu, Aug 04, 2016 at 04:44:34PM +0200, Andrea Bolognani wrote:
>On Thu, 2016-08-04 at 14:32 +0200, Ján Tomko wrote:
>> > +        } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
>> > +                   cont->model == -1) {
>> > +            /* Pick a suitable default model for the USB controller if none
>> > +             * has been selected by the user.
>> > +             *
>> > +             * We rely on device availability instead of setting the model
>> > +             * unconditionally because, for some machine types, there's a
>> > +             * chance we will get away with using the legacy USB controller
>> > +             * when the relevant device is not available.
>> > +             *
>> > +             * See qemuBuildControllerDevCommandLine() */
>> > +            if (ARCH_IS_PPC64(def->os.arch)) {
>> > +                /* Default USB controller for ppc64 is pci-ohci */
>> > +                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_OHCI))
>> > +                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI;
>> 
>> <thinking_out_loud>
>> 
>> So, if I understand correctly, if we left the model for PPC64 at -1:
>> [A] before 8156493 Fix USB model defaults for ppc64 [v1.3.1-rc1~47]
>>   we pass -usb on the command line, drop the controller from migration
>>   XML and possibly re-add it on the destination
>> [B] after that commit
>>   we pass -device pci-usb-ohci, lose that information in migration
>> [C] after 192a53e0 send default USB controller in xml [v1.3.5-rc1~459]
>>   We use -device pci-usb-ohci, passing the address to the
>>   destination
>> 
>> Migration between [A] and anything else is AFAIK broken since the
>> address used by -usb never matches the one we pick for the -device,
>> https://bugzilla.redhat.com/show_bug.cgi?id=1357468
>> 
>> Migration between [C] and [B] should just work as long as hotplugging
>> did not change the order of the devices and the controller auto-added on
>> the destination gets the same address as the one that was removed.
>
>You mean between [B] and [C]? When migrating between [C] and
>[B], the destination will receive the <controller> element,
>including the PCI address, so it should have no problem just
>using it.

Yes, [C] -> [B] should always work(TM).

>
>> </thinking_out_loud>
>> 
>> Is it possible to create a -device syntax that will match the -usb
>> command line generated by [A] (e.g. by specifying a 0000:00.00 PCI
>> addr)?
>
>Yes, something like
>
>  -device pci-ohci,id=usb,bus=pci.0,addr=0x0
>
>will work for QEMU, but we have no way of representing that in
>the guest configuration:
>
>  <address type='pci'
>           domain='0x0000' bus='0x00'
>           slot='0x00' function='0x0'/>
>
>will result in libvirt assigning a new PCI address for the
>device in recent libvirt versions, and an outright error in
>older libvirt versions, including [A].
>
>Assuming that we made it possible to specify such an address,
>it would be impossible for the target of the migration to make
>out whether the migration is coming from [A] or [B] - one of
>the two would be broken anyway.
>

Okay, seems [B] has a higher chance of working.

>> This change should fix migration from [B] and [C] and back:
>> * model=-1 coming from those hosts will get adjusted to OHCI on parsing
>> * we should not format model=-1 when migrating back to those hosts
>
>Migration between [B] and [C] should already work in both
>directions AFAICT. Can you give me an example where it
>wouldn't?
>

The use case described by the [C] commit: giving the USB controller an address
after another device, then unplugging that device.

[B] on destination will not get the controller address and assign the
earliest free slot, which does not match.

>> > +            } else {
>> > +                /* Default USB controller for anything else is piix3-uhci */
>> > +                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
>> > +                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
>> 
>> For non-PPC64, migration was not broken before.
>> 
>> Ancient QEMUs not supporting QEMU_CAPS_PIIX3_USB_UHCI will still get
>> model=-1 (or not, I doubt anyone will run new libvirt with them).
>> Should we set it unconditionally for i440fx machines?
>> Everything else gets the model changed to PIIX3-UHCI.
>
>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.

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.

> 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
* 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.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160808/9a8f8bc1/attachment-0001.sig>