Re: [libvirt] [PATCH v2] send default USB controller in xml to destination during migration

On Mon, May 02, 2016 at 02:21:41PM -0400, Laine Stump wrote: > On 05/02/2016 10:41 AM, Ján Tomko wrote: > > On Fri, Apr 29, 2016 at 01:09:26PM -0400, Cole Robinson wrote: > >> On 04/29/2016 10:01 AM, Shivaprasad G Bhat wrote: > >>> The default USB controller is not sent to destination as the older versions > >>> of libvirt(0.9.4 or earlier as I see in commit log of 409b5f54) didn't > >>> support them. For some archs where the support started much later can > >>> safely send the USB controllers without this worry. So, send the controller > >>> to destination for all archs except x86. Moreover this is not very applicable > >>> to x86 as the USB controller has model ich9_ehci1 on q35 and for pc-i440fx, > >>> there cant be any slots before USB as it is fixed on slot 1. > >>> > >>> The patch fixes a bug that, if the USB controller happens to occupy > >>> a slot after disks/interfaces and one of them is hot-unplugged, then > >>> the default USB controller added on destination takes the smallest slot > >>> number and that would lead to savestate mismatch and migration > >>> failure. Seen and verified on PPC64. > >>> > >>> Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > >>> --- > >>> src/qemu/qemu_domain.c | 6 +++++- > >>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > >>> index 6262bfe..963ff35 100644 > >>> --- a/src/qemu/qemu_domain.c > >>> +++ b/src/qemu/qemu_domain.c > >>> @@ -2599,7 +2599,11 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, > >>> usb = def->controllers[i]; > >>> } > >>> } > >>> - if (usb && usb->idx == 0 && usb->model == -1) { > >>> + /* The original purpose of the check was the migration compatibility > >>> + * with libvirt <= 0.9.4. Limitation doesn't apply to other archs > >>> + * and can cause problems on PPC64. > >>> + * > >>> + if (ARCH_IS_X86(def->os.arch) && usb && usb->idx == 0 && usb->model == -1) { > > As of commit fcbfd50: > > qemu: only check for PIIX3-specific device addrs on pc-* machinetypes > > we only check that the controller is on slot 1 for PIIX3 machines, > > so qemuDomainMachineIsI440FX(def) should also be one of the conditions. > > > > It is possible to specify a usb controller with model='-1' on q35 too, > > but I do not know if anyone actually uses it in the wild. > > > I have a vague memory of searching through for the possible ways that > model could be -1 and coming up with the answer "almost never". As for > Q35 specifically - support was added for PCIe controllers required by > Q35 in libvirt 1.1.2, so any configuration for a Q35 domain wouldn't > work if you tried to move it to a system with a libvirt old enough to > require removing the usb controller from the XML. > Yes, that's is why I suggested to just check for the machine type. For i440fx, we can safely leave it out because it was always at slot 1. For q35, it could be on any other slot and leaving it out does not give us any benefit. For isapc, we stopped adding the default USB in commit c66da9d22. > This removal was useful at the time it was added, but at this point, do > we really expect that anyone is going to be successful migrating a > running virtual machine from a system running libvirt 1.3.4 to a system > with libvirt that is 0.9.4 or less (or downgrading their libvirt by that > many revisions)? Is there a use for doing it at all? Probably not. It might not work at all. Should we break it on purpose? The oldest QEMU version we support is 0.12.0. 0.12.0-rc2 was released in Dec 2009. Libvirt 0.9.4 was released in August 2011, that's about 20 months later (and 4 years 9 months ago.) If we want to start breaking backwards compatibility, I would rather not break libvirt newer than the oldest supposed QEMU. Also, it would deserve a separate discussion thread with Daniel and Daniel. Jan