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

["Hao, Xudong" <xudong.hao@xxxxxxxxx>]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> Sent: Monday, April 23, 2012 10:38 AM
> To: Hao, Xudong
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> >
> > 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;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel