[libvirt] [PATCH] qemu: use "ide" as device name for implicit SATA controller on Q35
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); } else { /* All other ahci controllers have been created by - * libvirt, so they *do* have an id, and we can identify - * them that way. + * libvirt, and we gave them the id "ahci${n}" where ${n} + * is the controller number. So we identify them that way. */ virBufferAsprintf(&opt, ",bus=ahci%d.%d", disk->info.addr.drive.controller, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args index e10ccb1..175c0dc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args @@ -5,5 +5,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ --device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 4461eba..6bd5f4c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -4,5 +4,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ --device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.8.3.1
On 09/20/2013 12:25 PM, 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(-)
ACK
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/> 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 :|
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/>
From what I understand right now (thanks to what kraxel pointed out in
Right. We were just discussing this on IRC. 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? :-/)
Hi,
1) Every SATA controller has 6 buses, and each bus allows a single target (or "unit").
That is how it is modeled in qemu, largely due to reusing the ide code. On real hardware you have a sata controller with a number of ports (happens to be 6 for the one emulated by qemu, but that may be different on other controllers). You can plug a single device into each port. There is not really a concept of a sata bus.
(or are we just stuck with the current state of unit being interpreted as bus for sata addresses? :-/)
I'd say continuing using 'unit' is fine. When changing the way the address is specified in xml I'd rather go for something like this: <address type=sata controller=0 port=2 /> cheers, Gerd
On Fri, Sep 20, 2013 at 04:35:12PM +0200, Gerd Hoffmann wrote:
Hi,
1) Every SATA controller has 6 buses, and each bus allows a single target (or "unit").
That is how it is modeled in qemu, largely due to reusing the ide code.
On real hardware you have a sata controller with a number of ports (happens to be 6 for the one emulated by qemu, but that may be different on other controllers). You can plug a single device into each port. There is not really a concept of a sata bus.
Ok, so in libvirt terminology then, using 'unit' to map to the 'port' is what we want to be doing, and ignore the 'bus' attribute for sata drives.
(or are we just stuck with the current state of unit being interpreted as bus for sata addresses? :-/)
I'd say continuing using 'unit' is fine. When changing the way the address is specified in xml I'd rather go for something like this:
<address type=sata controller=0 port=2 />
We already use type=drive for addressing sata drives (and indeed any other drives attached to disk controllers), so we won't be inventing a new address type. 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 :|
On 09/20/2013 08:15 AM, Laine Stump wrote:
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?
We use the same hacks we do for other migration issues - if the _MIGRATABLE flag is on during dumpxml, we output the old style (stick it in the unit slot, since that's what older libvirt expected). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 :|
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Gerd Hoffmann -
Ján Tomko -
Laine Stump