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/>

From what I understand right now (thanks to what kraxel pointed out in

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? :-/)