[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 4/7] qemu: fix handling of default/implicit devices for q35



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]