On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine(a)laine.org&gt; wrote: > + if ((cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 && > + addr->function == 1) || > + (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && > + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || > + cont->model == -1) && addr->function == 2)) { > + /* Note the check for nbuses > 0 - if there are no PCI > + * buses, we skip this check. This is a quirk required for > + * some machinetypes such as s390, which pretend to have a > + * PCI bus for long enough to generate the "-usb" on the > + * commandline, but that don't really care if a PCI bus > + * actually exists. */ > + if (addrs->nbuses > 0 && > + !(addrs->buses[0].flags & QEMU_PCI_CONNECT_TYPE_PCI)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Bus 0 must be PCI for integrated PIIX3 " > + "USB or IDE controllers")); > + return -1; > + } else { > + return 0; > + } > + } Still very hacky but at least you improved it by providing a code comment as to why and a sensible error message if the user gets in this path. I'd still love to see us improve the situation so we didn't have to play games like this to get -usb to be emitted for different machine types.

> } > > entireSlot = (addr->function == 0 && > @@ -1695,8 +1728,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, > int nbuses = 0; > size_t i; > int rv; > - qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | > - QEMU_PCI_CONNECT_TYPE_PCI); > + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCI; So just for my edification, we previously were saying that pci-bridge devices were hotpluggable when in fact they are not? You could probably add a code comment above the default flags to that effect, but not strictly necessary.

> for (i = 0; i < def->ncontrollers; i++) { > if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { > @@ -1941,7 +1973,11 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, > virDomainDeviceInfoPtr dev) > { > int ret = 0; > - /* FIXME: flags should be set according to the particular device */ > + /* Flags should be set according to the particular device, > + * but only the caller knows the type of device. Currently this > + * function is only used for hot-plug, though, and hot-plug is > + * only supported for standard PCI devices, so we can safely use > + * the setting below */ > qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | > QEMU_PCI_CONNECT_TYPE_PCI); > > @@ -2005,7 +2041,13 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, > virDevicePCIAddressPtr next_addr, > qemuDomainPCIConnectFlags flags) > { > - virDevicePCIAddress a = addrs->lastaddr; > + virDevicePCIAddress a = { 0, 0, 0, 0, false }; Again, me being nitpicky but maybe a comment that says start the search from the top of the PCI addresses by settings this to all 0s and then below the comment says that this is a fast out when the flags match.

This patch adds in special handling for a few devices that need to be treated differently for q35 domains: usb - there is no implicit/default usb controller for the q35 machinetype. This is done because normally the default usb controller is added to a domain by just adding "-usb" to the qemu commandline, and it's assumed that this will add a single piix3 usb1 controller at slot 1 function 2. That's not what happens when the machinetype is q35, though. Instead, adding -usb to the commandline adds 3 usb (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather than having <controller type='usb' index='0'/> translate into 3 separate devices on the PCI bus, it's cleaner to not automatically add a default usb device; one can always be added explicitly if desired. Or we may decide that on q35 machines, 3 usb controllers will be automatically added when none is given. But for this initial commit, at least we aren't locking ourselves into something we later won't want. video - for pc machine types, the primary video device is always in slot 2, and that slot is reserved even when no video device has been specified. On q35, when you specify "-vga qxl" on the qemu commandline, the vga device is put in slot 1, not 2. Assuming that this was done for a reason, this patch always puts the primary video for q35 machines in slot 1, and reserves slot 1 even if there is no video.

sata - a q35 machine always has a sata controller implicitly added at slot 0x1F, function 2. There is no way to avoid this controller, so we always add it. Note that the xml2xml tests for the pcie-root and q35 cases were changed to use DO_TEST_DIFFERENT() so that we can check for the sata controller being automatically added. This is especially important because we can't check for it in the xml2argv output (it has no effect on that output since it's an implicit device).

ide - q35 has no ide controllers. isa and smbus controllers - these two are always present in a q35 (at slot 0x1F functions 0 and 3) but we have no way of modelling them in our config. We do need to reserve those functions so that the user doesn't attempt to put anything else there though. --- src/qemu/qemu_command.c | 150 ++++++++++++++++++++- src/qemu/qemu_domain.c | 7 + tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 4 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-q35.args | 3 +- tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-pcie-root.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 26 ++++ tests/qemuxml2xmltest.c | 2 +- 9 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13a68a5..a6d6819 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1686,6 +1686,18 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, break; } } + /* SATA controllers aren't hot-plugged, and can be put in either a + * PCI or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && + device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + + /* video cards aren't hot-plugged, and can be put in either a PCI + * or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_VIDEO) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; /* Ignore implicit controllers on slot 0:0:1.0: * implicit IDE controller on 0:0:1.1 (no qemu command line) @@ -2315,6 +2327,133 @@ error: } +static bool +qemuDomainMachineIsQ35(virDomainDefPtr def) +{ + return (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")); +} + + +static int +qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + qemuDomainPCIAddressSetPtr addrs) +{ + size_t i; + virDevicePCIAddress tmp_addr; + bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + virDevicePCIAddressPtr addrptr; + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; + + /* Verify that the first SATA controller is at 00:1F.2 */ + /* the q35 machine type *always* has a SATA controller at this address */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + def->controllers[i]->idx == 0) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 0x1F || + def->controllers[i]->info.addr.pci.function != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary SATA controller must have PCI address 0:0:1f.2")); + goto error; + } + } else { + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 0x1F; + def->controllers[i]->info.addr.pci.function = 2; + } + } + } + + /* Reserve slot 0x1F function 0 (ISA bridge, not in config model) + * and function 3 (SMBus, also not (yet) in config model). As with + * the SATA controller, these devices are always present in a q35 + * machine; there is no way to not have them. + */ + if (addrs->nbuses) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1F; + tmp_addr.function = 0; + tmp_addr.multi = 1; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + tmp_addr.function = 3; + tmp_addr.multi = 0; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + } + + if (def->nvideos > 0) { + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA + * at slot 1 for some reason. + */ + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 1; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &primaryVideo->info, + flags) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:1.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 1 || + primaryVideo->info.addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary video card must have PCI address 0:0:1.0")); + goto error; + } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + } + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { + goto error; + } + }

