Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713655 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/3] vfio/pci: Don't probe devices that can't be reset Jan Glauber <jglauber@cavium.com> - 2017-08-17 10:20 +0200
    Re: [PATCH v2 3/3] vfio/pci: Don't probe devices that can't be  reset Alex Williamson <alex.williamson@redhat.com> - 2017-08-17 15:10 +0200
      Re: [PATCH v2 3/3] vfio/pci: Don't probe devices that can't be  reset Alex Williamson <alex.williamson@redhat.com> - 2017-08-18 16:20 +0200
        Re: [PATCH v2 3/3] vfio/pci: Don't probe devices that can't be  reset Alex Williamson <alex.williamson@redhat.com> - 2017-08-19 06:00 +0200

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

If a PCI device supports neither function-level reset, nor slot
or bus reset then refuse to probe it. A line is printed to inform
the user.

Without this change starting qemu with a vfio-pci device can lead to
a kernel panic on some Cavium cn8xxx systems, depending on the used
device.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/vfio/pci/vfio_pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 063c1ce..029ba13 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1196,6 +1196,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
 		return -EINVAL;
 
+	ret = pci_probe_reset_bus(pdev->bus);
+	if (ret) {
+		dev_warn(&pdev->dev, "Refusing to probe because reset is not possible.\n");
+		return ret;
+	}
+
 	group = vfio_iommu_group_get(&pdev->dev);
 	if (!group)
 		return -EINVAL;
-- 
2.9.0.rc0.21.g7777322

[toc] | [next] | [standalone]

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

On Thu, 17 Aug 2017 10:14:23 +0200
Jan Glauber <jglauber@cavium.com> wrote:

> If a PCI device supports neither function-level reset, nor slot
> or bus reset then refuse to probe it. A line is printed to inform
> the user.

But that's not what this does, this requires that the device is on a
reset-able bus.  This is a massive regression.  With this we could no
longer assign devices on the root complex or any device which doesn't
return from bus reset and currently makes use of the NO_BUS_RESET flag
and works happily otherwise.  Full NAK.  Thanks,

Alex
 
> Without this change starting qemu with a vfio-pci device can lead to
> a kernel panic on some Cavium cn8xxx systems, depending on the used
> device.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 063c1ce..029ba13 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1196,6 +1196,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>  		return -EINVAL;
>  
> +	ret = pci_probe_reset_bus(pdev->bus);
> +	if (ret) {
> +		dev_warn(&pdev->dev, "Refusing to probe because reset is not possible.\n");
> +		return ret;
> +	}
> +
>  	group = vfio_iommu_group_get(&pdev->dev);
>  	if (!group)
>  		return -EINVAL;

[toc] | [prev] | [next] | [standalone]

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

On Fri, 18 Aug 2017 15:42:31 +0200
Jan Glauber <jan.glauber@caviumnetworks.com> wrote:

> On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:
> > On Thu, 17 Aug 2017 10:14:23 +0200
> > Jan Glauber <jglauber@cavium.com> wrote:
> >   
> > > If a PCI device supports neither function-level reset, nor slot
> > > or bus reset then refuse to probe it. A line is printed to inform
> > > the user.  
> > 
> > But that's not what this does, this requires that the device is on a
> > reset-able bus.  This is a massive regression.  With this we could no
> > longer assign devices on the root complex or any device which doesn't
> > return from bus reset and currently makes use of the NO_BUS_RESET flag
> > and works happily otherwise.  Full NAK.  Thanks,  
> 
> Looks like I missed the slot reset check. So how about this:
> 
> if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
> 	dev_warn(...);
> 	return -ENODEV;
> }
> 
> Or am I still missing something here?

We don't require that a device is on a reset-able bus/slot, so any
attempt to impose that requirement means that there are devices that
might work perfectly fine that are now excluded from assignment.  The
entire premise is unacceptable.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

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

On Fri, 18 Aug 2017 08:57:09 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 08/18/2017 07:12 AM, Alex Williamson wrote:
> > On Fri, 18 Aug 2017 15:42:31 +0200
> > Jan Glauber <jan.glauber@caviumnetworks.com> wrote:
> >   
> >> On Thu, Aug 17, 2017 at 07:00:17AM -0600, Alex Williamson wrote:  
> >>> On Thu, 17 Aug 2017 10:14:23 +0200
> >>> Jan Glauber <jglauber@cavium.com> wrote:
> >>>      
> >>>> If a PCI device supports neither function-level reset, nor slot
> >>>> or bus reset then refuse to probe it. A line is printed to inform
> >>>> the user.  
> >>>
> >>> But that's not what this does, this requires that the device is on a
> >>> reset-able bus.  This is a massive regression.  With this we could no
> >>> longer assign devices on the root complex or any device which doesn't
> >>> return from bus reset and currently makes use of the NO_BUS_RESET flag
> >>> and works happily otherwise.  Full NAK.  Thanks,  
> >>
> >> Looks like I missed the slot reset check. So how about this:
> >>
> >> if (pci_probe_reset_slot(pdev->slot) && pci_probe_reset_bus(pdev->bus)) {
> >> 	dev_warn(...);
> >> 	return -ENODEV;
> >> }
> >>
> >> Or am I still missing something here?  
> > 
> > We don't require that a device is on a reset-able bus/slot, so any
> > attempt to impose that requirement means that there are devices that
> > might work perfectly fine that are now excluded from assignment.  The
> > entire premise is unacceptable.  Thanks,  
> 
> 
> 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.

> 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,

Alex

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web