[libvirt] [PATCH] qemu: Don't special case mdevs when assigning PCI addresses
We can treat mdevs the same as all other PCI hostdevs and figure out whether they are PCI Express or legacy PCI by checking the size of their config space. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain_address.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2106b34..3277d18 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -645,9 +645,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return pcieFlags; } - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) - return pcieFlags; - if (!(pciDev = virPCIDeviceNew(hostAddr->domain, hostAddr->bus, hostAddr->slot, -- 2.7.4
On 05/26/2017 06:51 AM, Andrea Bolognani wrote:
We can treat mdevs the same as all other PCI hostdevs and figure out whether they are PCI Express or legacy PCI by checking the size of their config space.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain_address.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 2106b34..3277d18 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -645,9 +645,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return pcieFlags; }
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) - return pcieFlags; - if (!(pciDev = virPCIDeviceNew(hostAddr->domain, hostAddr->bus, hostAddr->slot,
This can't work. From the host's point of view, there is no PCI device whose config space can be read, and from the code's point of view, hostAddr->domain|bus|slot are invalid (they are in hostdev->source.subsys.u.pci.addr, which is only valid when (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI). Is there maybe something standard in the mdev's sysfs entry that could be used to determine PCI vs PCIe? I think back when we were discussing the implementation of this, Alex had said there wasn't.
On Thu, 2017-06-01 at 21:24 -0400, Laine Stump wrote:
@@ -645,9 +645,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return pcieFlags; } - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) - return pcieFlags; - if (!(pciDev = virPCIDeviceNew(hostAddr->domain, hostAddr->bus, hostAddr->slot, This can't work. From the host's point of view, there is no PCI device whose config space can be read, and from the code's point of view, hostAddr->domain|bus|slot are invalid (they are in hostdev->source.subsys.u.pci.addr, which is only valid when (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI).
Surely you meant to use '==' here?
Is there maybe something standard in the mdev's sysfs entry that could be used to determine PCI vs PCIe? I think back when we were discussing the implementation of this, Alex had said there wasn't.
If that's the case, then there should be a comment explaining why mdevs are treated this way. It's not at all obvious to a reader who's not familiar with them :) -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 02, 2017 at 08:20:12AM +0200, Andrea Bolognani wrote:
On Thu, 2017-06-01 at 21:24 -0400, Laine Stump wrote:
@@ -645,9 +645,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return pcieFlags; }
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) - return pcieFlags; - if (!(pciDev = virPCIDeviceNew(hostAddr->domain, hostAddr->bus, hostAddr->slot,
This can't work. From the host's point of view, there is no PCI device whose config space can be read, and from the code's point of view, hostAddr->domain|bus|slot are invalid (they are in hostdev->source.subsys.u.pci.addr, which is only valid when (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI).
In fact it does work, because we return pcieFlags on error which might only be a problem if the backing device itself would be a legacy PCI.
Surely you meant to use '==' here?
Is there maybe something standard in the mdev's sysfs entry that could be used to determine PCI vs PCIe? I think back when we were discussing the implementation of this, Alex had said there wasn't.
No, there's not without looking at the parent PCI device. IIRC Alex said we would have to go to the parent device's sysfs, read the config space and determine it from there.
If that's the case, then there should be a comment explaining why mdevs are treated this way. It's not at all obvious to a reader who's not familiar with them :)
You're absolutely right, I have to admit, I don't remember the exact reason why I added that check, whether it was just to follow a private discussion about this or I actually checked the log for errors and figured this myself. Feel free to send another patch adding the comment. Anyhow, thank you Laine for providing feedback, since when Andrea described the issue to me, it made sense and looked to me like a feasible thing to do the whole time and I couldn't come up with an argument why it wasn't OK, so, appreciated. Erik
-- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Erik Skultety -
Laine Stump