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 hostsMigration 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@xxxxxxxxxx> AuthorDate: 2009-12-09 17:07:53 +0100 Commit: Anthony Liguori <aliguori@xxxxxxxxxx> 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@xxxxxxxxxx> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list