+ return 0; + +error: + return -1; +} + + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; } + if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + goto error; + } + /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, _("SATA is not supported with this " "QEMU binary")); goto error; + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { + /* first SATA controller on Q35 machines is implicit */ + continue; } else { char *devstr; @@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && + !qemuDomainMachineIsQ35(def) && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (usbcontroller == 0) + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) virCommandAddArg(cmd, "-usb"); for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5030cd..75be591 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addDefaultUSB = true; + bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false; @@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, STREQ(def->os.machine, "q35")) { addPCIeRoot = true; addDefaultUSB = false; + addImplicitSATA = true; break; } if (!STRPREFIX(def->os.machine, "pc-0.") && @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) return -1; + if (addImplicitSATA && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) + return -1; + if (addPCIRoot && virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 23db85c..cecef7b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml index 1aa5455..d7fb90c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -15,7 +15,6 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index ddff6f0..6c24407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ --usb \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index aba0f88..0068d27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,11 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + 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-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml index 25c77f1..f10e85b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> + <controller type='sata' index='0'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml new file mode 100644 index 0000000..2a86e61 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <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'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b4590a..5c6730d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -295,7 +295,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); - DO_TEST("q35"); + DO_TEST_DIFFERENT("q35"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 08/04/2013 07:53 PM, Doug Goldstein wrote: > On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine(a)laine.org&gt; wrote: >> This patch adds in special handling for a few devices that need to be >> treated differently for q35 domains: >> >> usb - there is no implicit/default usb controller for the q35 >> machinetype. This is done because normally the default usb controller >> is added to a domain by just adding "-usb" to the qemu commandline, >> and it's assumed that this will add a single piix3 usb1 controller at >> slot 1 function 2. That's not what happens when the machinetype is >> q35, though. Instead, adding -usb to the commandline adds 3 usb >> (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather >> than having >> >> <controller type='usb' index='0'/> >> >> translate into 3 separate devices on the PCI bus, it's cleaner to not >> automatically add a default usb device; one can always be added >> explicitly if desired. Or we may decide that on q35 machines, 3 usb >> controllers will be automatically added when none is given. But for >> this initial commit, at least we aren't locking ourselves into >> something we later won't want. >> >> video - for pc machine types, the primary video device is always in >> slot 2, and that slot is reserved even when no video device has been >> specified. On q35, when you specify "-vga qxl" on the qemu >> commandline, the vga device is put in slot 1, not 2. Assuming that >> this was done for a reason, this patch always puts the primary video >> for q35 machines in slot 1, and reserves slot 1 even if there is no >> video. > Might be worth updating the comments here to say that QEMU will assign > it in the first available slot, which with q35 is slot 1 and i440fx > (pc) is slot 2 (due to the always present IDE controller). I see you caught my question (and the later discussion) on qemu-devel :-) yeah, I think it would be reasonable to change this comment, now that I understand better what is happening. > >> sata - a q35 machine always has a sata controller implicitly added at >> slot 0x1F, function 2. There is no way to avoid this controller, so we >> always add it. Note that the xml2xml tests for the pcie-root and q35 >> cases were changed to use DO_TEST_DIFFERENT() so that we can check for >> the sata controller being automatically added. This is especially >> important because we can't check for it in the xml2argv output (it has >> no effect on that output since it's an implicit device). > The note about pcie-root switching to DO_TEST_DIFFERENT should go into > the earlier patch. Done. > >> ide - q35 has no ide controllers. >> >> isa and smbus controllers - these two are always present in a q35 (at >> slot 0x1F functions 0 and 3) but we have no way of modelling them in >> our config. We do need to reserve those functions so that the user >> doesn't attempt to put anything else there though. >> --- >> >> + if (def->nvideos > 0) { >> + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA >> + * at slot 1 for some reason. >> + */ >> + virDomainVideoDefPtr primaryVideo = def->videos[0]; >> + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; >> + primaryVideo->info.addr.pci.domain = 0; >> + primaryVideo->info.addr.pci.bus = 0; >> + primaryVideo->info.addr.pci.slot = 1; >> + primaryVideo->info.addr.pci.function = 0; >> + addrptr = &primaryVideo->info.addr.pci; >> + >> + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) >> + goto error; >> + >> + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { >> + if (qemuDeviceVideoUsable) { >> + virResetLastError(); >> + if (qemuDomainPCIAddressReserveNextSlot(addrs, >> + &primaryVideo->info, >> + flags) < 0) >> + goto error; >> + } else { >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> + _("PCI address 0:0:1.0 is in use, " >> + "QEMU needs it for primary video")); >> + goto error; >> + } >> + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { >> + goto error; >> + } >> + } else if (!qemuDeviceVideoUsable) { >> + if (primaryVideo->info.addr.pci.domain != 0 || >> + primaryVideo->info.addr.pci.bus != 0 || >> + primaryVideo->info.addr.pci.slot != 1 || >> + primaryVideo->info.addr.pci.function != 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> + _("Primary video card must have PCI address 0:0:1.0")); >> + goto error; >> + } >> + /* If TYPE==PCI, then qemuCollectPCIAddress() function >> + * has already reserved the address, so we must skip */ >> + } >> + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { >> + memset(&tmp_addr, 0, sizeof(tmp_addr)); >> + tmp_addr.slot = 1; >> + >> + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { >> + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" >> + " device will not be possible without manual" >> + " intervention"); >> + virResetLastError(); >> + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { >> + goto error; >> + } >> + } > Is this really necessary/correct? From the qemu-devel thread it really > seems like the VGA controller will just take the first available slot > but is it not possible for us to stick this somewhere other than slot > 1? I know with current libvirt and qemu we limit ourselves to -vga qxl > rather than -device qxl-vga due to a bug in QEMU but I think with QEMU > 1.6, that limitation goes away so that might free us up to move it > around. Unfortunately, I think that if we want to avoid surprises with the location of the video device then we have to do this. The problem is that not everybody has qemu 1.6+ (actually at this point *almost nobody* does). If we don't reserve "the first" slot (be it 1 or 2) for the video adapter when the domain is created, and continue to watch out for it as long as no video is added, another device would probably go in that place, and when the user finally did get around to adding a device, it would go in "some other" indeterminate place (well, we could figure it out, but it would be much more inconvenient that simply reserving slot 1 or 2 on every domain for video) I may be taking this too seriously though - does anyone else want to chime in one way or another?

