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

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

[Hao, Xudong]

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

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;
+
 	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;

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

[Jan Beulich]

>>> On 04.05.12 at 09:49, "Hao, Xudong" <xudong.hao@intel.com> 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
> 
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>

I continue to disagree to doing this always, unconditionally, with
hard-coded settings. If these features require explicit enabling,
there likely is something that drivers unaware of them could have
problems with - otherwise I would think these features would be
always enabled when a device supports them, and no options
would exist to control certain parameters.

Plus, if the patch is nevertheless going to be acceptable, ...

> --- 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;
> +
>  	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");

... dev_err() here and below is certainly too severe, particularly for
the -ENOTSUPP cases. And the spelling would need fixing, especially
with it being broken exactly the same way a second time below.

> +
> +	err = pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);

What's the point of calling this function when pci_enable_ltr() failed?

Jan

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

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

[Hao, Xudong]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, May 04, 2012 4:07 PM
> To: Hao, Xudong
> Cc: xen-devel; konrad.wilk@oracle.com
> Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned
> by pciback
> 
> >>> On 04.05.12 at 09:49, "Hao, Xudong" <xudong.hao@intel.com> 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
> >
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> 
> I continue to disagree to doing this always, unconditionally, with
> hard-coded settings. If these features require explicit enabling,
> there likely is something that drivers unaware of them could have
> problems with - otherwise I would think these features would be
> always enabled when a device supports them, and no options
> would exist to control certain parameters.
> 
I agree the driver control it, but it's difficult to let guest driver do it till now(because of PCIe emulation by qemu issue).

> Plus, if the patch is nevertheless going to be acceptable, ...
> 
> > --- 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;
> > +
> >  	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");
> 
> ... dev_err() here and below is certainly too severe, particularly for
> the -ENOTSUPP cases. And the spelling would need fixing, especially
> with it being broken exactly the same way a second time below.
> 

dev_alert should be ok. Thanks for the careful review of typo "enable", I'll modify it in next version.

> > +
> > +	err = pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> 
> What's the point of calling this function when pci_enable_ltr() failed?
> 

Oh, thanks the point.
It's unnecessary to call this function when pci_enable_ltr() failed indeed, I'll insert this calling in pci_enable_ltr() passing logical in next version.

> Jan
> 
> > +	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
> 
> 

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

[Jan Beulich]

>>> On 06.05.12 at 10:10, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 04.05.12 at 09:49, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> I continue to disagree to doing this always, unconditionally, with
>> hard-coded settings. If these features require explicit enabling,
>> there likely is something that drivers unaware of them could have
>> problems with - otherwise I would think these features would be
>> always enabled when a device supports them, and no options
>> would exist to control certain parameters.
>> 
> I agree the driver control it, but it's difficult to let guest driver do it 
> till now(because of PCIe emulation by qemu issue).

Then why not think about how to address this properly?

