[libvirt] [PATCH] util: disallow resetting a pci-bridge device
It is distructive to attempt a reset on 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 for them. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 0601f49..860f7aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -933,6 +933,16 @@ 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_PCI_BRIDGE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resetting a pci-bridge device is not allowed")); + return -1; + } if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR,
On 01/17/2017 09:51 AM, Shivaprasad G Bhat wrote:
It is distructive to attempt a reset on 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 for them.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 0601f49..860f7aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -933,6 +933,16 @@ virPCIDeviceReset(virPCIDevicePtr dev,
This function is a nice centralized place to perform this check, but it could lead to an odd error message if someone tried to assign a PCI bridge device to a guest - instead of logging "PCI bridge devices can't be assigned to guests" (or something like that) it would tell us that we can't *reset* the device. Maybe that can be remedied by doing an additional check at an earlier stage during virHostdevPreparePCIDevices().
char *drvName = NULL; int ret = -1; int fd = -1; + int hdrType = -1; + + if (virPCIGetHeaderType(dev, &hdrType) < 0) + return -1; + + if (hdrType == VIR_PCI_HEADER_PCI_BRIDGE) {
I think we can only reset (or assign) endpoint devices, so how about using "hdrType != VIR_PCI_HEADER_ENDPOINT", and then having the error message say "Invalid attempt to reset non-endpoint PCI device at xxxx:xx:xx.x. Only PCI endpoint devices can be reset"
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resetting a pci-bridge device is not allowed"));
Using "pci-bridge" could be confusing, since that's the exact name of a particular emulated device in libvirt. The error message should include the PCI address of the device, and (as suggested above) just say that it isn't an endpoint device, so we can't reset it.
+ 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
Thanks Laine. I agree to all your comments. I am checking the type to pci-bridge during virHostdevPreparePCIDevices in my V2 as I am not sure if the CARDBUS_BRIDGE is assignable or not. Let me know if you want to me change it to "not an endpoint" there. Regards, Shiva On Wed, Jan 18, 2017 at 12:15 AM, Laine Stump <laine@laine.org> wrote:
On 01/17/2017 09:51 AM, Shivaprasad G Bhat wrote:
It is distructive to attempt a reset on 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 for them.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/util/virpci.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 0601f49..860f7aa 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -933,6 +933,16 @@ virPCIDeviceReset(virPCIDevicePtr dev,
This function is a nice centralized place to perform this check, but it could lead to an odd error message if someone tried to assign a PCI bridge device to a guest - instead of logging "PCI bridge devices can't be assigned to guests" (or something like that) it would tell us that we can't *reset* the device. Maybe that can be remedied by doing an additional check at an earlier stage during virHostdevPreparePCIDevices().
char *drvName = NULL;
int ret = -1; int fd = -1; + int hdrType = -1; + + if (virPCIGetHeaderType(dev, &hdrType) < 0) + return -1; + + if (hdrType == VIR_PCI_HEADER_PCI_BRIDGE) {
I think we can only reset (or assign) endpoint devices, so how about using "hdrType != VIR_PCI_HEADER_ENDPOINT", and then having the error message say "Invalid attempt to reset non-endpoint PCI device at xxxx:xx:xx.x. Only PCI endpoint devices can be reset"
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Resetting a pci-bridge device is not allowed"));
Using "pci-bridge" could be confusing, since that's the exact name of a particular emulated device in libvirt. The error message should include the PCI address of the device, and (as suggested above) just say that it isn't an endpoint device, so we can't reset it.
+ 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Laine Stump -
Shivaprasad bhat -
Shivaprasad G Bhat