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

[Shivaprasad bhat]

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 at 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 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
>>
>>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170118/c05f6029/attachment-0001.htm>