On Thu, May 19, 2016 at 11:59 PM, Laine Stump <la...@laine.org> wrote:
> On 05/18/2016 05:32 PM, Shivaprasad G Bhat wrote: > >> This patch assigns multifunction pci addresses to devices in the devlist. >> The pciaddrs passed in the argument is not altered so that the actual >> call to >> reserve the address using virDomainPCIAddressEnsureAddr() passes. The >> function >> focuses on user given address validation and also the auto-assign of >> addresses. >> The address auto-assignment works well for PPC64 and x86-i440fx machines. >> > > Since you know that after hotplugging these devices into this slot, you > won't be able to add any new devices to it, I think it's unnecessary to > keep track of exactly which functions of the slot are occupied and which > aren't. Effectively, they *all* are. > > So instead of copy-pasting the huge chunk of code and allocating one > function at a time, how about just marking the entire slot in use at a > higher level rather than trying to mark individual functions? (unless > you're looking at these individual bits later for something *other* than > just deciding which ones to free. > Missed to answer to this point. I am using the function numbers later with the device_add. I need the function numbers to be passed along. So, arriving at it is important here. > > Note that you'll need to determine the CONNECT_TYPE at the higher level > too, and be sure to choose PCI_DEVICE or PCIE_DEVICE appropriately (and if > there is any attempt to mix the two, I would say you should refuse to > auto-assign an address (but allow it if they manually specify the address). > > > >> The q35 machines needs some level of logic here to get the correct next >> valid >> slot so that the hotplug go through fine. >> > > Can you explain that? There should be no difference. > > > > >> Signed-off-by: Shivaprasad G Bhat <sb...@linux.vnet.ibm.com> >> --- >> src/conf/domain_addr.c | 199 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> src/conf/domain_addr.h | 4 + >> src/libvirt_private.syms | 1 >> 3 files changed, 204 insertions(+) >> >> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c >> index 7ea9e4d..7c52f84 100644 >> --- a/src/conf/domain_addr.c >> +++ b/src/conf/domain_addr.c >> @@ -454,6 +454,205 @@ >> virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs, >> return virDomainPCIAddressReserveAddr(addrs, addr, flags, true, >> false); >> } >> +static int >> +virDomainPCIAddressGetNextFunctionAddr(uint8_t *slot, >> + virPCIDeviceAddressPtr addr) >> +{ >> + size_t i = 0; >> + >> + for (i = 0; i < 8; i++) { >> + if (!(*slot & 1 << i)) { >> + addr->function = i; >> + break; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int >> +virDomainPCIAddressAssignFunctionAddr(virDomainPCIAddressSetPtr addrs, >> + uint8_t *slot, >> + virPCIDeviceAddressPtr addr, >> + virDomainPCIConnectFlags flags, >> + bool fromConfig) >> +{ >> + int ret = -1; >> + char *addrStr = NULL; >> + virErrorNumber errType = (fromConfig >> + ? VIR_ERR_XML_ERROR : >> VIR_ERR_INTERNAL_ERROR); >> + >> + if (!(addrStr = virDomainPCIAddressAsString(addr))) >> + goto cleanup; >> + >> + /* Check that the requested bus exists, is the correct type, and we >> + * are asking for a valid slot and function >> + */ >> + if (!virDomainPCIAddressValidate(addrs, addr, addrStr, flags, >> fromConfig)) >> + goto cleanup; >> + >> + if (*slot & (1 << addr->function)) { >> + virReportError(errType, >> + _("Attempted double use of PCI Address %s"), >> + addrStr); >> + goto cleanup; >> + } >> + *slot |= (1 << addr->function); >> + if (addr->function == 0) >> + addr->multi = VIR_TRISTATE_SWITCH_ON; >> + VIR_DEBUG("Reserving PCI address %s", addrStr); >> + >> + ret = 0; >> + cleanup: >> + VIR_FREE(addrStr); >> + return ret; >> +} >> + >> + >> +static int >> +virDomainPCIAddressAssignSlotNextFunction(virDomainPCIAddressSetPtr >> addrs, >> + uint8_t *slot, >> + virDomainDeviceInfoPtr dev, >> + virDomainPCIConnectFlags flags) >> +{ >> + if (virDomainPCIAddressGetNextFunctionAddr(slot, &dev->addr.pci) < 0) >> + return -1; >> + >> + if (virDomainPCIAddressAssignFunctionAddr(addrs, slot, >> &dev->addr.pci, flags, false) < 0) >> + return -1; >> + >> + dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; >> + >> + return 0; >> +} >> + >> + >> +static int >> +virDomainPCIAddressAssignSlotAddr(virDomainPCIAddressSetPtr addrs, >> + uint8_t *slot, >> + virDomainDeviceInfoPtr dev) >> +{ >> + int ret = -1; >> + char *addrStr = NULL; >> + virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | >> + VIR_PCI_CONNECT_TYPE_PCI_DEVICE); >> + >> + if (!(addrStr = virDomainPCIAddressAsString(&dev->addr.pci))) >> + goto cleanup; >> + >> + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + if (((dev->addr.pci.function == 0) && (dev->addr.pci.multi == >> VIR_TRISTATE_SWITCH_ON)) || >> + dev->addr.pci.function != 0) { >> + >> + if (!virDomainPCIAddressValidate(addrs, &dev->addr.pci, >> + addrStr, flags, true)) >> + goto cleanup; >> + >> + ret = virDomainPCIAddressAssignFunctionAddr(addrs, slot, >> &dev->addr.pci, flags, true); >> + } else { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Not a multifunction device address %s"), >> addrStr); >> + } >> + } else { >> + ret = virDomainPCIAddressAssignSlotNextFunction(addrs, slot, >> dev, flags); >> + } >> + >> + cleanup: >> + VIR_FREE(addrStr); >> + return ret; >> +} >> + >> +int >> +virDomainPCIMultifunctionDeviceAddressAssign(virDomainPCIAddressSetPtr >> addrs, virDomainDeviceDefListPtr devlist) >> +{ >> + size_t i; >> + virPCIDeviceAddress addr; >> + virDomainPCIAddressBusPtr bus; >> + uint8_t slot; >> + virDomainDeviceInfoPtr info = NULL, privinfo = NULL; >> + virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPES_MASK; >> + >> + if (virDomainPCIAddressGetNextSlot(addrs, &addr, flags) < 0) >> + return -1; >> + >> + bus = &addrs->buses[addr.bus]; >> + slot = bus->slots[addr.slot]; >> + >> + for (i = 0; i < devlist->count; i++) { >> + virDomainDeviceDefPtr dev = devlist->devs[devlist->count - i - >> 1]; >> + switch ((virDomainDeviceType) dev->type) { >> + case VIR_DOMAIN_DEVICE_DISK: >> + info = &dev->data.disk->info; >> + break; >> + case VIR_DOMAIN_DEVICE_NET: >> + info = &dev->data.net->info; >> + break; >> + case VIR_DOMAIN_DEVICE_RNG: >> + info = &dev->data.rng->info; >> + break; >> + case VIR_DOMAIN_DEVICE_HOSTDEV: >> + info = dev->data.hostdev->info; >> + break; >> + case VIR_DOMAIN_DEVICE_CONTROLLER: >> + info = &dev->data.controller->info; >> + break; >> + case VIR_DOMAIN_DEVICE_CHR: >> + info = &dev->data.chr->info; >> + break; >> + case VIR_DOMAIN_DEVICE_FS: >> + case VIR_DOMAIN_DEVICE_INPUT: >> + case VIR_DOMAIN_DEVICE_SOUND: >> + case VIR_DOMAIN_DEVICE_VIDEO: >> + case VIR_DOMAIN_DEVICE_WATCHDOG: >> + case VIR_DOMAIN_DEVICE_HUB: >> + case VIR_DOMAIN_DEVICE_SMARTCARD: >> + case VIR_DOMAIN_DEVICE_MEMBALLOON: >> + case VIR_DOMAIN_DEVICE_NVRAM: >> + case VIR_DOMAIN_DEVICE_SHMEM: >> + case VIR_DOMAIN_DEVICE_LEASE: >> + case VIR_DOMAIN_DEVICE_REDIRDEV: >> + case VIR_DOMAIN_DEVICE_MEMORY: >> + case VIR_DOMAIN_DEVICE_NONE: >> + case VIR_DOMAIN_DEVICE_TPM: >> + case VIR_DOMAIN_DEVICE_PANIC: >> + case VIR_DOMAIN_DEVICE_GRAPHICS: >> + case VIR_DOMAIN_DEVICE_LAST: >> + break; >> + } >> + >> + if (i == 0 && info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + /* User has given an address in xml */ >> + bus = &addrs->buses[info->addr.pci.bus]; >> + slot = bus->slots[info->addr.pci.slot]; >> + } >> + >> + if (privinfo) { >> + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + if (privinfo->addr.pci.bus != info->addr.pci.bus || >> + privinfo->addr.pci.slot != info->addr.pci.slot) { >> + virReportError(VIR_ERR_XML_ERROR, "%s", >> + _("Multifunction PCI device " >> + "cant be split across different guest >> PCI slots")); >> + return -1; >> + } >> + } >> + } >> + >> + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { >> + info->addr.pci.bus = addr.bus; >> + info->addr.pci.slot = addr.slot; >> + info->addr.pci.domain = addr.domain; >> + } >> + >> + if (virDomainPCIAddressAssignSlotAddr(addrs, &slot, info) < 0) >> + return -1; >> + privinfo = info; >> + } >> + >> + return 0; >> +} >> + >> + >> int >> virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, >> virDomainDeviceInfoPtr dev) >> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h >> index f3eda89..9eb6b9d 100644 >> --- a/src/conf/domain_addr.h >> +++ b/src/conf/domain_addr.h >> @@ -157,6 +157,10 @@ int >> virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs, >> virDomainPCIConnectFlags flags) >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); >> +int >> +virDomainPCIMultifunctionDeviceAddressAssign(virDomainPCIAddressSetPtr >> addrs, >> + virDomainDeviceDefListPtr >> devlist); >> + >> struct _virDomainCCWAddressSet { >> virHashTablePtr defined; >> virDomainDeviceCCWAddress next; >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> index d6a60b5..d72dc63 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -109,6 +109,7 @@ virDomainPCIAddressSetGrow; >> virDomainPCIAddressSlotInUse; >> virDomainPCIAddressValidate; >> virDomainPCIControllerModelToConnectType; >> +virDomainPCIMultifunctionDeviceAddressAssign; >> virDomainPCIMultifunctionDeviceDefParseNode; >> virDomainVirtioSerialAddrAssign; >> virDomainVirtioSerialAddrAutoAssign; >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >> >> >
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list