[PATCH] PCI: Add link_change error handler and vfio-pci user

[PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex Williamson]

The PCIe bandwidth notification service generates logging any time a
link changes speed or width to a state that is considered downgraded.
Unfortunately, it cannot differentiate signal integrity related link
changes from those intentionally initiated by an endpoint driver,
including drivers that may live in userspace or VMs when making use
of vfio-pci.  Therefore, allow the driver to have a say in whether
the link is indeed downgraded and worth noting in the log, or if the
change is perhaps intentional.

For vfio-pci, we don't know the intentions of the user/guest driver
either, but we do know that GPU drivers in guests actively manage
the link state and therefore trigger the bandwidth notification for
what appear to be entirely intentional link changes.

Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Changing to pci_dbg() logging is not super usable, so let's try the
previous idea of letting the driver handle link change events as they
see fit.  Ideally this might be two patches, but for easier handling,
folding the pci and vfio-pci bits together.  Comments?  Thanks,

Alex

 drivers/pci/probe.c         |   13 +++++++++++++
 drivers/vfio/pci/vfio_pci.c |   10 ++++++++++
 include/linux/pci.h         |    3 +++
 3 files changed, 26 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7e12d0163863..233cd4b5b6e8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)
 	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
 		return;
 
+	/*
+	 * If driver handles link_change event, defer to driver.  PCIe drivers
+	 * can call pcie_print_link_status() to print current link info.
+	 */
+	device_lock(&dev->dev);
+	if (dev->driver && dev->driver->err_handler &&
+	    dev->driver->err_handler->link_change) {
+		dev->driver->err_handler->link_change(dev);
+		device_unlock(&dev->dev);
+		return;
+	}
+	device_unlock(&dev->dev);
+
 	/* Print link status only if the device is constrained by the fabric */
 	__pcie_print_link_status(dev, false);
 }
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index cab71da46f4a..c9ffc0ccabb3 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 	return PCI_ERS_RESULT_CAN_RECOVER;
 }
 
+/*
+ * Ignore link change notification, we can't differentiate signal related
+ * link changes from user driver power management type operations, so do
+ * nothing.  Potentially this could be routed out to the user.
+ */
+static void vfio_pci_link_change(struct pci_dev *pdev)
+{
+}
+
 static const struct pci_error_handlers vfio_err_handlers = {
 	.error_detected = vfio_pci_aer_err_detected,
+	.link_change = vfio_pci_link_change,
 };
 
 static struct pci_driver vfio_pci_driver = {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 27854731afc4..e9194bc03f9e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -763,6 +763,9 @@ struct pci_error_handlers {
 
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
+
+	/* PCIe link change notification */
+	void (*link_change)(struct pci_dev *dev);
 };
 
 

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex_Gagniuc]

On 4/23/2019 5:42 PM, Alex Williamson wrote:
> The PCIe bandwidth notification service generates logging any time a
> link changes speed or width to a state that is considered downgraded.
> Unfortunately, it cannot differentiate signal integrity related link
> changes from those intentionally initiated by an endpoint driver,
> including drivers that may live in userspace or VMs when making use
> of vfio-pci.  Therefore, allow the driver to have a say in whether
> the link is indeed downgraded and worth noting in the log, or if the
> change is perhaps intentional.
> 
> For vfio-pci, we don't know the intentions of the user/guest driver
> either, but we do know that GPU drivers in guests actively manage
> the link state and therefore trigger the bandwidth notification for
> what appear to be entirely intentional link changes.
> 
> Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Changing to pci_dbg() logging is not super usable, so let's try the
> previous idea of letting the driver handle link change events as they
> see fit.  Ideally this might be two patches, but for easier handling,
> folding the pci and vfio-pci bits together.  Comments?  Thanks,

I think this callback opens up a can of worms where drivers can ad-hoc 
kill a number what otherwise can be indicators of problems. But I don't 
have to like it to review it :).

>   drivers/pci/probe.c         |   13 +++++++++++++
>   drivers/vfio/pci/vfio_pci.c |   10 ++++++++++
>   include/linux/pci.h         |    3 +++
>   3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 7e12d0163863..233cd4b5b6e8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)

