On Fri, Sep 20, 2013 at 10:15:30AM -0400, Laine Stump wrote: > On 09/20/2013 09:33 AM, Daniel P. Berrange wrote: > > On Fri, Sep 20, 2013 at 06:25:10AM -0400, Laine Stump wrote: > >> This resolves https://bugzilla.redhat.com/show_bug.cgi?id=1008903 > >> > >> The Q35 machinetype has an implicit SATA controller at 00:1F.2 which > >> isn't given the "expected" id of ahci0 by qemu when it's created. The > >> original suggested solution to this problem was to not specify any > >> controller for the disks that use the default controller and just > >> specify "unit=n" instead; qemu should then use the first ide or sata > >> controller for the disk. > >> > >> Unfortunately, this "solution" is ignorant of the fact that in the > >> case of SATA disks, the "unit" attribute in the disk XML is actually > >> *not* being used for the unit, but is instead used to specify the > >> "bus" number; each SATA controller has 6 buses, and each bus only > >> allows a single unit. This makes it nonsensical to specify unit='n' > >> where n is anything other than 0. It also means that the only way to > >> connect more than a single device to the implicit SATA controller is > >> to explicitly give the bus names, which happen to be "ide.$n", where > >> $n can be replaced by the disk's "unit" number. > >> --- > >> src/qemu/qemu_command.c | 15 > >> ++++++--------- > >> tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args | 2 +- > >> tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- > >> 3 files changed, 8 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > >> index 4628dac..e6239c9 100644 > >> --- a/src/qemu/qemu_command.c > >> +++ b/src/qemu/qemu_command.c > >> @@ -4383,18 +4383,15 @@ qemuBuildDriveDevStr(virDomainDefPtr def, > >> if (qemuDomainMachineIsQ35(def) && > >> disk->info.addr.drive.controller == 0) { > >> /* Q35 machines have an implicit ahci (sata) controller at > >> - * 00:1F.2 which has no "id" associated with it. For this > >> - * reason, we can't refer to it as "ahci0". Instead, we > >> - * don't give an id, which qemu interprets as "use the > >> - * first ahci controller". We then need to specify the > >> - * unit with "unit=%d" rather than adding it onto the bus > >> - * arg. > >> + * 00:1F.2 which for some reason is hardcoded with the id > >> + * "ide" instead of the seemingly more reasonable "ahci0" > >> + * or "sata0". > >> */ > >> - virBufferAsprintf(&opt, ",unit=%d", > >> disk->info.addr.drive.unit); > >> + virBufferAsprintf(&opt, ",bus=ide.%d", > >> disk->info.addr.drive.unit); > > This isn't right - you're now using the 'unit' number to refer to a bus. > > > > If the built-in controller only supports 1 unit and 6 buses, then the > > XML should be using one of > > > > <address type=disk controller=0 bus=0 unit=0/> > > <address type=disk controller=0 bus=1 unit=0/> > > <address type=disk controller=0 bus=2 unit=0/> > > <address type=disk controller=0 bus=3 unit=0/> > > <address type=disk controller=0 bus=4 unit=0/> > > <address type=disk controller=0 bus=5 unit=0/> > > Right. We were just discussing this on IRC. > > >From what I understand right now (thanks to what kraxel pointed out in > the comments of Bug 1008903, and confirmed with some simple > experiments), it's not just the builtin controller that only supports 1 > unit and 6 buses - the way we use bus and unit for SATA controllers has > been messed up since the SATA controller support was first added to > libvirt in 2011. So this patch is perpetuating an existing mistake. > > Here is what I understand as of right now; someone with better knowledge > please correct me if I'm wrong (in particular, I want to make sure that > 1 and 2 are true for *every* SATA controller, not just the one > integrated in the Q35 chipset): > > 1) Every SATA controller has 6 buses, and each bus allows a single > target (or "unit"). > > 2) The way to specify a particular bus for any qemu SATA controller is > "$idname.$busnumber". So for example, if we've given a controller the > name "ahci3" when we created it (that's what we would name a controller > with index='3'), then the bus names recognized by qemu would be: > > ahci3.0 > ahci3.1 > ... > ahci3.5 > > 3) When libvirt assigns a SATA address to a disk it always assigns > bus='0', and unit='(0..5)' > > 4) When libvirt generates the commandline, it always gives qemu the > "bus" arg as "ahci$controller.$unit", ignoring the bus attribute in the > config, and not specifying a "unit=n" in the qemu commandline (which is > good, because if unit is non-0, you get an error). > > So every address that libvirt has assigned to a SATA drive between 2011 > and now has been wrong (except for the ones with both bus=0 and unit=0). > > Fortunately, we can fix this in new configs and still retain forward > compatibility of pre-existing configs. What we need to do is: > > 1) change the address auto-assign code to now assign bus = 0.5 and > always force unit to 0 > > 2) modify the commandline generator code to use > "ahci$controller.$bus-or-unit-whichever-is-non-0" > > 3) if both are non-zero, when defining a domain or generating the > commandline, that is an error. > > This will leave one problem - what to do if someone migrates a guest > with the new non-0 unit# to a host that's still running old libvirt that > ignores unit#. I don't see how to get around this problem without having > knowledge of the source and destination libvirt versions when formatting > the XML to send across, but that is something we've avoided like the > plague. So what do we do for new->old migration? > > (or are we just stuck with the current state of unit being interpreted > as bus for sata addresses? :-/)
Actually based on Gerd's followup, I don't think we need todo anything now. Your first patch in this thread fixed the only problem we had. Since SATA has no concept of a bus, we only need use the controller and unit attributes of the address, and reject any bus != 0. What was causing me confusion is that QEMU's terminology for bus isn't quite the same as ours, nor the same as real hardwares'. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
