Bug 1176071

This is probably actually libvirt's automatic address assignment going wrong, rather than something virt-manager has done itself.

Actually on Q35 machines these are not IDE buses, but are SATA (aka ahci) ports.

There was a previous bug on this subject: Bug 1008903.

In the upstream review of the patch for that bug, there was a discussion of the interpretation of "unit" wrt to sata controllers. It turns out that sata controllers, unlike ide controllers, don't have anything called "units". Instead, each sata controller has 6 "ports", which are referred to as "buses" in qemu. When the sata controller was added in 2011, the existing unit attribute in the xml was mapped to the sata port. (upstream commit c1bc3d892 for those interested in details).

Here are a couple of mails from the thread. You can follow the links to get to the rest of the thread:

My response to a suggestion that my patch was incorrect in using "unit" to specify the "bus" for qemu (which is really the "port"):

  https://www.redhat.com/archives/libvir-list/2013-September/msg01198.html

Gerd's explanation of the difference between real hardware and how (and why) it is modeled in qemu:

  https://www.redhat.com/archives/libvir-list/2013-September/msg01200.html

Dan's acknowledgement that libvirt was probably doing the right thing in this respect:

  https://www.redhat.com/archives/libvir-list/2013-September/msg01203.html
  https://www.redhat.com/archives/libvir-list/2013-September/msg01204.html

So it sounds like the problem is that the commandline generator really does believe this is an ide controller - it should be perfectly find to have a non-0 unit # in the XML. (this is further confused by the fact that the builtin sata controller in the Q35 machinetype is actually given the name "ide" by qemu; apparently this is because a lot of the IDE controller code was re-used for the sata controller).

(Just so it isn't misinterpreted - the previous bug was in related code, but was a different bug; it's just that the discussion of unit vs. bus vs. port came up during the discussion of that bug.)

I've just looked into this and found the following:

1) If you add a cdrom to a Q35 domain as:

   <disk type='file' device='cdrom'>
      <driver name='qemu' type='raw'/>
      <source file='blah.iso'/>
      <target dev='sdg' bus='sata'/>
      <readonly/>
    </disk>

then the disk will be auto-assigned an address like this:

    <address type='drive' controller='c' bus='0' target='0' unit='u'/>

"u" will vary from 0 to 5, then controller will be incremented by 1.

Assuming that controller='0' and unit='4', the commandline that is generated in this case looks like:

-drive file=blah.iso,if=none,media=cdrom,id=drive-sata0-0-4,\
       readonly=on,format=raw
-device ide-cd,bus=ide.4,drive=drive-sata0-0-4,id=sata0-0-4

The bus arg. references the controller named "ide" not because it is an IDE controller, but because in its wisdom/attempt to re-use existing code, qemu chose to hardcode the id/name/alias of the Q35 machine's builtin (and non-removable) SATA controller as "ide", just like the i440fx machine has a builtin *IDE* controller named "ide". Makes sense, right?

Note that, as you can learn from the discussion pointed to in Comment 2, qemu believes that each SATA controller has 6 "buses", while real SATA controller don't have *any* buses or units, they have "ports". libvirt's driver uses its "unit" to indicate which port a disk is attached to, and when it knows that it's hooking a disk to a SATA controller, it puts this in the position where qemu expects the "bus" to be (e.g. "bus=ide.4").

However, if libvirt is told that the disk is attaching to an IDE controller (of which there are none by default on a Q35 machine, and which I've just learned that libvirt will not tell qemu to add even if you specify it in the config!), then libvirt will assume that it needs to put its "unit" value in the place that qemu expects a unit number (e.g. "bus=ide.0,unit=4").

So there are multiple levels to this problem:

1) virt-manager doesn't set the disk type to SATA by default when the machinetype is Q35, so disks (including CDROMs) are added to the config  with 'bus="ide"'

2) virt-manager doesn't even allow both "CDROM" and "SATA" to be set at the same time, so not only is the default selection wrong, it's impossible to select the correct settings.