I don't think you want to change pcie_report_downtraining(). You're 
advertising to "report" something, by nomenclature, but then go around 
and also call a notification callback. This is also used during probe, 
and you've now just killed your chance to notice you've booted with a 
degraded link.
If what you want to do is silence the bandwidth notification, you want 
to modify the threaded interrupt that calls this.

>   	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
>   		return;
>   
> +	/*
> +	 * If driver handles link_change event, defer to driver.  PCIe drivers
> +	 * can call pcie_print_link_status() to print current link info.
> +	 */
> +	device_lock(&dev->dev);
> +	if (dev->driver && dev->driver->err_handler &&
> +	    dev->driver->err_handler->link_change) {
> +		dev->driver->err_handler->link_change(dev);
> +		device_unlock(&dev->dev);
> +		return;
> +	}
> +	device_unlock(&dev->dev);

Can we write this such that there is a single lock()/unlock() pair?

> +
>   	/* Print link status only if the device is constrained by the fabric */
>   	__pcie_print_link_status(dev, false);
>   }
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index cab71da46f4a..c9ffc0ccabb3 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>   	return PCI_ERS_RESULT_CAN_RECOVER;
>   }
>   
> +/*
> + * Ignore link change notification, we can't differentiate signal related
> + * link changes from user driver power management type operations, so do
> + * nothing.  Potentially this could be routed out to the user.
> + */
> +static void vfio_pci_link_change(struct pci_dev *pdev)
> +{
> +}
> +
>   static const struct pci_error_handlers vfio_err_handlers = {
>   	.error_detected = vfio_pci_aer_err_detected,
> +	.link_change = vfio_pci_link_change,
>   };
>   
>   static struct pci_driver vfio_pci_driver = {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 27854731afc4..e9194bc03f9e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -763,6 +763,9 @@ struct pci_error_handlers {
>   
>   	/* Device driver may resume normal operations */
>   	void (*resume)(struct pci_dev *dev);
> +
> +	/* PCIe link change notification */
> +	void (*link_change)(struct pci_dev *dev);
>   };
>   
>   
> 
> 

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex Williamson]

On Wed, 24 Apr 2019 16:45:45 +0000
<Alex_Gagniuc@Dellteam.com> wrote:

> On 4/23/2019 5:42 PM, Alex Williamson wrote:
> > The PCIe bandwidth notification service generates logging any time a
> > link changes speed or width to a state that is considered downgraded.
> > Unfortunately, it cannot differentiate signal integrity related link
> > changes from those intentionally initiated by an endpoint driver,
> > including drivers that may live in userspace or VMs when making use
> > of vfio-pci.  Therefore, allow the driver to have a say in whether
> > the link is indeed downgraded and worth noting in the log, or if the
> > change is perhaps intentional.
> > 
> > For vfio-pci, we don't know the intentions of the user/guest driver
> > either, but we do know that GPU drivers in guests actively manage
> > the link state and therefore trigger the bandwidth notification for
> > what appear to be entirely intentional link changes.
> > 
> > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > Changing to pci_dbg() logging is not super usable, so let's try the
> > previous idea of letting the driver handle link change events as they
> > see fit.  Ideally this might be two patches, but for easier handling,
> > folding the pci and vfio-pci bits together.  Comments?  Thanks,  
> 
> I think this callback opens up a can of worms where drivers can ad-hoc 
> kill a number what otherwise can be indicators of problems. But I don't 
> have to like it to review it :).
> 
> >   drivers/pci/probe.c         |   13 +++++++++++++
> >   drivers/vfio/pci/vfio_pci.c |   10 ++++++++++
> >   include/linux/pci.h         |    3 +++
> >   3 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 7e12d0163863..233cd4b5b6e8 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)  
> 
> I don't think you want to change pcie_report_downtraining(). You're 
> advertising to "report" something, by nomenclature, but then go around 
> and also call a notification callback. This is also used during probe, 
> and you've now just killed your chance to notice you've booted with a 
> degraded link.
> If what you want to do is silence the bandwidth notification, you want 
> to modify the threaded interrupt that calls this.

During probe, ie. discovery, a device wouldn't have a driver attached,
so we'd fall through to simply printing the link status.  Nothing
lost afaict.  The "report" verb doesn't have a subject here, report to
whom?  Therefore I thought it reasonable that a driver ask that it be
reported to them via a callback.  I don't see that as such a stretch of
the interface.
 
