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

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;

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

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

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

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

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

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

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

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

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

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

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?

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

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?

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

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

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

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