[libvirt] [PATCH] util: disallow resetting a pci-bridge device

[Laine Stump]

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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>