> >   	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
> >   		return;
> >   
> > +	/*
> > +	 * If driver handles link_change event, defer to driver.  PCIe drivers
> > +	 * can call pcie_print_link_status() to print current link info.
> > +	 */
> > +	device_lock(&dev->dev);
> > +	if (dev->driver && dev->driver->err_handler &&
> > +	    dev->driver->err_handler->link_change) {
> > +		dev->driver->err_handler->link_change(dev);
> > +		device_unlock(&dev->dev);
> > +		return;
> > +	}
> > +	device_unlock(&dev->dev);  
> 
> Can we write this such that there is a single lock()/unlock() pair?

Not without introducing a tracking variable, ex.

bool handled = false;

lock()
if (stuff) {
  link_change()
  handled = true;
}
unlock()

if (!handled)
  dmesg spew

That's not markedly better imo, but if it's preferred I can send a v2.
Thanks,

Alex
 
> > +
> >   	/* Print link status only if the device is constrained by the fabric */
> >   	__pcie_print_link_status(dev, false);
> >   }
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index cab71da46f4a..c9ffc0ccabb3 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
> >   	return PCI_ERS_RESULT_CAN_RECOVER;
> >   }
> >   
> > +/*
> > + * Ignore link change notification, we can't differentiate signal related
> > + * link changes from user driver power management type operations, so do
> > + * nothing.  Potentially this could be routed out to the user.
> > + */
> > +static void vfio_pci_link_change(struct pci_dev *pdev)
> > +{
> > +}
> > +
> >   static const struct pci_error_handlers vfio_err_handlers = {
> >   	.error_detected = vfio_pci_aer_err_detected,
> > +	.link_change = vfio_pci_link_change,
> >   };
> >   
> >   static struct pci_driver vfio_pci_driver = {
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 27854731afc4..e9194bc03f9e 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -763,6 +763,9 @@ struct pci_error_handlers {
> >   
> >   	/* Device driver may resume normal operations */
> >   	void (*resume)(struct pci_dev *dev);
> > +
> > +	/* PCIe link change notification */
> > +	void (*link_change)(struct pci_dev *dev);
> >   };
> >   
> >   
> > 
> >   
> 
> 

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex G]

On 4/24/19 12:19 PM, Alex Williamson wrote:
> On Wed, 24 Apr 2019 16:45:45 +0000
> <Alex_Gagniuc@Dellteam.com> wrote:
>> On 4/23/2019 5:42 PM, Alex Williamson wrote:
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 7e12d0163863..233cd4b5b6e8 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)
>>
>> I don't think you want to change pcie_report_downtraining(). You're
>> advertising to "report" something, by nomenclature, but then go around
>> and also call a notification callback. This is also used during probe,
>> and you've now just killed your chance to notice you've booted with a
>> degraded link.
>> If what you want to do is silence the bandwidth notification, you want
>> to modify the threaded interrupt that calls this.
> 
> During probe, ie. discovery, a device wouldn't have a driver attached,
> so we'd fall through to simply printing the link status.  Nothing
> lost afaict.  The "report" verb doesn't have a subject here, report to
> whom?  Therefore I thought it reasonable that a driver ask that it be
> reported to them via a callback.  I don't see that as such a stretch of
> the interface.

That's just stretching the logic, and IMO makes the intent harder to 
understand. The argument relies on the state of the PCI device and 
logic, which is not obvious to the casual observer. If you want to 
bypass the bandwidth notification, then bypass the notification.

>>>    	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
>>>    		return;
>>>    
>>> +	/*
>>> +	 * If driver handles link_change event, defer to driver.  PCIe drivers
>>> +	 * can call pcie_print_link_status() to print current link info.
>>> +	 */
>>> +	device_lock(&dev->dev);
>>> +	if (dev->driver && dev->driver->err_handler &&
>>> +	    dev->driver->err_handler->link_change) {
>>> +		dev->driver->err_handler->link_change(dev);
>>> +		device_unlock(&dev->dev);
>>> +		return;
>>> +	}
>>> +	device_unlock(&dev->dev);
>>
>> Can we write this such that there is a single lock()/unlock() pair?
> 
> Not without introducing a tracking variable, ex.
[snip bad code]
> That's not markedly better imo, but if it's preferred I can send a v2.

