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

Re: [libvirt] [PATCH 4/5] qemu: set/validate slot/connection type when assigning slots for PCI devices



On 07/23/2013 11:31 AM, Daniel P. Berrange wrote:
> On Tue, Jul 23, 2013 at 10:44:54AM -0400, Laine Stump wrote:
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 059aa6a..64787b6 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1412,7 +1412,15 @@ cleanup:
>>  #define QEMU_PCI_ADDRESS_FUNCTION_LAST 7
>>  
>>  typedef struct {
>> -    /* Each bit in a slot represents one function on that slot */
>> +    virDomainControllerModelPCI model;
>> +    /* flags an min/max can be computed from model, but
>> +     * having them ready makes life easier.
>> +     */
>> +    qemuDomainPCIConnectFlags flags;
>> +    size_t minSlot, maxSlot; /* usually 0,0 or 1,31 */
>> +    /* Each bit in a slot represents one function on that slot. If the
>> +     * bit is set, that function is in use by a device.
>> +     */
>>      uint8_t slots[QEMU_PCI_ADDRESS_SLOT_LAST + 1];
>>  } qemuDomainPCIAddressBus;
>>  typedef qemuDomainPCIAddressBus *qemuDomainPCIAddressBusPtr;
>> @@ -1430,9 +1438,13 @@ struct _qemuDomainPCIAddressSet {
>>   * Check that the PCI address is valid for use
>>   * with the specified PCI address set.
>>   */
>> -static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED,
>> -                                   virDevicePCIAddressPtr addr)
>> +static bool
>> +qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED,
>> +                       virDevicePCIAddressPtr addr,
>> +                       qemuDomainPCIConnectFlags flags)
>>  {
>> +    qemuDomainPCIAddressBusPtr bus;
>> +
>>      if (addrs->nbuses == 0) {
>>          virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
>>          return false;
>> @@ -1448,32 +1460,81 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
>>                         addrs->nbuses - 1);
>>          return false;
>>      }
>> -    if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) {
>> +
>> +    bus = &addrs->buses[addr->bus];
>> +
>> +    /* assure that at least one of the requested connection types is
>> +     * provided by this bus
>> +     */
>> +    if (!(flags & bus->flags & QEMU_PCI_CONNECT_TYPES_MASK)) {
>>          virReportError(VIR_ERR_XML_ERROR,
>> -                       _("Invalid PCI address: function must be <= %u"),
>> -                       QEMU_PCI_ADDRESS_FUNCTION_LAST);
>> +                       _("Invalid PCI address: The PCI controller "
>> +                         "providing bus %04x doesn't support "
>> +                         "connections appropriate for the device "
>> +                         "(%x required vs. %x provided by bus)"),
>> +                       addr->bus, flags & QEMU_PCI_CONNECT_TYPES_MASK,
>> +                       bus->flags & QEMU_PCI_CONNECT_TYPES_MASK);
>>          return false;
>>      }
>> -    if (addr->slot > QEMU_PCI_ADDRESS_SLOT_LAST) {
>> +    /* make sure this bus allows hot-plug if the caller demands it */
>> +    if ((flags & QEMU_PCI_CONNECT_HOTPLUGGABLE) &&
>> +        !(bus->flags &  QEMU_PCI_CONNECT_HOTPLUGGABLE)) {
>>          virReportError(VIR_ERR_XML_ERROR,
>> -                       _("Invalid PCI address: slot must be <= %u"),
>> -                       QEMU_PCI_ADDRESS_SLOT_LAST);
>> +                       _("Invalid PCI address: hot-pluggable slot requested, "
>> +                         "but bus %04x doesn't support hot-plug"), addr->bus);
>>          return false;
>>      }
>> -    if (addr->slot == 0) {
>> -        if (addr->bus) {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("Slot 0 is unusable on PCI bridges"));
>> -        } else {
>> -            virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                           _("Slot 0 on bus 0 is reserved for the host bridge"));
>> -        }
>> +    /* some "buses" are really just a single port */
>> +    if (bus->minSlot && addr->slot < bus->minSlot) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       _("Invalid PCI address: slot must be >= %zu"),
>> +                       bus->minSlot);
>> +        return false;
>> +    }
>> +    if (addr->slot > bus->maxSlot) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       _("Invalid PCI address: slot must be <= %zu"),
>> +                       bus->maxSlot);
>> +        return false;
>> +    }
>> +    if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) {
>> +        virReportError(VIR_ERR_XML_ERROR,
>> +                       _("Invalid PCI address: function must be <= %u"),
>> +                       QEMU_PCI_ADDRESS_FUNCTION_LAST);
>
>> +
>> +static int
>> +qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus,
>> +                                virDomainControllerModelPCI model)
>> +{
>> +    switch (model) {
>> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>> +        bus->flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
>> +                      QEMU_PCI_CONNECT_TYPE_PCI);
>> +        bus->minSlot = 1;
>> +        bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST;
>> +        break;
>> +    default:
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Invalid PCI controller model %d"), model);
>> +        return -1;
>> +    }
>> +
>> +    bus->model = model;
>> +    return 0;
>> +}
>> +
>> +
>>  /* Ensure addr fits in the address set, by expanding it if needed.
>> + * This will only grow if the flags say that we need a normal
>> + * hot-pluggable PCI slot. If we need a different type of slot, it
>> + * will fail.
>> + *
>>   * Return value:
>>   * -1 = OOM
>>   *  0 = no action performed
>> @@ -1481,7 +1542,8 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
>>   */
>>  static int
>>  qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
>> -                            virDevicePCIAddressPtr addr)
>> +                            virDevicePCIAddressPtr addr,
>> +                            qemuDomainPCIConnectFlags flags)
>>  {
>>      int add;
>>      size_t i;
>> @@ -1490,16 +1552,33 @@ qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs,
>>      i = addrs->nbuses;
>>      if (add <= 0)
>>          return 0;
>> +
>> +    /* auto-grow only works when we're adding plain PCI devices */
>> +    if (!(flags & QEMU_PCI_CONNECT_TYPE_PCI)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Cannot automatically add a new PCI bus for a "
>> +                         "device requiring a slot other than standard PCI."));
>> +        return -1;
>> +    }
>> +
>>      if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, add) < 0)
>>          return -1;
>> -    /* reserve slot 0 on the new buses */
>> -    for (; i < addrs->nbuses; i++)
>> -        addrs->buses[i].slots[0] = 0xFF;
>> +
>> +    for (; i < addrs->nbuses; i++) {
>> +        /* Any time we auto-add a bus, we will want a multi-slot
>> +         * bus. Currently the only type of bus we will auto-add is a
>> +         * pci-bridge, which is hot-pluggable and provides standard
>> +         * PCI slots.
>> +         */
>> +        qemuDomainPCIAddressBusSetModel(&addrs->buses[i],
>> +                                        VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
>> +    }
>>      return add;
> In this old code we're setting slot 0 to 0xFF to reserve it,
> and qemuDomainPCIAddressBusSetModel does not do that. I
> think that's ok, since qemuPCIAddressValidate uses minSlot
> now, but wanted to check that this was correct

Yes, that's correct. I had originally left that code in (setting slot 0
to 0xFF) just to be paranoid, but later convinced myself I could remove
it immediately rather than leaving it around to confound someone else in
6 months.

>
>
> ACK if you can answer that q.
>
> Definitely could do with some test coverage in the XML -> ARGV convertor
> for complex cases with multiple bridges in the XML

Yes. I'm going to add a slightly more compact version of the "287 virtio
disk devices" test case from the BZ for pci-bridge.

Also I think we need more test cases where the original XML has no
guest-side PCI addresses specified, and we put an "xmlout" version in
qemuxml2xmloutdata with pci addresses filled in; that way we can verify
that the auto-allocation of slots doesn't regress.

Thanks for the review. I'll add some test cases and push later tonight.


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