[libvirt] [PATCH v2 1/2] util: virhostdev: disallow assigning a pci-bridge to a guest
Non-endpoint devices like pci-bridges cannot be passedthrough to guests. Prevent such attempts. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virhostdev.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 0673afb..b23fe1f 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -532,6 +532,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == VIR_PCI_STUB_DRIVER_VFIO); struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVFIO }; + int hdrType = -1; + + if (virPCIGetHeaderType(pci, &hdrType) < 0) + goto cleanup; + + if (hdrType != VIR_PCI_HEADER_ENDPOINT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI bridge devices " + "cannot be assigned to guests")); + goto cleanup; + } if (!usesVFIO && !virPCIDeviceIsAssignable(pci, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID,
It is destructive to attempt reset on a pci-bridge, the host can crash. The bridges won't contain any guest data and neither they can be passed through using vfio/stub. So, no point in allowing a reset on them. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 0601f49..ae72587 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -933,6 +933,17 @@ virPCIDeviceReset(virPCIDevicePtr dev, char *drvName = NULL; int ret = -1; int fd = -1; + int hdrType = -1; + + if (virPCIGetHeaderType(dev, &hdrType) < 0) + return -1; + + if (hdrType != VIR_PCI_HEADER_ENDPOINT) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid attempt to reset " + "non-endpoint PCI device %s. Only PCI endpoint " + "devices can be reset"), dev->name); + return -1; + } if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR,
On Mon, Jan 23, 2017 at 07:07:10PM +0530, Shivaprasad G Bhat wrote:
It is destructive to attempt reset on a pci-bridge, the host can crash. The bridges won't contain any guest data and neither they can be passed through using vfio/stub. So, no point in allowing a reset on them.
We should note somewhere that it's not something you want to do. But forbidding that is fine too.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 0601f49..ae72587 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -933,6 +933,17 @@ virPCIDeviceReset(virPCIDevicePtr dev, char *drvName = NULL; int ret = -1; int fd = -1; + int hdrType = -1; + + if (virPCIGetHeaderType(dev, &hdrType) < 0) + return -1; + + if (hdrType != VIR_PCI_HEADER_ENDPOINT) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid attempt to reset " + "non-endpoint PCI device %s. Only PCI endpoint " + "devices can be reset"), dev->name);
This can be reworded to be shorter. ACK otherwise, so I fixed this one and pushed it also.
+ return -1; + }
if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR,
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 23, 2017 at 07:06:29PM +0530, Shivaprasad G Bhat wrote:
Non-endpoint devices like pci-bridges cannot be passedthrough to guests.
"disallow" is a mouthful, I would also use "assigned" instead of "passedthrough".
Prevent such attempts.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virhostdev.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 0673afb..b23fe1f 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -532,6 +532,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == VIR_PCI_STUB_DRIVER_VFIO); struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVFIO }; + int hdrType = -1; + + if (virPCIGetHeaderType(pci, &hdrType) < 0) + goto cleanup; + + if (hdrType != VIR_PCI_HEADER_ENDPOINT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI bridge devices " + "cannot be assigned to guests"));
Actually, you should continue the string in the same column it was started, just start the long strings on their own lines, it's more readable. FTFY, ACK and pushed. I wanted you to make it part of virPCIDeviceIsAssignable(), but we use it kind of weirdly, so I'll keep it as-is, Laine (Cc'd) will know more about whether we want to move it there or not. And we can do that later.
+ goto cleanup; + }
if (!usesVFIO && !virPCIDeviceIsAssignable(pci, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID,
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/23/2017 11:34 AM, Martin Kletzander wrote:
On Mon, Jan 23, 2017 at 07:06:29PM +0530, Shivaprasad G Bhat wrote:
Non-endpoint devices like pci-bridges cannot be passedthrough to guests.
"disallow" is a mouthful, I would also use "assigned" instead of "passedthrough".
Correct - "passthrough" is considered a "bad word" by the VFIO maintainer, and shouldn't be used. VFIO's purpose is "device assignment".
Prevent such attempts.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virhostdev.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 0673afb..b23fe1f 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -532,6 +532,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == VIR_PCI_STUB_DRIVER_VFIO); struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVFIO }; + int hdrType = -1; + + if (virPCIGetHeaderType(pci, &hdrType) < 0) + goto cleanup; + + if (hdrType != VIR_PCI_HEADER_ENDPOINT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI bridge devices " + "cannot be assigned to guests"));
Actually, you should continue the string in the same column it was started, just start the long strings on their own lines, it's more readable.
Yeah, I didn't think to say that in my review.
FTFY, ACK and pushed.
I wanted you to make it part of virPCIDeviceIsAssignable(), but we use it kind of weirdly, so I'll keep it as-is, Laine (Cc'd) will know more
Not really :-P. I'm sure I'd looked at that function at least once before, but didn't remember it until you pointed it out. It's been there for a very long time with no change (introduced in Dec. 2009 by jdenemar) and is a leftover from legacy KVM assignment.
about whether we want to move it there or not.
In it's current form that wouldn't work, since virPCIDeviceIsAssignable() is only called when we're not using VFIO, but we actually want to *always* check the PCI header type, both for VFIO, *AND* legacy KVM. But if somebody wants to change the one call so that it's called unconditionally, and change the 2nd arg to say "strict_acs_check && !usesVFIO), then the check of device type could be moved to that function, which sounds like a reasonable thing, based on the name.
participants (3)
-
Laine Stump -
Martin Kletzander -
Shivaprasad G Bhat