How about:

if (!invoke_link_changed_handler(pdev))
	very_useful_downtraining_message(pdev);

> Alex
Alex

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Bjorn Helgaas]

On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote:
> The PCIe bandwidth notification service generates logging any time a
> link changes speed or width to a state that is considered downgraded.
> Unfortunately, it cannot differentiate signal integrity related link
> changes from those intentionally initiated by an endpoint driver,
> including drivers that may live in userspace or VMs when making use
> of vfio-pci.  Therefore, allow the driver to have a say in whether
> the link is indeed downgraded and worth noting in the log, or if the
> change is perhaps intentional.
> 
> For vfio-pci, we don't know the intentions of the user/guest driver
> either, but we do know that GPU drivers in guests actively manage
> the link state and therefore trigger the bandwidth notification for
> what appear to be entirely intentional link changes.
> 
> Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Changing to pci_dbg() logging is not super usable, so let's try the
> previous idea of letting the driver handle link change events as they
> see fit.  Ideally this might be two patches, but for easier handling,
> folding the pci and vfio-pci bits together.  Comments?  Thanks,

I'm a little uneasy about the bandwidth notification logging as a
whole.  Messages in dmesg don't seem like a solid base for building
management tools.

I assume the eventual goal would be some sort of digested notification
along the lines of "hey mr/ms administrator, the link to device X
unexpectedly became slower, you might want to check that out."

If I were building that, I don't think I would use dmesg.  I might
write a daemon that polls /sys/.../current_link_{speed,width}, or
maybe use some sort of netlink event.  Maybe it would be useful to
have the admin designate devices of interest.

I'm hesitant about adding a .link_change() handler.  If there's
something useful a driver could do with it, that's one thing.  But
using it merely to suppress a message doesn't really seem worth the
trouble, and it seems unfriendly to ask drivers to add it when they
didn't ask for it and get no benefit from it.

>  drivers/pci/probe.c         |   13 +++++++++++++
>  drivers/vfio/pci/vfio_pci.c |   10 ++++++++++
>  include/linux/pci.h         |    3 +++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 7e12d0163863..233cd4b5b6e8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)
>  	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
>  		return;
>  
> +	/*
> +	 * If driver handles link_change event, defer to driver.  PCIe drivers
> +	 * can call pcie_print_link_status() to print current link info.
> +	 */
> +	device_lock(&dev->dev);
> +	if (dev->driver && dev->driver->err_handler &&
> +	    dev->driver->err_handler->link_change) {
> +		dev->driver->err_handler->link_change(dev);
> +		device_unlock(&dev->dev);
> +		return;
> +	}
> +	device_unlock(&dev->dev);
> +
>  	/* Print link status only if the device is constrained by the fabric */
>  	__pcie_print_link_status(dev, false);
>  }
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index cab71da46f4a..c9ffc0ccabb3 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  	return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> +/*
> + * Ignore link change notification, we can't differentiate signal related
> + * link changes from user driver power management type operations, so do
> + * nothing.  Potentially this could be routed out to the user.
> + */
> +static void vfio_pci_link_change(struct pci_dev *pdev)
> +{
> +}
> +
>  static const struct pci_error_handlers vfio_err_handlers = {
>  	.error_detected = vfio_pci_aer_err_detected,
> +	.link_change = vfio_pci_link_change,
>  };
>  
>  static struct pci_driver vfio_pci_driver = {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 27854731afc4..e9194bc03f9e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -763,6 +763,9 @@ struct pci_error_handlers {
>  
>  	/* Device driver may resume normal operations */
>  	void (*resume)(struct pci_dev *dev);
> +
> +	/* PCIe link change notification */
> +	void (*link_change)(struct pci_dev *dev);
>  };
>  
>  
> 

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex Williamson]

