Re: [PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset

[Jan Glauber]

On Fri, Aug 18, 2017 at 09:55:53PM -0600, Alex Williamson wrote:
> On Fri, 18 Aug 2017 08:57:09 -0700
> David Daney <ddaney@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > On 08/18/2017 07:12 AM, Alex Williamson wrote:

[...]

> > You previously rejected the idea to silently ignore bus reset requests 
> > on buses that do not support it.
> > 
> > So this leaves us with two options:
> > 
> > 1) Do nothing, and crash the kernel on systems with bad combinations of 
> > PCIe target devices and cn88xx when vfio_pci is used.
> > 
> > 2) Do something else.
> > 
> > We are trying to figure out what that something else should be.  The 
> > general concept we are working on is that if vfio_pci wants to reset a 
> > device, *and* bus reset is the only option available, *and* cn88xx, then 
> > make vfio_pci fail.
> 
> But that's not what these attempts do, they say if we can't do a bus or
> slot reset, fail the device probe.  The comment is trying to suggest
> they do something else, am I misinterpreting the actual code change?
> There are plenty of devices out there that don't care if bus reset
> doesn't work, they support FLR or PM reset or device specific reset or
> just deal without a reset.  We can't suddenly say this new thing is a
> requirement and sorry if you were happily using device assignment
> before, but there's a slim chance you're on this platform that falls
> over if we attempt to do a secondary bus reset.

Thanks for explaining this, I agree that we should not fail the device
probe as we only need to prevent the reset from happening.
So let's just drop this patch.


> > What is your opinion of doing that (assuming it is properly implemented)?
> 
> It seems like these attempts are trying to completely turn off vfio-pci
> on cn88xx, do you just want it unsupported on these platforms?  Should
> we blacklist anything where dev->bus->self is this root port?
> Otherwise, what's wrong with returning an error if a bus reset fails,
> because we should *never* silently ignore the request and pretend that
> it worked, perhaps even dev_warn()'ing that the platform doesn't
> support bus resets?  Thanks,

The ioctl's that trigger the slot/bus reset are already checking
if reset is possible. With David's patches pci_probe_reset_bus()
already fails.

But we also need to make pci_probe_reset_slot() fail on cn88xx to avoid
the same issue for the slot reset:

[  178.815041] [<fffffc000850b67c>] pci_generic_config_read+0x5c/0xf0
[  178.821221] [<fffffc0008534f60>] thunder_pem_config_read+0x90/0x228
[  178.827487] [<fffffc000850b564>] pci_bus_read_config_dword+0x84/0xb8
[  178.833841] [<fffffc000850d374>] pci_read_config_dword+0x5c/0x70
[  178.839848] [<fffffc0008513e54>] pci_find_next_ext_capability.part.7+0x44/0xc8
[  178.847075] [<fffffc0008514b00>] pci_find_ext_capability+0x48/0x58
[  178.853256] [<fffffc0008520e6c>] pci_restore_vc_state+0x44/0xa0
[  178.859175] [<fffffc0008514d4c>] pci_restore_state.part.26+0x3c/0x240
[  178.865614] [<fffffc0008514fe0>] pci_dev_restore+0x58/0x60
[  178.871098] [<fffffc00085150a0>] pci_slot_restore+0x60/0x78
[  178.876669] [<fffffc000851599c>] pci_try_reset_slot+0xcc/0x140
[  178.882512] [<fffffc0000d91b78>] vfio_pci_ioctl+0xb30/0xb88 [vfio_pci]
[  178.889050] [<fffffc0000ba02b4>] vfio_device_fops_unl_ioctl+0x44/0x70 [vfio]
[  178.896100] [<fffffc0008267e00>] do_vfs_ioctl+0xb0/0x748
[  178.901411] [<fffffc000826852c>] SyS_ioctl+0x94/0xa8
[  178.906375] [<fffffc00080834a0>] __sys_trace_return+0x0/0x4
[  178.911947] Code: 7100069f 540003c0 71000a9f 54000240 (b9400001) 
[  178.918108] ---[ end trace 07143dcba854194e ]---
[  178.922784] Kernel panic - not syncing: Fatal exception

So far I don't see how this can be done in a clean way, there is no quirk
available for the slot.

--Jan