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

[PATCH] 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.

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;

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

[Konrad Rzeszutek Wilk]

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;

Re: [PATCH] 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, 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;

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

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Hao, Xudong]

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

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

[Jan Beulich]

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

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

[Hao, Xudong]

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

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

[Jan Beulich]

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

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

[Hao, Xudong]

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