Re: [PATCH v2] Re-enable LTR/OBFF when device is owned by pciback

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Sun, May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
> 
> > -----Original Message-----
> > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > Sent: Friday, May 04, 2012 9:21 PM
> > To: Hao, Xudong
> > Cc: xen-devel@lists.xensource.com
> > Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned
> > by pciback
> > 
> > On Fri, May 04, 2012 at 07:49:21AM +0000, Hao, Xudong wrote:
> > > When PCIE device which has LTR/OBFF capabilities is owned by pciback,
> > LTR/OBFF feature may be disabled. This patch re-enable LTR and OBFF, so that
> > guest with device assigned can be benefitted.
> > >
> > > Changes from v1:
> > > - put the variable definition at the start of function
> > > - add error log report
> > >
> > 
> > Don't you need to disable this when the device is un-assigned from the guest?
> > 
> 
> I don't think this need to be disabled when device is un-assigned from guest. If host want to use device again, the host device driver need re-load, so whether disabling ltr/obff is up to host device driver.

What if the driver isn't doing that?
> 
> > > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > >
> > > diff --git a/drivers/xen/xen-pciback/pci_stub.c
> > b/drivers/xen/xen-pciback/pci_stub.c
> > > index 097e536..74fbf23 100644
> > > --- a/drivers/xen/xen-pciback/pci_stub.c
> > > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > > @@ -313,6 +313,10 @@ static int __devinit pcistub_init_device(struct
> > pci_dev *dev)
> > >  	struct xen_pcibk_dev_data *dev_data;
> > >  	int err = 0;
> > >
> > > +	/* set default value */
> > > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> > > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024;
> > 
> > Why these values? Is there a way to fetch optimal values?
> 
> These values is the max value that the register supported. I've no idea to get an optimal value, here it just be set max value as default.

Is the max value defined somewhere? Can it be defined somewhere so that
both KVM adn the Xen patches re-use the same #define?

> 
> > > +
> > >  	dev_dbg(&dev->dev, "initializing...\n");
> > >
> > >  	/* The PCI backend is not intended to be a module (or to work with
> > > @@ -369,6 +373,28 @@ static int __devinit pcistub_init_device(struct
> > pci_dev *dev)
> > >  	dev_dbg(&dev->dev, "reset device\n");
> > >  	xen_pcibk_reset_device(dev);
> > >
> > > +	/* Enable LTR and OBFF before do device assignment */
> > > +	/* LTR(Latency tolerance reporting) allows devices to send
> > > +	 * messages to the root complex indicating their latency tolerance
> > > +	 * for snooped & unsnooped memory transactions.
> > > +	 */
> > > +	err = pci_enable_ltr(dev);
> > > +	if (err)
> > > +		dev_err(&dev->dev, "Counld not enalbe LTR for device!\n");
> > > +
> > > +	err = pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> > > +	if (err)
> > > +		dev_err(&dev->dev, "Set LTR latency values failed.\n");
> > > +
> > > +	/* OBFF (optimized buffer flush/fill), where supported, can help
> > > +	 * improve energy efficiency by giving devices information about
> > > +	 * when interrupts and other activity will have a reduced power
> > > +	 * impact.
> > > +	 */
> > > +	err = pci_enable_obff(dev, type);
> > > +	if (err)
> > > +		dev_err(&dev->dev, "Counld not enalbe OBFF for device!\n");
> > > +
> > >  	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> > >  	return 0;
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel