On Mon, Aug 5, 2013 at 1:33 AM, Laine Stump <laine@xxxxxxxxx> wrote:
> On 08/04/2013 05:01 PM, Doug Goldstein wrote:
>> On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@xxxxxxxxx> 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.
>
>
> Yes. Totally agree.
>
>
>
>>> }
>>>
>>> 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.
>
>
> You can hot-plug a device into a pci-bridge, but a pci-bridge itself
> cannot be hot-plugged into the system. (Maybe a better way to describe
> it is that pci-bridge slots can accept hot-plugs, but the pci-bridge
> device cannot be hot-plugged. No, that's not any better. Let's see...)
No that makes sense.
>
>
>>
>>> 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.
>
> Is this better?
>
>
> /* default to starting the search for a free slot from
> * 0000:00:00.0
> */
> virDevicePCIAddress a = { 0, 0, 0, 0, false };
>
> /* except if this search is for the exact same type of device as
> * last time, continue the search from the previous match
> */
> if (flags == addrs->lastFlags)
> a = addrs->lastaddr;
>
> I squashed that in.
>
>
ACK this patch. Thanks for the updates.
--
Doug Goldstein
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list