Xen devel - Apr 2012 - [PATCH] 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.

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 19834d1..82aac5b 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -334,6 +334,14 @@ static int __devinit pcistub_init_device(struct pci_dev
*dev)
 	dev_dbg(&dev->dev, "reset device\n");
 	xen_pcibk_reset_device(dev);
 
+	/* set default value */
+	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
+	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max value */
+
+	/* 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. 
+	 */
+	pci_enable_ltr(dev);
+	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
+
+	/* 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.
+	 */
+	pci_enable_obff(dev, type);
+
 	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
 	return 0;

On Sun, Apr 22, 2012 at 03:25:04PM +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.
Shouldn''t they be reset when the device is de-assigned from the guest
as well?
> 
> 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 19834d1..82aac5b 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -334,6 +334,14 @@ static int __devinit pcistub_init_device(struct
pci_dev *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> +	/* set default value */
> +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max value */

Please put them in the appropiate location - at the start of the 
function.
> +
> +	/* 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. 
> +	 */
> +	pci_enable_ltr(dev);
> +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
OK, but didn''t you just come with up those values?
> +
> +	/* 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.
> +	 */
> +	pci_enable_obff(dev, type);
> +
>  	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>  	return 0;

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Monday, April 23, 2012 10:38 AM
> To: Hao, Xudong
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [PATCH] Re-enable LTR/OBFF when device is owned by pciback
> 
> On Sun, Apr 22, 2012 at 03:25:04PM +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.
> 
> Shouldn''t they be reset when the device is de-assigned from the
guest as well?
Yes, pci device will be reset when device is assigned to guest and de-assigned
from guest. In pci_reset_function(), the ltr/obff is saved and restored, so
nothing to consider the reset here.
However, hiding device with pciback did not call reset function, and device is
disabled, so we re-enable it here.

driver/pci/pci.c
    if (pcie_cap_has_devctl2(dev->pcie_type, flags))
        pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &cap[i++]);

> >
> > 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 19834d1..82aac5b 100644
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > @@ -334,6 +334,14 @@ static int __devinit pcistub_init_device(struct
> pci_dev *dev)
> >  	dev_dbg(&dev->dev, "reset device\n");
> >  	xen_pcibk_reset_device(dev);
> >
> > +	/* set default value */
> > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max
> > +value */
> 
> 
> Please put them in the appropiate location - at the start of the function.
> 
OK, I''ll modify in next version.
> > +
> > +	/* 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.
> > +	 */
> > +	pci_enable_ltr(dev);
> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> 
> OK, but didn''t you just come with up those values?
> 
The value should be set by driver, but now seems no real device support these
two feature, at least I don''t know such a device, and no driver call
ltr/obff function.
I just set the max value as default here, any suggestion?
> > +
> > +	/* 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.
> > +	 */
> > +	pci_enable_obff(dev, type);
> > +
> >  	dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >  	return 0;

>>> On 22.04.12 at 17:25, "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.
> 
> 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 19834d1..82aac5b 100644
> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -334,6 +334,14 @@ static int __devinit pcistub_init_device(struct
pci_dev
> *dev)
>  	dev_dbg(&dev->dev, "reset device\n");
>  	xen_pcibk_reset_device(dev);
>  
> +	/* set default value */
> +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max value */
> +
> +	/* 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. 
> +	 */
> +	pci_enable_ltr(dev);
> +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
Wouldn''t the guest kernel be able to do this itself, as it''s
just playing
with bits in the PCIE capability structure, or the LTR extended one?

If not, shouldn''t you properly report (or at least log) errors here?
> +
> +	/* 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.
> +	 */
> +	pci_enable_obff(dev, type);
Similarly here.

Jan
> +
>  	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 23.04.12 at 05:16, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> On Sun, Apr 22, 2012 at 03:25:04PM +0000, Hao, Xudong wrote:
>> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
>> 
>> OK, but didn''t you just come with up those values?
>> 
> 
> The value should be set by driver, but now seems no real device support 
> these two feature, at least I don''t know such a device, and no
driver call
> ltr/obff function. 
> I just set the max value as default here, any suggestion?
Matching my previous reply - why don''t you just let the guest do
this itself, should it so desire, thus allowing it to pick a suitable
value.

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, April 23, 2012 3:50 PM
> To: Hao, Xudong
> Cc: xen-devel; konrad.wilk@oracle.com
> Subject: Re: [Xen-devel] [PATCH] Re-enable LTR/OBFF when device is owned by
> pciback
> 
> >>> On 22.04.12 at 17:25, "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.
> >
> > 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 19834d1..82aac5b 100644
> > --- a/drivers/xen/xen-pciback/pci_stub.c
> > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > @@ -334,6 +334,14 @@ static int __devinit pcistub_init_device(struct
> > pci_dev
> > *dev)
> >  	dev_dbg(&dev->dev, "reset device\n");
> >  	xen_pcibk_reset_device(dev);
> >
> > +	/* set default value */
> > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max
> > +value */
> > +
> > +	/* 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.
> > +	 */
> > +	pci_enable_ltr(dev);
> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> 
> Wouldn''t the guest kernel be able to do this itself, as
it''s just playing with bits in
> the PCIE capability structure, or the LTR extended one?
> 
> If not, shouldn''t you properly report (or at least log) errors
here?
> 
There are two reasons I want to enable ltr/obff in host but not in guest.
1) ltr/obff is not only for one device, enabling them need to enable the whole
pci bus, include root port, upstream and downstream port. It''s complex
to let guest enable the whole path.
2) LTR/obff are PCIE device capabilities2, current qemu did not support exposing
PCIe capabilities2 to guest.

Yes, I''ll add error report log here.