On Wed, 24 Apr 2019 12:57:58 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote:
> > The PCIe bandwidth notification service generates logging any time a
> > link changes speed or width to a state that is considered downgraded.
> > Unfortunately, it cannot differentiate signal integrity related link
> > changes from those intentionally initiated by an endpoint driver,
> > including drivers that may live in userspace or VMs when making use
> > of vfio-pci.  Therefore, allow the driver to have a say in whether
> > the link is indeed downgraded and worth noting in the log, or if the
> > change is perhaps intentional.
> > 
> > For vfio-pci, we don't know the intentions of the user/guest driver
> > either, but we do know that GPU drivers in guests actively manage
> > the link state and therefore trigger the bandwidth notification for
> > what appear to be entirely intentional link changes.
> > 
> > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > Changing to pci_dbg() logging is not super usable, so let's try the
> > previous idea of letting the driver handle link change events as they
> > see fit.  Ideally this might be two patches, but for easier handling,
> > folding the pci and vfio-pci bits together.  Comments?  Thanks,  
> 
> I'm a little uneasy about the bandwidth notification logging as a
> whole.  Messages in dmesg don't seem like a solid base for building
> management tools.
> 
> I assume the eventual goal would be some sort of digested notification
> along the lines of "hey mr/ms administrator, the link to device X
> unexpectedly became slower, you might want to check that out."
> 
> If I were building that, I don't think I would use dmesg.  I might
> write a daemon that polls /sys/.../current_link_{speed,width}, or
> maybe use some sort of netlink event.  Maybe it would be useful to
> have the admin designate devices of interest.
> 
> I'm hesitant about adding a .link_change() handler.  If there's
> something useful a driver could do with it, that's one thing.  But
> using it merely to suppress a message doesn't really seem worth the
> trouble, and it seems unfriendly to ask drivers to add it when they
> didn't ask for it and get no benefit from it.

So where do we go from here?  I agree that dmesg is not necessarily a
great choice for these sorts of events and if they went somewhere else,
maybe I wouldn't have the same concerns about them generating user
confusion or contributing to DoS vectors from userspace drivers.  As it
is though, we have known cases where benign events generate confusing
logging messages, which seems like a regression.  Drivers didn't ask
for a link_change handler, but nor did they ask that the link state to
their device be monitored so closely.  Maybe this not only needs some
sort of change to the logging mechanism, but also an opt-in by the
driver if they don't expect runtime link changes.  Thanks,

Alex

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Sinan Kaya]

On 4/29/2019 10:51 AM, Alex Williamson wrote:
> So where do we go from here?  I agree that dmesg is not necessarily a
> great choice for these sorts of events and if they went somewhere else,
> maybe I wouldn't have the same concerns about them generating user
> confusion or contributing to DoS vectors from userspace drivers.  As it
> is though, we have known cases where benign events generate confusing
> logging messages, which seems like a regression.  Drivers didn't ask
> for a link_change handler, but nor did they ask that the link state to
> their device be monitored so closely.  Maybe this not only needs some
> sort of change to the logging mechanism, but also an opt-in by the
> driver if they don't expect runtime link changes.  Thanks,

Is there anyway to detect autonomous hardware management support and
not report link state changes in that situation?

I thought there were some capability bits for these.

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex Williamson]

On Mon, 29 Apr 2019 09:45:28 -0700
Sinan Kaya <Okaya@kernel.org> wrote:

> On 4/29/2019 10:51 AM, Alex Williamson wrote:
> > So where do we go from here?  I agree that dmesg is not necessarily a
> > great choice for these sorts of events and if they went somewhere else,
> > maybe I wouldn't have the same concerns about them generating user
> > confusion or contributing to DoS vectors from userspace drivers.  As it
> > is though, we have known cases where benign events generate confusing
> > logging messages, which seems like a regression.  Drivers didn't ask
> > for a link_change handler, but nor did they ask that the link state to
> > their device be monitored so closely.  Maybe this not only needs some
> > sort of change to the logging mechanism, but also an opt-in by the
> > driver if they don't expect runtime link changes.  Thanks,  
> 
> Is there anyway to detect autonomous hardware management support and
> not report link state changes in that situation?
> 
> I thought there were some capability bits for these.

Not that we can find, this doesn't trigger the separate autonomous
bandwidth notification interrupt.  Thanks,

Alex

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Keith Busch]

