On Mon, Aug 5, 2013 at 3:37 AM, Laine Stump <laine@xxxxxxxxx> wrote:
> On 08/04/2013 07:53 PM, Doug Goldstein wrote:
>> On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@xxxxxxxxx> 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@xxxxxxxxxx
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list