3) libvirt's address assignment code doesn't understand that a Q35 machine has no builtin IDE controller - if you tell it a disk is connected to a SATA controller then it will do the right thing, but if you tell it that a disk is connected to an IDE controller, it will add a a <controller type='ide'/> (which is correct behavior) but then believe that the controller "just exists", because that's the way it works for i440FX.

4) libvirt has no way of adding an IDE controller to the qemu commandline - it's correct to not add anything for the first IDE controller on an i440FX machine, but if somebody needs more than 4 disks, then we should be adding a piix3-ide or piix4-ide device and assigning it an alias. Likewise, if *any* disk device with bus='ide' is added to a Q35 machine, we need to add an IDE controller.

I'm going to clone this bug back to virt-manager for problems 1 and 2, and keep this one assigned to libvirt for 3 and 4.

Setting assignee to laine since he set this NEW->ASSIGNED

I just pushed patches upstream to properly log an error when we try to create a domain with an IDE controller if one isn't supported for that domain. That will prevent the reported situation from occurring (instead, an error will be logged; to resolve the error switch the domain to connect the driver to a sata bus instead of ide).

I decided that since IDE is very old an slow, and up until now nobody has actually asked for support for additional IDE controllers, it would just cause support headaches with no real gain to add support, so I'm not going to do that bit (if someone else wants to they should feel free to submit patches, but it seems like a losing deal when there are so many better options available).

Here is a minimum list of commits that would need to be backported to get this into F21. My suggestion is that, since it's not really adding any functionality, but just properly reporting an error, it isn't worth the potential destabilization to backport to F21 (or maybe even F22). Any other opinions?


commit da558e72c4999d21d279ff2ab15f0b2446436c46
Author: Laine Stump <laine>
Date:   Wed Apr 29 12:23:02 2015 -0400

    qemu: use qemuDomainMachineIsI440FX() in appropriate place
    
commit e27c5c8fcb290abf89beebeadc541b81977ad839
Author: Laine Stump <laine>
Date:   Thu Apr 30 12:51:51 2015 -0400

    qemu: eliminate duplicated code in qemuBuildDriveDevStr()
    
 commit a3dfaf1272f57453c7c4f3924c2ce60dbd6de4bb
Author: Laine Stump <laine>
Date:   Wed Apr 29 15:37:20 2015 -0400

    conf: utility to return alias of a controller based on type/index
    
commit 75cd7d9b05492dff42fc5519ed04b56af7ac4409
Author: Laine Stump <laine>
Date:   Mon May 11 20:51:52 2015 -0400

    qemu: fix exceptions in qemuAssignDeviceControllerAlias
    
commit 0260506c65d51637bfd8ef6cdf2829d34fb05902
Author: Laine Stump <laine>
Date:   Thu Apr 30 13:19:10 2015 -0400

    qemu: use controller alias when constructing device/controller args
    
commit 548ba430289bb47beaeafa6296370b613708af82
Author: Laine Stump <laine>
Date:   Tue May 5 13:09:42 2015 -0400

    qemu: remove test for allowing ide controller in s390, rename usb tests
    
 commit b8f345b486c3e6d0c59cff7d8c4883d158d410cb
Author: Laine Stump <laine>
Date:   Thu May 14 11:10:22 2015 -0400

    qemu: clean up qemuBuildCommandline loop that builds controller args
    
commit eadd757ccee0e72156a5345e6c25477aa057a9f1
Author: Laine Stump <laine>
Date:   Thu Apr 30 16:59:04 2015 -0400

    qemu: log error when domain has an unsupported IDE controller

(In reply to Laine Stump from comment #6)

> Here is a minimum list of commits that would need to be backported to get
> this into F21. My suggestion is that, since it's not really adding any
> functionality, but just properly reporting an error, it isn't worth the
> potential destabilization to backport to F21 (or maybe even F22). Any other
> opinions?

Sounds reasonable to me, especially since f21+ virt-manager will use AHCI by default and hopefully avoid this issue. So just closing