Thanks,
-Xudong
> > +
> > +	/* 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.
> > +	 */
> > +	pci_enable_obff(dev, type);
> 
> Similarly here.
> 
> Jan
> 
> > +
> >  	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 24.04.12 at 04:49, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 22.04.12 at 17:25, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> > +	/* set default value */
>> > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
>> > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns is max
value */
>> > +
>> > +	/* 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.
>> > +	 */
>> > +	pci_enable_ltr(dev);
>> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
>> 
>> Wouldn''t the guest kernel be able to do this itself, as
it''s just playing
> with bits in
>> the PCIE capability structure, or the LTR extended one?
> 
> There are two reasons I want to enable ltr/obff in host but not in guest.
> 1) ltr/obff is not only for one device, enabling them need to enable the 
> whole pci bus, include root port, upstream and downstream port.
It''s complex
> to let guest enable the whole path.
Then it would still be more clean to extend the pciif, allowing the guest
to request enabling, or even better snoop the writes to the capabilities/
LTR structures in pciback.
> 2) LTR/obff are PCIE device capabilities2, current qemu did not support 
> exposing PCIe capabilities2 to guest.
Mostly the same here, except that adding support for these capabilities
is obviously going to be needed.

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, April 24, 2012 2:49 PM
> To: Hao, Xudong
> Cc: xen-devel; konrad.wilk@oracle.com
> Subject: RE: [Xen-devel] [PATCH] Re-enable LTR/OBFF when device is owned by
> pciback
> 
> >>> On 24.04.12 at 04:49, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> On 22.04.12 at 17:25, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
> >> > +	/* set default value */
> >> > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> >> > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /* 1024ns
is max
> value */
> >> > +
> >> > +	/* 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.
> >> > +	 */
> >> > +	pci_enable_ltr(dev);
> >> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> >>
> >> Wouldn''t the guest kernel be able to do this itself, as
it''s just playing
> > with bits in
> >> the PCIE capability structure, or the LTR extended one?
> >
> > There are two reasons I want to enable ltr/obff in host but not in
guest.
> > 1) ltr/obff is not only for one device, enabling them need to enable
the
> > whole pci bus, include root port, upstream and downstream port.
It''s
> complex
> > to let guest enable the whole path.
> 
> Then it would still be more clean to extend the pciif, allowing the guest
> to request enabling, or even better snoop the writes to the capabilities/
> LTR structures in pciback.
> 
Qemu only emulate the device which is assigned to guest, but it does not emulate
the physic device''s whole bus path. Seems allowing guest request
enabling looks reasonable, but I think it''s only applicable for
"per device feature(that unnecessary to enable the whole pci path)".
> > 2) LTR/obff are PCIE device capabilities2, current qemu did not
support
> > exposing PCIe capabilities2 to guest.
> 
> Mostly the same here, except that adding support for these capabilities
> is obviously going to be needed.
> 
> Jan

>>> On 25.04.12 at 04:46, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Tuesday, April 24, 2012 2:49 PM
>> To: Hao, Xudong
>> Cc: xen-devel; konrad.wilk@oracle.com 
>> Subject: RE: [Xen-devel] [PATCH] Re-enable LTR/OBFF when device is
owned by
>> pciback
>> 
>> >>> On 24.04.12 at 04:49, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> >>> On 22.04.12 at 17:25, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> >> > +	/* set default value */
>> >> > +	unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
>> >> > +	int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024; /*
1024ns is max
>> value */
>> >> > +
>> >> > +	/* 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.
>> >> > +	 */
>> >> > +	pci_enable_ltr(dev);
>> >> > +	pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
>> >>
>> >> Wouldn''t the guest kernel be able to do this itself,
as it''s just playing
>> > with bits in
>> >> the PCIE capability structure, or the LTR extended one?
>> >
>> > There are two reasons I want to enable ltr/obff in host but not in
guest.
>> > 1) ltr/obff is not only for one device, enabling them need to
enable the
>> > whole pci bus, include root port, upstream and downstream port.
It''s
>> complex
>> > to let guest enable the whole path.
>> 
>> Then it would still be more clean to extend the pciif, allowing the
guest
>> to request enabling, or even better snoop the writes to the
capabilities/
>> LTR structures in pciback.
>> 
> 
> Qemu only emulate the device which is assigned to guest, but it does not 
> emulate the physic device''s whole bus path. Seems allowing guest
request
> enabling looks reasonable, but I think it''s only applicable for
"per device
> feature(that unnecessary to enable the whole pci path)".  
No - if the guest asks the host to call pci_enable_ltr(), it''ll imply
enabling
for the entire path (as that''s what the call results in). Which imo is
better
than enabling a feature upfront, just in case.

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> > Qemu only emulate the device which is assigned to guest, but it does
not
> > emulate the physic device''s whole bus path. Seems allowing
guest request
> > enabling looks reasonable, but I think it''s only applicable
for "per device
> > feature(that unnecessary to enable the whole pci path)".
> 
> No - if the guest asks the host to call pci_enable_ltr(), it''ll
imply enabling
> for the entire path (as that''s what the call results in). Which
imo is better
> than enabling a feature upfront, just in case.
> 
For purpose of guest asking the host to call pci_enable_ltr(), there are two
probable ways:
a. According to PCIE spec, "software must not enable LTR in an Endpoint
unless the Root Complex and all intermediate Switches indicate support for
LTR". That means Qemu has to emulate the entire path of platform, so that
guest can detect whether endpoint device can be enabled firstly.
b. using pv guest mode, let the host call pci_enable_ltr().

Obviously, the two ways are not very good for this case. For the first way, we
can emulate the entire path when qemu support PCIE emulation. But currently
it''s better to enable ltr/obff in pciback driver.

Thanks,
-Xudong