[libvirt] [PATCHv3 1/2] qemu: enable using implicit sata controller in q35 machines

[Doug Goldstein]

On Mon, Aug 5, 2013 at 8:13 PM, Laine Stump <laine at laine.org> wrote:
> 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 on the commandline, 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.
> ---
>  src/qemu/qemu_command.c                         | 23 ++++++++++++++++++++---
>  tests/qemuxml2argvdata/qemuxml2argv-q35.args    |  2 ++
>  tests/qemuxml2argvdata/qemuxml2argv-q35.xml     |  5 +++++
>  tests/qemuxml2argvtest.c                        |  2 +-
>  tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml |  5 +++++
>  5 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 22ffa79..3d670f9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4211,9 +4211,26 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
>              virBufferAddLit(&opt, "ide-drive");
>          }
>
> -        virBufferAsprintf(&opt, ",bus=ahci%d.%d",
> -                          disk->info.addr.drive.controller,
> -                          disk->info.addr.drive.unit);
> +        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.
> +             */
> +            virBufferAsprintf(&opt, ",unit=%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.
> +             */
> +            virBufferAsprintf(&opt, ",bus=ahci%d.%d",
> +                              disk->info.addr.drive.controller,
> +                              disk->info.addr.drive.unit);
> +        }
>          break;
>      case VIR_DOMAIN_DISK_BUS_VIRTIO:
>          if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args
> index a0ec66e..6b30b4d 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args
> @@ -3,4 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
>  -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \
>  -device pci-bridge,chassis_nr=1,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 \
>  -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> index 3541b14..edaf6cb 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> @@ -14,6 +14,11 @@
>    <on_crash>destroy</on_crash>
>    <devices>
>      <emulator>/usr/libexec/qemu-kvm</emulator>
> +    <disk type='block' device='disk'>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='sda' bus='sata'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
>      <controller type='pci' index='0' model='pcie-root'/>
>      <controller type='pci' index='1' model='dmi-to-pci-bridge'/>
>      <controller type='pci' index='2' model='pci-bridge'/>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 0068d27..679124e 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1001,7 +1001,7 @@ mymain(void)
>      DO_TEST("q35",
>              QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
>              QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> -            QEMU_CAPS_ICH9_AHCI,
> +            QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI,
>              QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>              QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml
> index 2a86e61..96f8eaf 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml
> @@ -14,6 +14,11 @@
>    <on_crash>destroy</on_crash>
>    <devices>
>      <emulator>/usr/libexec/qemu-kvm</emulator>
> +    <disk type='block' device='disk'>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='sda' bus='sata'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
>      <controller type='pci' index='0' model='pcie-root'/>
>      <controller type='pci' index='1' model='dmi-to-pci-bridge'/>
>      <controller type='pci' index='2' model='pci-bridge'/>
> --
> 1.7.11.7
>

After discussing this online, Laine informed me that its a stable
characteristic of QEMU to treat "no id" as "first X" so this will
always be stable. The names I had found when digging in the code
aren't actually documented so they aren't guaranteed to stay that way.
So long story short....

ACK.

-- 
Doug Goldstein