>> > +	/* 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");
>> 
>> ... dev_err() here and below is certainly too severe, particularly for
>> the -ENOTSUPP cases. And the spelling would need fixing, especially
>> with it being broken exactly the same way a second time below.
>> 
> 
> dev_alert should be ok.

dev_alert() is even higher severity, whereas I was hinting towards
lowering it (and perhaps being completely silent upon encountering
-ENOTSUPP), i.e. dev_warn() or even dev_notice(), .

> Thanks for the careful review of typo "enable", I'll 
> modify it in next version.

Plus the one in "Could".

Jan

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

[Hao, Xudong]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, May 07, 2012 3:25 PM
> To: Hao, Xudong
> Cc: xen-devel; konrad.wilk@oracle.com
> Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned
> by pciback
> 
> >>> On 06.05.12 at 10:10, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> On 04.05.12 at 09:49, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> >> I continue to disagree to doing this always, unconditionally, with
> >> hard-coded settings. If these features require explicit enabling,
> >> there likely is something that drivers unaware of them could have
> >> problems with - otherwise I would think these features would be
> >> always enabled when a device supports them, and no options
> >> would exist to control certain parameters.
> >>
> > I agree the driver control it, but it's difficult to let guest driver do it
> > till now(because of PCIe emulation by qemu issue).
> 
> Then why not think about how to address this properly?
> 
I means the patch is the most appropriate method now. 

When qemu can emulate PCIe, then we shall migrate the method to guest driver controlling.

> >> > +	/* 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");
> >>
> >> ... dev_err() here and below is certainly too severe, particularly for
> >> the -ENOTSUPP cases. And the spelling would need fixing, especially
> >> with it being broken exactly the same way a second time below.
> >>
> >
> > dev_alert should be ok.
> 
> dev_alert() is even higher severity, whereas I was hinting towards
> lowering it (and perhaps being completely silent upon encountering
> -ENOTSUPP), i.e. dev_warn() or even dev_notice(), .
> 
> > Thanks for the careful review of typo "enable", I'll
> > modify it in next version.
> 
> Plus the one in "Could".
> 
Thanks.

> Jan

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

[Konrad Rzeszutek Wilk]

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?

> 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?
> +
>  	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

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

[Hao, Xudong]

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

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

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

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

[Konrad Rzeszutek Wilk]

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

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

[Hao, Xudong]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Monday, May 07, 2012 9:54 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 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?

Make it clear, here host side do not be considered, host has its own driver. The only thing is to make sure ltr/obff is enabled before assigning guest, so that benefit on power.

Since device is owned by pciback again when it un-assigned from guest, we need not disable explicitly.
  
> >
> > > > 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?
> 

Since there is not real device with ltr/obff supported now, so we can't determine the platform's maximum supported latency. I think the best way is using the default value 0, so I'd like to remove value setting. (if device is reset, the value still is 0)

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

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

[Jan Beulich]

>>> On 08.05.12 at 11:05, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>>  -----Original Message-----
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> On Sun, May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
>> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> > > 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?
> 
> Make it clear, here host side do not be considered, host has its own driver. 
> The only thing is to make sure ltr/obff is enabled before assigning guest, so 
> that benefit on power.
> 
> Since device is owned by pciback again when it un-assigned from guest, we 
> need not disable explicitly.

As you didn't answer my respective earlier question - _if_ this is a
feature needing enabling (and parameter tweaking), I'd imply there
are possible incompatibilities (i.e. reasons for not enabling this
always), and hence this shouldn't be done universally (and with
fixed values for the parameters) _and_ should be properly reset.

Jan

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

[Hao, Xudong]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, May 08, 2012 5:42 PM
> To: Hao, Xudong; Konrad Rzeszutek Wilk
> Cc: xen-devel
> Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned
> by pciback
> 
> >>> On 08.05.12 at 11:05, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> >>  -----Original Message-----
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> On Sun, May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
> >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> > > 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?
> >
> > Make it clear, here host side do not be considered, host has its own driver.
> > The only thing is to make sure ltr/obff is enabled before assigning guest, so
> > that benefit on power.
> >
> > Since device is owned by pciback again when it un-assigned from guest, we
> > need not disable explicitly.
> 
> As you didn't answer my respective earlier question - _if_ this is a
> feature needing enabling (and parameter tweaking), I'd imply there
> are possible incompatibilities (i.e. reasons for not enabling this
> always), and hence this shouldn't be done universally (and with
> fixed values for the parameters) _and_ should be properly reset.
> 
Only Xen administrator can hide a device by pciback, and can assign a device to guest. These power feature such as ltr and obff should be enabled by a sys-admin, I do not know which situation is a possible disabling these features, and why sys-admin want to disable them?

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

[Zhang, Xiantao]

Hi, Jan/Konrad
   Do you have further comments about this patch ?   Thanks!
Xiantao

> -----Original Message-----
> From: Hao, Xudong
> Sent: Wednesday, May 09, 2012 2:26 PM
> To: Jan Beulich; Konrad Rzeszutek Wilk
> Cc: xen-devel; Zhang, Xiantao
> Subject: RE: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> owned by pciback
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Tuesday, May 08, 2012 5:42 PM
> > To: Hao, Xudong; Konrad Rzeszutek Wilk
> > Cc: xen-devel
> > Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> > owned by pciback
> >
> > >>> On 08.05.12 at 11:05, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> > >>  -----Original Message-----
> > >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] On Sun,
> > >> May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
> > >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > >> > > 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?
> > >
> > > Make it clear, here host side do not be considered, host has its own
> driver.
> > > The only thing is to make sure ltr/obff is enabled before assigning
> > > guest, so that benefit on power.
> > >
> > > Since device is owned by pciback again when it un-assigned from
> > > guest, we need not disable explicitly.
> >
> > As you didn't answer my respective earlier question - _if_ this is a
> > feature needing enabling (and parameter tweaking), I'd imply there are
> > possible incompatibilities (i.e. reasons for not enabling this
> > always), and hence this shouldn't be done universally (and with fixed
> > values for the parameters) _and_ should be properly reset.
> >
> Only Xen administrator can hide a device by pciback, and can assign a device
> to guest. These power feature such as ltr and obff should be enabled by a
> sys-admin, I do not know which situation is a possible disabling these
> features, and why sys-admin want to disable them?

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

[Jan Beulich]

>>> On 22.05.12 at 03:57, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote:
>    Do you have further comments about this patch ?   Thanks!

I don't have anything beyond the reservations I have already
expressed (and which remain in place).

Jan

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

[Zhang, Xiantao]

Hi, Jan 
Okay,  I understand your concern about this implementation, and you think ltr/obff should be enabled by guest instead of host.   AS we know, LTR/OBFF is a special PCI-e feature for optimizing the whole system's  PM, and to enable it, the whole system(including root port and all upstream bridges or ports) should have these two capabilities. If anyone in this chain doesn't have or enable  these two capabilities, the whole system would fail to benefit from this optimized PM.   In addition, since Qemu doesn't have PCI-e support, and also hasn't such  LTR/OBFF capabilities supported,  guest shouldn't be able to enable this feature for its any device. 
Certainly, we can change linux's pci interface and para-virtualize this two features, in this way, guest can ask host to enable this feature, but how to handle Windows guest's case ?  We can't change or para-virtualize Windows's PCI interface, I think. Do you have good suggestion or proposals?  Thanks!
Xiantao




> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, May 22, 2012 3:21 PM
> To: Zhang, Xiantao; Hao, Xudong
> Cc: xen-devel; Konrad Rzeszutek Wilk
> Subject: RE: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> owned by pciback
> 
> >>> On 22.05.12 at 03:57, "Zhang, Xiantao" <xiantao.zhang@intel.com>
> wrote:
> >    Do you have further comments about this patch ?   Thanks!
> 
> I don't have anything beyond the reservations I have already expressed (and
> which remain in place).
> 
> Jan

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

[Jan Beulich]

>>> On 23.05.12 at 16:45, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote:
> Okay,  I understand your concern about this implementation, and you think 
> ltr/obff should be enabled by guest instead of host.   AS we know, LTR/OBFF 
> is a special PCI-e feature for optimizing the whole system's  PM, and to 
> enable it, the whole system(including root port and all upstream bridges or 
> ports) should have these two capabilities. If anyone in this chain doesn't 
> have or enable  these two capabilities, the whole system would fail to 
> benefit from this optimized PM.   In addition, since Qemu doesn't have PCI-e 
> support, and also hasn't such  LTR/OBFF capabilities supported,  guest 
> shouldn't be able to enable this feature for its any device. 
> Certainly, we can change linux's pci interface and para-virtualize this two 
> features, in this way, guest can ask host to enable this feature, but how to 
> handle Windows guest's case ?  We can't change or para-virtualize Windows's 
> PCI interface, I think. Do you have good suggestion or proposals?  Thanks!

Fix qemu to support PCI-e (and then make use of the to be added
PV interface).

Jan

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

[Zhang, Xiantao]

Hi, Jan
It is over-engineering to add PCI-e support to Qemu  for enabling this feature.  Basically, LTR/OBFF is a host PM feature,  and this patch only make this feature still work after the device is assigned to one guest.   Our goal is not to enable guest's LTR/OBFF support, and just let host not lose this advanced PM feature. Maybe we can move the logic to hypervisor like today's ATS feature does,  and once hypervisor finds the assigned device has this feature,  it can determine whether needs to enable it or not.   Thanks!
Xiantao

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, May 23, 2012 11:08 PM
> To: Zhang, Xiantao; Hao, Xudong
> Cc: xen-devel; Konrad Rzeszutek Wilk
> Subject: RE: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> owned by pciback
> 
> >>> On 23.05.12 at 16:45, "Zhang, Xiantao" <xiantao.zhang@intel.com>
> wrote:
> > Okay,  I understand your concern about this implementation, and you think
> > ltr/obff should be enabled by guest instead of host.   AS we know,
> LTR/OBFF
> > is a special PCI-e feature for optimizing the whole system's  PM, and
> > to enable it, the whole system(including root port and all upstream
> > bridges or
> > ports) should have these two capabilities. If anyone in this chain
> > doesn't have or enable  these two capabilities, the whole system would fail
> to
> > benefit from this optimized PM.   In addition, since Qemu doesn't have PCI-
> e
> > support, and also hasn't such  LTR/OBFF capabilities supported,  guest
> > shouldn't be able to enable this feature for its any device.
> > Certainly, we can change linux's pci interface and para-virtualize
> > this two features, in this way, guest can ask host to enable this
> > feature, but how to handle Windows guest's case ?  We can't change or
> > para-virtualize Windows's PCI interface, I think. Do you have good
> suggestion or proposals?  Thanks!
> 
> Fix qemu to support PCI-e (and then make use of the to be added PV
> interface).
> 
> Jan

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

[Konrad Rzeszutek Wilk]

On Tue, May 22, 2012 at 01:57:44AM +0000, Zhang, Xiantao wrote:
> Hi, Jan/Konrad
>    Do you have further comments about this patch ?   Thanks!

Well .. What about making this be in the generic code? That way
both KVM and Xen can benefit? And also then the default values
don't have to show up in two places.


> Xiantao
> 
> > -----Original Message-----
> > From: Hao, Xudong
> > Sent: Wednesday, May 09, 2012 2:26 PM
> > To: Jan Beulich; Konrad Rzeszutek Wilk
> > Cc: xen-devel; Zhang, Xiantao
> > Subject: RE: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> > owned by pciback
> > 
> > > -----Original Message-----
> > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > Sent: Tuesday, May 08, 2012 5:42 PM
> > > To: Hao, Xudong; Konrad Rzeszutek Wilk
> > > Cc: xen-devel
> > > Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> > > owned by pciback
> > >
> > > >>> On 08.05.12 at 11:05, "Hao, Xudong" <xudong.hao@intel.com> wrote:
> > > >>  -----Original Message-----
> > > >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] On Sun,
> > > >> May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
> > > >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > >> > > 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?
> > > >
> > > > Make it clear, here host side do not be considered, host has its own
> > driver.
> > > > The only thing is to make sure ltr/obff is enabled before assigning
> > > > guest, so that benefit on power.
> > > >
> > > > Since device is owned by pciback again when it un-assigned from
> > > > guest, we need not disable explicitly.
> > >
> > > As you didn't answer my respective earlier question - _if_ this is a
> > > feature needing enabling (and parameter tweaking), I'd imply there are
> > > possible incompatibilities (i.e. reasons for not enabling this
> > > always), and hence this shouldn't be done universally (and with fixed
> > > values for the parameters) _and_ should be properly reset.
> > >
> > Only Xen administrator can hide a device by pciback, and can assign a device
> > to guest. These power feature such as ltr and obff should be enabled by a
> > sys-admin, I do not know which situation is a possible disabling these
> > features, and why sys-admin want to disable them?

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

[Zhang, Xiantao]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Wednesday, May 23, 2012 4:34 AM
> To: Zhang, Xiantao
> Cc: Hao, Xudong; Jan Beulich; xen-devel
> Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is
> owned by pciback
> 
> On Tue, May 22, 2012 at 01:57:44AM +0000, Zhang, Xiantao wrote:
> > Hi, Jan/Konrad
> >    Do you have further comments about this patch ?   Thanks!
> 
> Well .. What about making this be in the generic code? That way both KVM
> and Xen can benefit? And also then the default values don't have to show up
> in two places.

Good point! Maybe we can merge the implementations for Xen and KVM, and pci-stub driver maybe the proper place for accommodating such logic, I think.  Thanks!  
Xiantao



> > Xiantao
> >
> > > -----Original Message-----
> > > From: Hao, Xudong
> > > Sent: Wednesday, May 09, 2012 2:26 PM
> > > To: Jan Beulich; Konrad Rzeszutek Wilk
> > > Cc: xen-devel; Zhang, Xiantao
> > > Subject: RE: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device
> > > is owned by pciback
> > >
> > > > -----Original Message-----
> > > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > > Sent: Tuesday, May 08, 2012 5:42 PM
> > > > To: Hao, Xudong; Konrad Rzeszutek Wilk
> > > > Cc: xen-devel
> > > > Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device
> > > > is owned by pciback
> > > >
> > > > >>> On 08.05.12 at 11:05, "Hao, Xudong" <xudong.hao@intel.com>
> wrote:
> > > > >>  -----Original Message-----
> > > > >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] On
> > > > >> Sun, May 06, 2012 at 07:35:47AM +0000, Hao, Xudong wrote:
> > > > >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > > >> > > 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?
> > > > >
> > > > > Make it clear, here host side do not be considered, host has its
> > > > > own
> > > driver.
> > > > > The only thing is to make sure ltr/obff is enabled before
> > > > > assigning guest, so that benefit on power.
> > > > >
> > > > > Since device is owned by pciback again when it un-assigned from
> > > > > guest, we need not disable explicitly.
> > > >
> > > > As you didn't answer my respective earlier question - _if_ this is
> > > > a feature needing enabling (and parameter tweaking), I'd imply
> > > > there are possible incompatibilities (i.e. reasons for not
> > > > enabling this always), and hence this shouldn't be done
> > > > universally (and with fixed values for the parameters) _and_ should be
> properly reset.
> > > >
> > > Only Xen administrator can hide a device by pciback, and can assign
> > > a device to guest. These power feature such as ltr and obff should
> > > be enabled by a sys-admin, I do not know which situation is a
> > > possible disabling these features, and why sys-admin want to disable
> them?