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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list