On Mon, Apr 29, 2019 at 10:59:26AM -0600, Alex Williamson wrote:
> On Mon, 29 Apr 2019 09:45:28 -0700
> Sinan Kaya <Okaya@kernel.org> wrote:
> 
> > On 4/29/2019 10:51 AM, Alex Williamson wrote:
> > > So where do we go from here?  I agree that dmesg is not necessarily a
> > > great choice for these sorts of events and if they went somewhere else,
> > > maybe I wouldn't have the same concerns about them generating user
> > > confusion or contributing to DoS vectors from userspace drivers.  As it
> > > is though, we have known cases where benign events generate confusing
> > > logging messages, which seems like a regression.  Drivers didn't ask
> > > for a link_change handler, but nor did they ask that the link state to
> > > their device be monitored so closely.  Maybe this not only needs some
> > > sort of change to the logging mechanism, but also an opt-in by the
> > > driver if they don't expect runtime link changes.  Thanks,  
> > 
> > Is there anyway to detect autonomous hardware management support and
> > not report link state changes in that situation?
> > 
> > I thought there were some capability bits for these.
> 
> Not that we can find, this doesn't trigger the separate autonomous
> bandwidth notification interrupt.  Thanks,

I think the only control is to disable automomous lane and link rate
changes. When set, any changes to either should only be in response to
errors, so enabling those controls might be the right thing to do with
this feature.

Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Bjorn Helgaas]

On Mon, Apr 29, 2019 at 08:51:04AM -0600, Alex Williamson wrote:
> On Wed, 24 Apr 2019 12:57:58 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote:
> > > The PCIe bandwidth notification service generates logging any time a
> > > link changes speed or width to a state that is considered downgraded.
> > > Unfortunately, it cannot differentiate signal integrity related link
> > > changes from those intentionally initiated by an endpoint driver,
> > > including drivers that may live in userspace or VMs when making use
> > > of vfio-pci.  Therefore, allow the driver to have a say in whether
> > > the link is indeed downgraded and worth noting in the log, or if the
> > > change is perhaps intentional.
> > > 
> > > For vfio-pci, we don't know the intentions of the user/guest driver
> > > either, but we do know that GPU drivers in guests actively manage
> > > the link state and therefore trigger the bandwidth notification for
> > > what appear to be entirely intentional link changes.
> > > 
> > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> > > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > > 
> > > Changing to pci_dbg() logging is not super usable, so let's try the
> > > previous idea of letting the driver handle link change events as they
> > > see fit.  Ideally this might be two patches, but for easier handling,
> > > folding the pci and vfio-pci bits together.  Comments?  Thanks,  
> > 
> > I'm a little uneasy about the bandwidth notification logging as a
> > whole.  Messages in dmesg don't seem like a solid base for building
> > management tools.
> > 
> > I assume the eventual goal would be some sort of digested notification
> > along the lines of "hey mr/ms administrator, the link to device X
> > unexpectedly became slower, you might want to check that out."
> > 
> > If I were building that, I don't think I would use dmesg.  I might
> > write a daemon that polls /sys/.../current_link_{speed,width}, or
> > maybe use some sort of netlink event.  Maybe it would be useful to
> > have the admin designate devices of interest.
> > 
> > I'm hesitant about adding a .link_change() handler.  If there's
> > something useful a driver could do with it, that's one thing.  But
> > using it merely to suppress a message doesn't really seem worth the
> > trouble, and it seems unfriendly to ask drivers to add it when they
> > didn't ask for it and get no benefit from it.
> 
> So where do we go from here?  I agree that dmesg is not necessarily a
> great choice for these sorts of events and if they went somewhere else,
> maybe I wouldn't have the same concerns about them generating user
> confusion or contributing to DoS vectors from userspace drivers.  As it
> is though, we have known cases where benign events generate confusing
> logging messages, which seems like a regression.  Drivers didn't ask
> for a link_change handler, but nor did they ask that the link state to
> their device be monitored so closely.  Maybe this not only needs some
> sort of change to the logging mechanism, but also an opt-in by the
> driver if they don't expect runtime link changes.  Thanks,

I think it's really too late in the cycle to rework this and get
changes merged before the v5.1 release (probably on May 5), so I'll
queue up a revert and we can iron out the wrinkles for v5.2.

Bjorn