On Mon, Aug 5, 2013 at 3:37 AM, Laine Stump <laine@laine.org> wrote:
On 08/04/2013 07:53 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> 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?
I think you're probably right and we want to keep this check in. Obviously we want libvirt to do the right thing for users and not give them a bunch of bits that they need to sort themselves so this check helps users along the right path and helps get a consistent environment for different QEMU binaries so keep it.
+ 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@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.
-- Doug Goldstein