[libvirt] [PATCH] qemu: use "ide" as device name for implicit SATA controller on Q35

[Laine Stump]

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