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

[Daniel P. Berrange]

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