> > >> + return 0; >> + >> +error: >> + return -1; >> +} >> + >> + >> /* >> * This assigns static PCI slots to all configured devices. >> * The ordering here is chosen to match the ordering used >> @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, >> goto error; >> } >> >> + if (qemuDomainMachineIsQ35(def) && >> + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { >> + goto error; >> + } >> + >> /* PCI controllers */ >> for (i = 0; i < def->ncontrollers; i++) { >> if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { >> @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, >> _("SATA is not supported with this " >> "QEMU binary")); >> goto error; >> + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { >> + /* first SATA controller on Q35 machines is implicit */ >> + continue; >> } else { >> char *devstr; >> >> @@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, >> } >> } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && >> cont->model == -1 && >> + !qemuDomainMachineIsQ35(def) && >> (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || >> def->os.arch == VIR_ARCH_PPC64)) { >> if (usblegacy) { >> @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, >> } >> } >> >> - if (usbcontroller == 0) >> + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) >> virCommandAddArg(cmd, "-usb"); >> >> for (i = 0; i < def->nhubs; i++) { >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index f5030cd..75be591 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, >> void *opaque ATTRIBUTE_UNUSED) >> { >> bool addDefaultUSB = true; >> + bool addImplicitSATA = false; >> bool addPCIRoot = false; >> bool addPCIeRoot = false; >> >> @@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, >> STREQ(def->os.machine, "q35")) { >> addPCIeRoot = true; >> addDefaultUSB = false; >> + addImplicitSATA = true; >> break; >> } >> if (!STRPREFIX(def->os.machine, "pc-0.") && >> @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, >> def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) >> return -1; >> >> + if (addImplicitSATA && >> + virDomainDefMaybeAddController( >> + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) >> + return -1; >> + >> if (addPCIRoot && >> virDomainDefMaybeAddController( >> def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args >> index 23db85c..cecef7b 100644 >> --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args >> @@ -1,5 +1,5 @@ >> LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ >> -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ >> -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ >> --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ >> --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb >> +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ >> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml >> index 1aa5455..d7fb90c 100644 >> --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml >> @@ -15,7 +15,6 @@ >> <devices> >> <emulator>/usr/libexec/qemu-kvm</emulator> >> <controller type='pci' index='0' model='pcie-root'/> >> - <controller type='usb' index='0'/> >> <memballoon model='none'/> >> </devices> >> </domain> >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args >> index ddff6f0..6c24407 100644 >> --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args >> @@ -1,7 +1,6 @@ >> LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ >> /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ >> -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ >> --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ >> +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ >> -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ >> --usb \ >> -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index aba0f88..0068d27 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c >> @@ -995,11 +995,13 @@ mymain(void) >> DO_TEST("pci-bridge-many-disks", >> QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); >> DO_TEST("pcie-root", >> + QEMU_CAPS_ICH9_AHCI, >> QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, >> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); >> DO_TEST("q35", >> QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, >> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, >> + 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-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml >> index 25c77f1..f10e85b 100644 >> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml >> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml >> @@ -15,7 +15,7 @@ >> <devices> >> <emulator>/usr/libexec/qemu-kvm</emulator> >> <controller type='pci' index='0' model='pcie-root'/> >> - <controller type='usb' index='0'/> >> + <controller type='sata' index='0'/> >> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> >> <controller type='pci' index='2' model='pci-bridge'/> >> <memballoon model='none'/> >> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml >> new file mode 100644 >> index 0000000..2a86e61 >> --- /dev/null >> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml >> @@ -0,0 +1,26 @@ >> +<domain type='qemu'> >> + <name>q35-test</name> >> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> >> + <memory unit='KiB'>2097152</memory> >> + <currentMemory unit='KiB'>2097152</currentMemory> >> + <vcpu placement='static' cpuset='0-1'>2</vcpu> >> + <os> >> + <type arch='x86_64' machine='q35'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/libexec/qemu-kvm</emulator> >> + <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'/> >> + <controller type='sata' index='0'/> >> + <video> >> + <model type='qxl' ram='65536' vram='18432' heads='1'/> >> + </video> >> + <memballoon model='none'/> >> + </devices> >> +</domain> >> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c >> index 8b4590a..5c6730d 100644 >> --- a/tests/qemuxml2xmltest.c >> +++ b/tests/qemuxml2xmltest.c >> @@ -295,7 +295,7 @@ mymain(void) >> DO_TEST_DIFFERENT("pci-autoadd-addr"); >> DO_TEST_DIFFERENT("pci-autoadd-idx"); >> DO_TEST_DIFFERENT("pcie-root"); >> - DO_TEST("q35"); >> + DO_TEST_DIFFERENT("q35"); >> >> DO_TEST("hostdev-scsi-lsi"); >> DO_TEST("hostdev-scsi-virtio-scsi"); >> -- >> 1.7.11.7 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list > I'm going to hold off on saying ACK here due to my question above, but > we might not even need a code change if my understanding is wrong. >

We had been setting the device alias in the devinceinfo for pci controllers to "pci%u", but then hardcoding "pci.%u" when creating the device address for other devices using that pci bus. This all worked just fine until we encountered the built-in "pcie.0" bus (the PCIe root complex) in Q35 machines. In order to create the correct commandline for this one case, this patch: 1) sets the alias for PCI controllers correctly, to "pci.%u" (or "pcie.%u" for the pcie-root controller) 2) eliminates the hardcoded "pci.%u" for pci controllers when generatuing device address strings, and instead uses the controller's alias. 3) plumbs a pointer to the virDomainDef all the way down to qemuBuildDeviceAddressStr. This was necessary in order to make the aliase of the controller *used by a device* available (previously qemuBuildDeviceAddressStr only had the deviceinfo of the device itself, *not* of the controller it was connecting to). This made for a larger than desired diff, but at least in the future we won't have to do it again, since all the information we could possibly ever need for future enhancements is in the virDomainDef. (right?) This should be done for *all* controllers, but for now we just do it in the case of PCI controllers, to reduce the likelyhood of regression. --- src/qemu/qemu_command.c | 164 ++++++++++++++------- src/qemu/qemu_command.h | 28 ++-- src/qemu/qemu_hotplug.c | 6 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 5 files changed, 136 insertions(+), 66 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a6d6819..3353c61 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -860,10 +860,18 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) { const char *prefix = virDomainControllerTypeToString(controller->type); - if (virAsprintf(&controller->info.alias, "%s%d", prefix, - controller->idx) < 0) - return -1; - return 0; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + /* only pcie-root uses a different naming convention + * ("pcie.0"), because it is hardcoded that way in qemu. All + * other buses use the consistent "pci.%u". + */ + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return virAsprintf(&controller->info.alias, "pcie.%d", controller->idx); + else + return virAsprintf(&controller->info.alias, "pci.%d", controller->idx); + } + + return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } static ssize_t @@ -2781,22 +2789,57 @@ qemuUsbId(virBufferPtr buf, int idx) static int qemuBuildDeviceAddressStr(virBufferPtr buf, + virDomainDefPtr domainDef, virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { + int ret = -1; + char *devStr = NULL; + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + const char *contAlias = NULL; + size_t i; + + if (!(devStr = qemuDomainPCIAddressAsString(&info->addr.pci))) + goto cleanup; + for (i = 0; i < domainDef->ncontrollers; i++) { + virDomainControllerDefPtr cont = domainDef->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->idx == info->addr.pci.bus) { + contAlias = cont->info.alias; + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device alias was not set for PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + break; + } + } + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only PCI device addresses with function=0 " "are supported with this QEMU binary")); - return -1; + goto cleanup; } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'multifunction=on' is not supported with " "this QEMU binary")); - return -1; + goto cleanup; } } @@ -2810,18 +2853,19 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, */ if (info->addr.pci.bus != 0) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virBufferAsprintf(buf, ",bus=pci.%u", info->addr.pci.bus); + virBufferAsprintf(buf, ",bus=%s", contAlias); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Multiple PCI buses are not supported " "with this QEMU binary")); - return -1; + goto cleanup; } } else { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) - virBufferAddLit(buf, ",bus=pci.0"); - else + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) { + virBufferAsprintf(buf, ",bus=%s", contAlias); + } else { virBufferAddLit(buf, ",bus=pci"); + } } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) virBufferAddLit(buf, ",multifunction=on"); @@ -2845,7 +2889,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, info->addr.ccw.devno); } - return 0; + ret = 0; +cleanup: + VIR_FREE(devStr); + return ret; } static int @@ -4179,13 +4226,13 @@ qemuBuildDriveDevStr(virDomainDefPtr def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } - if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage"); - if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; default: @@ -4309,7 +4356,8 @@ error: char * -qemuBuildFSDevStr(virDomainFSDefPtr fs, +qemuBuildFSDevStr(virDomainDefPtr def, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; @@ -4325,7 +4373,7 @@ qemuBuildFSDevStr(virDomainFSDefPtr fs, virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); - if (qemuBuildDeviceAddressStr(&opt, &fs->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error; if (virBufferError(&opt)) { @@ -4545,7 +4593,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, if (def->queues) virBufferAsprintf(&buf, ",num_queues=%u", def->queues); - if (qemuBuildDeviceAddressStr(&buf, &def->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, domainDef, &def->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4583,7 +4631,8 @@ qemuBuildNicStr(virDomainNetDefPtr net, char * -qemuBuildNicDevStr(virDomainNetDefPtr net, +qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps) @@ -4645,7 +4694,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, virBufferAsprintf(&buf, ",id=%s", net->info.alias); virBufferAsprintf(&buf, ",mac=%s", virMacAddrFormat(&net->mac, macaddr)); - if (qemuBuildDeviceAddressStr(&buf, &net->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0) goto error; @@ -4800,7 +4849,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, char * -qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +qemuBuildWatchdogDevStr(virDomainDefPtr def, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4813,7 +4863,7 @@ qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, } virBufferAsprintf(&buf, "%s,id=%s", model, dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4830,7 +4880,8 @@ error: char * -qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +qemuBuildMemballoonDevStr(virDomainDefPtr def, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4850,7 +4901,7 @@ qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, } virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4893,7 +4944,8 @@ error: } char * -qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +qemuBuildUSBInputDevStr(virDomainDefPtr def, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4902,7 +4954,7 @@ qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, dev->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ? "usb-mouse" : "usb-tablet", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4919,7 +4971,8 @@ error: char * -qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +qemuBuildSoundDevStr(virDomainDefPtr def, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4940,7 +4993,7 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound, model = "intel-hda"; virBufferAsprintf(&buf, "%s,id=%s", model, sound->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &sound->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &sound->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5000,7 +5053,8 @@ error: } static char * -qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, +qemuBuildDeviceVideoStr(virDomainDefPtr def, + virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps, bool primary) { @@ -5054,7 +5108,7 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); } - if (qemuBuildDeviceAddressStr(&buf, &video->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5094,7 +5148,9 @@ qemuOpenPCIConfig(virDomainHostdevDefPtr dev) } char * -qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, +qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + const char *configfd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5114,7 +5170,7 @@ qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, virBufferAsprintf(&buf, ",id=%s", dev->info->alias); if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); - if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0) goto error; @@ -5220,7 +5276,7 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",bootindex=%d", dev->info.bootIndex); } - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5236,7 +5292,8 @@ error: } char * -qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5259,7 +5316,7 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); - if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5276,7 +5333,8 @@ error: char * -qemuBuildHubDevStr(virDomainHubDefPtr dev, +qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5296,7 +5354,7 @@ qemuBuildHubDevStr(virDomainHubDefPtr dev, virBufferAddLit(&buf, "usb-hub"); virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5817,6 +5875,7 @@ cleanup: static int qemuBuildRNGDeviceArgs(virCommandPtr cmd, + virDomainDefPtr def, virDomainRNGDefPtr dev, virQEMUCapsPtr qemuCaps) { @@ -5846,7 +5905,7 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, virBufferAddLit(&buf, ",period=1000"); } - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto cleanup; virCommandAddArg(cmd, "-device"); @@ -7121,7 +7180,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); } else { @@ -7869,7 +7928,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildHubDevStr(hub, qemuCaps))) + if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8090,7 +8149,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr); virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildFSDevStr(fs, qemuCaps))) + if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8448,7 +8507,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { char *optstr; virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildUSBInputDevStr(input, qemuCaps))) + if (!(optstr = qemuBuildUSBInputDevStr(def, input, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8505,7 +8564,7 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0; i < def->nvideos; i++) { char *str; virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, !i))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, !i))) goto error; virCommandAddArg(cmd, str); @@ -8579,7 +8638,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, false))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, false))) goto error; virCommandAddArg(cmd, str); @@ -8643,7 +8702,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildSoundDevStr(sound, qemuCaps))) + if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) goto error; virCommandAddArg(cmd, str); @@ -8719,7 +8778,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device"); - optstr = qemuBuildWatchdogDevStr(watchdog, qemuCaps); + optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); if (!optstr) goto error; } else { @@ -8832,7 +8891,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps))) goto error; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -8880,7 +8939,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } virCommandAddArg(cmd, "-device"); - devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, qemuCaps); + devstr = qemuBuildPCIHostdevDevStr(def, hostdev, configfd_name, qemuCaps); VIR_FREE(configfd_name); if (!devstr) goto error; @@ -9009,7 +9068,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virCommandAddArg(cmd, "-device"); - optstr = qemuBuildMemballoonDevStr(def->memballoon, qemuCaps); + optstr = qemuBuildMemballoonDevStr(def, def->memballoon, qemuCaps); if (!optstr) goto error; virCommandAddArg(cmd, optstr); @@ -9025,7 +9084,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; /* add the device */ - if (qemuBuildRNGDeviceArgs(cmd, def->rng, qemuCaps) < 0) + if (qemuBuildRNGDeviceArgs(cmd, def, def->rng, qemuCaps) < 0) goto error; } @@ -9103,6 +9162,7 @@ error: */ static int qemuBuildSerialChrDeviceStr(char **deviceStr, + virDomainDefPtr def, virDomainChrDefPtr serial, virQEMUCapsPtr qemuCaps, virArch arch, @@ -9115,7 +9175,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s", serial->info.alias); - if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } else { @@ -9137,7 +9197,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, goto error; } - if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } @@ -9252,7 +9312,7 @@ qemuBuildChrDeviceStr(char **deviceStr, switch ((enum virDomainChrDeviceType) chr->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: - ret = qemuBuildSerialChrDeviceStr(deviceStr, chr, qemuCaps, + ret = qemuBuildSerialChrDeviceStr(deviceStr, vmdef, chr, qemuCaps, vmdef->os.arch, vmdef->os.machine); break; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e5111d2..5c5c025 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -98,7 +98,8 @@ char * qemuBuildNicStr(virDomainNetDefPtr net, int vlan); /* Current, best practice */ -char * qemuBuildNicDevStr(virDomainNetDefPtr net, +char * qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps); @@ -119,7 +120,8 @@ char * qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps); -char * qemuBuildFSDevStr(virDomainFSDefPtr fs, +char * qemuBuildFSDevStr(virDomainDefPtr domainDef, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); /* Current, best practice */ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, @@ -127,22 +129,27 @@ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, virQEMUCapsPtr qemuCaps, int *nusbcontroller); -char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +char * qemuBuildWatchdogDevStr(virDomainDefPtr domainDef, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +char * qemuBuildMemballoonDevStr(virDomainDefPtr domainDef, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +char * qemuBuildUSBInputDevStr(virDomainDefPtr domainDef, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +char * qemuBuildSoundDevStr(virDomainDefPtr domainDef, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps); /* Legacy, pre device support */ char * qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, const char *configfd, virQEMUCapsPtr qemuCaps); @@ -151,7 +158,8 @@ int qemuOpenPCIConfig(virDomainHostdevDefPtr dev); /* Legacy, pre device support */ char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, @@ -162,7 +170,9 @@ char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildHubDevStr(virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps); +char * qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, + virQEMUCapsPtr qemuCaps); char * qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 032de69..7a6946e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -859,7 +859,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nicstr = qemuBuildNicDevStr(net, vlan, 0, priv->qemuCaps))) + if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0, priv->qemuCaps))) goto try_remove; } else { if (!(nicstr = qemuBuildNicStr(net, NULL, vlan))) @@ -1057,7 +1057,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, goto error; } - if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, + if (!(devstr = qemuBuildPCIHostdevDevStr(vm->def, hostdev, configfd_name, priv->qemuCaps))) goto error; @@ -1302,7 +1302,7 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, priv->qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) goto cleanup; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index cecef7b..84428f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-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 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 6c24407..5ff4bc7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-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 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Current seabios doesn't recognize any disk device that is attached anywhere behind an i82801b11-bridge, so it can't boot from them. (It's possible/probable that seabios simply doesn't recognize that bridge at all, so *no* devices behind it are available to the bios). In order to allow booting from any disk no matter where it is attached, this patch flouts the hardware specs (which say that only PCIe devices can be connected directly to a PCIe bus such as the q35 machine's "pcie.0"), taking advantage of the fact that qemu also ignores this rule, and installs a stnadard pci-bridge device anywhere the config requests a "dmi-to-pci-bridge (which had been setup to use i82801b11-bridge). Because this isn't really the way things should be, the original code is left in place, and the new code is put inside an #ifdef indicating why it is there. --- src/qemu/qemu_command.c | 16 ++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3353c61..460144f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4571,7 +4571,23 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, _("dmi-to-pci-bridge index should be > 0")); goto error; } +#define SEABIOS_NO_I82801B11_SUPPORT +#if defined SEABIOS_NO_I82801B11_SUPPORT + /* since seabios currently doesn't recognize boot devices + * that are anywhere behind an i82801b11-bridge, we take + * advantage of the fact that qemu erroneously + * (generously) allows us to plug a pci-bridge into a PCIe + * slot, and create a pci-bridge to take the place of the + * i82801b11-bridge. When seabios is fixed, we will figure + * out the best way to determine whether or not the fix is + * present, and switch to the more proper device when + * appropriate. + */ + virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d", + def->idx, def->idx); +#else virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx); +#endif break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 84428f9..da970db 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-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 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 5ff4bc7..a0ec66e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-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 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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 460144f..cb25659 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4199,9 +4199,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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list