On Fri, Aug 2, 2013 at 11:55 AM, Laine Stump <laine@xxxxxxxxx> wrote:
> * The functions qemuDomainPCIAddressReserveAddr and
> qemuDomainPCIAddressReserveSlot were very similar (and should have
> been more similar) and were about to get more code added to them which
> would create even more duplicated code, so this patch gives
> qemuDomainPCIAddressReserveAddr a "reserveEntireSlot" arg, then
> replaces the body of qemuDomainPCIAddressReserveSlot with a call to
> qemuDomainPCIAddressReserveAddr.
>
> You will notice that addrs->lastaddr was previously set in
> qemuDomainPCIAddressReserveAddr (but *not* set in
> qemuDomainPCIAddressReserveSlot). For consistency and cleanliness of
> code, that bit was removed and put into the one caller of
> qemuDomainPCIAddressReserveAddr (there is a similar place where the
> caller of qemuDomainPCIAddressReserveSlot sets lastaddr). This does
> guarantee identical functionality to pre-patch code, but in practice
> isn't really critical, because lastaddr is just keeping track of where
> to start when looking for a free slot - if it isn't updated, we will
> just start looking on a slot that's already occupied, then skip up to
> one that isn't.
>
> * qemuCollectPCIAddress was essentially doing the same thing as
> qemuDomainPCIAddressReserveAddr, but with some extra special case
> checking at the beginning. The duplicate code has been replaced with
> a call to qemuDomainPCIAddressReserveAddr. This required adding a
> "fromConfig" boolean, which is only used to change the log error
> code from VIR_ERR_INTERNAL_ERROR (when the address was
> auto-generated by libvirt) to VIR_ERR_XML_ERROR (when the address is
> coming from the config); without this differentiation, it would be
> difficult to tell if an error was caused by something wrong in
> libvirt's auto-allocate code or just bad config.
>
> * the bit of code in qemuDomainPCIAddressValidate that checks the
> connect type flags is going to be used in a couple more places where
> we don't need to also check the slot limits (because we're generating
> the slot number ourselves), so that has been pulled out into a
> separate qemuDomainPCIAddressFlagsCompatible function.
> ---
> src/qemu/qemu_command.c | 232 ++++++++++++++++++++++++------------------------
> src/qemu/qemu_command.h | 4 +-
> 2 files changed, 119 insertions(+), 117 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4345456..abc973a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1434,9 +1434,53 @@ struct _qemuDomainPCIAddressSet {
> };
>
>
> -/*
> - * Check that the PCI address is valid for use
> - * with the specified PCI address set.
> +static bool
> +qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
> + qemuDomainPCIConnectFlags busFlags,
> + qemuDomainPCIConnectFlags devFlags,
> + bool reportError)
> +{
> + /* If this bus doesn't allow the type of connection (PCI
> + * vs. PCIe) required by the device, or if the device requires
> + * hot-plug and this bus doesn't have it, return false.
> + */
> + if (!(devFlags & busFlags & QEMU_PCI_CONNECT_TYPES_MASK)) {
> + if (reportError) {
> + if (devFlags & QEMU_PCI_CONNECT_TYPE_PCI) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("PCI bus %.4x:%.2x is not compatible with the "
> + "device. Device requires a standard PCI slot, "
> + "which is not provided by this bus"),
> + addr->domain, addr->bus);
So even though this patch has been ACK'd and committed. I really would
have liked to see some info about the device printed in the error
message because when you're defining a domain its really not clear
which device is the problem.
> + } else {
> + /* this should never happen. If it does, there is a
> + * bug in the code that sets the flag bits for devices.
> + */
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("The device information has no PCI "
> + "connection types listed"));
> + }
> + }
> + return false;
> + }
> + if ((devFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE) &&
> + !(busFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE)) {
> + if (reportError) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("PCI bus %.4x:%.2x is not compatible with the "
> + "device. Device requires hot-plug capability, "
> + "which is not provided by the bus"),
> + addr->domain, addr->bus);
Same as above, when you're defining the whole domain its really not
clear which device is the problem so provide some details on which one
it is.
> + }
> + return false;
> + }
> + return true;
> +}
> +
> +
> +/* Verify that the address is in bounds for the chosen bus, and
> + * that the bus is of the correct type for the device (via
> + * comparing the flags).
> */
> static bool
> qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,
> @@ -1466,24 +1510,9 @@ qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,
> /* 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: 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;
> - }
> - /* 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: hot-pluggable slot requested, "
> - "but bus %04x doesn't support hot-plug"), addr->bus);
> + if (!qemuDomainPCIAddressFlagsCompatible(addr, bus->flags, flags, true))
> return false;
> - }
> +
> /* some "buses" are really just a single port */
> if (bus->minSlot && addr->slot < bus->minSlot) {
> virReportError(VIR_ERR_XML_ERROR,
> @@ -1597,10 +1626,10 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> virDomainDeviceInfoPtr info,
> void *opaque)
> {
> + qemuDomainPCIAddressSetPtr addrs = opaque;
> int ret = -1;
> - char *str = NULL;
> virDevicePCIAddressPtr addr = &info->addr.pci;
> - qemuDomainPCIAddressSetPtr addrs = opaque;
> + bool entireSlot;
> /* FIXME: flags should be set according to the requirements of @device */
> qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
> QEMU_PCI_CONNECT_TYPE_PCI);
> @@ -1639,56 +1668,15 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> return 0;
> }
>
> - /* add an additional bus of the correct type if needed */
> - if (addrs->dryRun &&
> - qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)
> - return -1;
> -
> - /* verify that the address is in bounds for the chosen bus, and
> - * that the bus is of the correct type for the device (via
> - * comparing the flags).
> - */
> - if (!qemuDomainPCIAddressValidate(addrs, addr, flags))
> - return -1;
> -
> - if (!(str = qemuDomainPCIAddressAsString(addr)))
> - goto cleanup;
> + entireSlot = (addr->function == 0 &&
> + addr->multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON);
>
> - /* check if already in use */
> - if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) {
> - if (info->addr.pci.function != 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Attempted double use of PCI Address '%s' "
> - "(may need \"multifunction='on'\" for device on function 0)"),
> - str);
> - } else {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Attempted double use of PCI Address '%s'"), str);
> - }
> + if (qemuDomainPCIAddressReserveAddr(addrs, addr, flags,
> + entireSlot, true) < 0)
> goto cleanup;
> - }
>
> - /* mark as in use */
> - if ((info->addr.pci.function == 0) &&
> - (info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) {
> - /* a function 0 w/o multifunction=on must reserve the entire slot */
> - if (addrs->buses[addr->bus].slots[addr->slot]) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Attempted double use of PCI Address on slot '%s' "
> - "(need \"multifunction='off'\" for device "
> - "on function 0)"),
> - str);
> - goto cleanup;
> - }
> - addrs->buses[addr->bus].slots[addr->slot] = 0xFF;
> - VIR_DEBUG("Remembering PCI slot: %s (multifunction=off)", str);
> - } else {
> - VIR_DEBUG("Remembering PCI addr: %s", str);
> - addrs->buses[addr->bus].slots[addr->slot] |= 1 << addr->function;
> - }
> ret = 0;
> cleanup:
> - VIR_FREE(str);
> return ret;
> }
>
> @@ -1871,46 +1859,73 @@ static bool qemuDomainPCIAddressSlotInUse(qemuDomainPCIAddressSetPtr addrs,
> }
>
>
> +/*
> + * Reserve a slot (or just one function) for a device. If
> + * reserveEntireSlot is true, all functions for the slot are reserved,
> + * otherwise only one. If fromConfig is true, the address being
> + * requested came directly from the config and errors should be worded
> + * appropriately. If fromConfig is false, the address was
> + * automatically created by libvirt, so it is an internal error (not
> + * XML).
> + */
> int
> qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr,
> - qemuDomainPCIConnectFlags flags)
> + qemuDomainPCIConnectFlags flags,
> + bool reserveEntireSlot,
> + bool fromConfig)
> {
> - char *str;
> + int ret = -1;
> + char *str = NULL;
> qemuDomainPCIAddressBusPtr bus;
> -
> - if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)
> - return -1;
> + virErrorNumber errType = (fromConfig
> + ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
>
> if (!(str = qemuDomainPCIAddressAsString(addr)))
> - return -1;
> + goto cleanup;
>
> - VIR_DEBUG("Reserving PCI addr %s", str);
> + /* Add an extra bus if necessary */
> + if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)
> + goto cleanup;
> + /* Check that the requested bus exists, is the correct type, and we
> + * are asking for a valid slot
> + */
> + if (!qemuDomainPCIAddressValidate(addrs, addr, flags))
> + goto cleanup;
>
> bus = &addrs->buses[addr->bus];
> - if ((bus->minSlot && addr->slot < bus->minSlot) ||
> - addr->slot > bus->maxSlot) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unable to reserve PCI address %s. "
> - "Slot %02x can't be used on bus %04x, "
> - "only slots %02zx - %02zx are permitted on this bus."),
> - str, addr->slot, addr->bus, bus->minSlot, bus->maxSlot);
> - }
>
> - if (bus->slots[addr->slot] & (1 << addr->function)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unable to reserve PCI address %s already in use"), str);
> - VIR_FREE(str);
> - return -1;
> + if (reserveEntireSlot) {
> + if (bus->slots[addr->slot]) {
> + virReportError(errType,
> + _("Attempted double use of PCI slot %s "
> + "(may need \"multifunction='on'\" for "
> + "device on function 0)"), str);
> + goto cleanup;
> + }
> + bus->slots[addr->slot] = 0xFF; /* reserve all functions of slot */
> + VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", str);
> + } else {
> + if (bus->slots[addr->slot] & (1 << addr->function)) {
> + if (addr->function == 0) {
> + virReportError(errType,
> + _("Attempted double use of PCI Address %s"), str);
> + } else {
> + virReportError(errType,
> + _("Attempted double use of PCI Address %s "
> + "(may need \"multifunction='on'\" "
> + "for device on function 0)"), str);
> + }
> + goto cleanup;
> + }
> + bus->slots[addr->slot] |= (1 << addr->function);
> + VIR_DEBUG("Reserving PCI address %s", str);
> }
>
> + ret = 0;
> +cleanup:
> VIR_FREE(str);
> -
> - addrs->lastaddr = *addr;
> - addrs->lastaddr.function = 0;
> - addrs->lastaddr.multi = 0;
> - bus->slots[addr->slot] |= 1 << addr->function;
> - return 0;
> + return ret;
> }
>
>
> @@ -1919,26 +1934,7 @@ qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr,
> qemuDomainPCIConnectFlags flags)
> {
> - char *str;
> -
> - if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)
> - return -1;
> -
> - if (!(str = qemuDomainPCIAddressAsString(addr)))
> - return -1;
> -
> - VIR_DEBUG("Reserving PCI slot %s", str);
> -
> - if (addrs->buses[addr->bus].slots[addr->slot]) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unable to reserve PCI slot %s"), str);
> - VIR_FREE(str);
> - return -1;
> - }
> -
> - VIR_FREE(str);
> - addrs->buses[addr->bus].slots[addr->slot] = 0xFF;
> - return 0;
> + return qemuDomainPCIAddressReserveAddr(addrs, addr, flags, true, false);
> }
>
> int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
> @@ -2044,12 +2040,11 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
> VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> a.domain, a.bus, a.slot);
> }
> -
> }
> }
>
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("No more available PCI addresses"));
> + "%s", _("No more available PCI slots"));
> return -1;
>
> success:
> @@ -2409,9 +2404,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>
> addr.bus = tmp_addr.bus;
> addr.slot = tmp_addr.slot;
> +
> + addrs->lastaddr = addr;
> + addrs->lastaddr.function = 0;
> + addrs->lastaddr.multi = 0;
> }
> /* Finally we can reserve the slot+function */
> - if (qemuDomainPCIAddressReserveAddr(addrs, &addr, flags) < 0)
> + if (qemuDomainPCIAddressReserveAddr(addrs, &addr, flags,
> + false, false) < 0)
> goto error;
>
> def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index c9f1600..bf4953a 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -253,7 +253,9 @@ int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
> qemuDomainPCIConnectFlags flags);
> int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
> virDevicePCIAddressPtr addr,
> - qemuDomainPCIConnectFlags flags);
> + qemuDomainPCIConnectFlags flags,
> + bool reserveEntireSlot,
> + bool fromConfig);
> int qemuDomainPCIAddressReserveNextSlot(qemuDomainPCIAddressSetPtr addrs,
> virDomainDeviceInfoPtr dev,
> qemuDomainPCIConnectFlags flags);
> --
> 1.7.11.7
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Doug Goldstein
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list