1:40 p.m.
On Thu, May 19, 2016 at 11:59 PM, Laine Stump <laine(a)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.
This was mainly for validation. If a user gives the same function number
for more than one device, I wanted to refuse that. Also, the ordering of
the user
specified slot numbers can be different. User can give different slot
numbers also. But now you point it out, I realize we need the function zero
to be at the last in the device list anyway. So, we can just force user
specified function numbers to be in decreasing order if specified. Hope
that is the right way?
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).
Noted.
> 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.
I somehow couldn't get it working. May be my setup. I'll need some help
when I try next time on this machine.
> Signed-off-by: Shivaprasad G Bhat <sbhat(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>