| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <403610A45A2B5242BD291EDAE8B37D300FDCB508@SHSMSX102.ccr.corp.intel.com>
Date: Wed, 30 May 2012 11:28:33 +0000
From: "Hao, Xudong" <xudong.hao@...el.com>
To: Bjorn Helgaas <bhelgaas@...gle.com>
CC: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"avi@...hat.com" <avi@...hat.com>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"Zhang, Xiantao" <xiantao.zhang@...el.com>
Subject: RE: [PATCH 1/1] Enable LTR/OBFF before device is used by driver
> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@...gle.com]
> Sent: Saturday, May 19, 2012 9:20 AM
> To: Xudong Hao
> Cc: linux-pci@...r.kernel.org; linux-kernel@...r.kernel.org;
> kvm@...r.kernel.org; avi@...hat.com; alex.williamson@...hat.com; Zhang,
> Xiantao; Hao, Xudong
> Subject: Re: [PATCH 1/1] Enable LTR/OBFF before device is used by driver
>
> On Sun, May 13, 2012 at 8:48 PM, Xudong Hao <xudong.hao@...ux.intel.com>
> wrote:
> > Enable LTR(Latency tolerance reporting) and OBFF(optimized buffer flush/fill)
> in
> > pci_enable_device(), so that they are enabled before the device is used by
> driver.
>
> Please split this into two patches (one for LTR and another for OBFF)
> so they can be reverted individually if they cause trouble.
OK.
> It would
> be nice if you bundled these together with your other "save/restore
> max Latency Value" patch so they were all together on the mailing
> list.
>
Sure, I'll modify the save/restore patch and bundle them together.
> I read the LTR sections of the PCIe spec, but I can't figure out how
> it's supposed to work. It says "power management policies ... can be
> implemented to consider Endpoint service requirements." Does that
> mean there's some OS software that might be involved, or is this just
> a matter of software flipping the LTR-enable bit and the hardware
> doing everything else? How confident can we be that enabling this is
> safe?
>
Software only set the LTR-enable bit, then hardware/chipset/device do everything else. There are one thing that software can be involved: software can configure maximum latency tolerance.
> For OBFF, is there some OS piece not included here that tells a Root
> Complex that "now is a good time for Endpoints to do something," e.g.,
> the spec mentions an "operating system timer tick." Is there some
> benefit to this patch without that piece? I don't understand the big
> picture yet.
>
As like LTR, OBFF do not need OS do additional changes, just set obff-enable bit.
> > Signed-off-by: Xudong Hao <xudong.hao@...el.com>
> >
> > ---
> > drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
> > 1 files changed, 29 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 111569c..2369883 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1134,6 +1134,31 @@ int pci_load_and_free_saved_state(struct
> pci_dev *dev,
> > }
> > EXPORT_SYMBOL_GPL(pci_load_and_free_saved_state);
> >
> > +static void pci_enable_dev_caps(struct pci_dev *dev)
> > +{
> > + /* set default value */
> > + unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
>
> There's only one use of this value, so skip the variable and just use
> PCI_EXP_OBFF_SIGNAL_ALWAYS in the call.
>
Ok.
> The comment at pci_enable_obff() says PCI_OBFF_SIGNAL_L0 is the
> preferred type, so please explain why you're not using that.
>
Yes, here it's better to set PCI_OBFF_SIGNAL_L0 by default.
> > +
> > + /* LTR(Latency tolerance reporting) allows devices to send
> > + * messages to the root complex indicating their latency
> > + * tolerance for snooped & unsnooped memory transactions.
> > + */
>
> Follow Linux comment style, i.e.,
>
> /*
> * LTR ...
> */
>
Will modify, Thanks.
> > + pci_enable_ltr(dev);
> > +
> > + /* 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);
> > +}
> > +
> > +static void pci_disable_dev_caps(struct pci_dev *dev)
> > +{
> > + pci_disable_obff(dev);
> > + pci_disable_ltr(dev);
> > +}
> > +
> > static int do_pci_enable_device(struct pci_dev *dev, int bars)
> > {
> > int err;
> > @@ -1146,6 +1171,9 @@ static int do_pci_enable_device(struct pci_dev
> *dev, int bars)
> > return err;
> > pci_fixup_device(pci_fixup_enable, dev);
> >
> > + /* Enable some device capibility before it's used by driver. */
> > + pci_enable_dev_caps(dev);
>
> Why is this here? It seems similar to what's already in
> pci_init_capabilities(). Is there a reason to do this in the
> pci_enable_device() path rather than in the pci_device_add() path?
>
pci_enable_device is called by any pci driver including kvm driver, Considering such a case in kvm, when device is assigned to guest(the device will be reset), we want not host lose those advanced PM feature, so add it in pci_enable_device so that kvm driver call it.
> > +
> > return 0;
> > }
> >
> > @@ -1361,6 +1389,7 @@ static void do_pci_disable_device(struct pci_dev
> *dev)
> > }
> >
> > pcibios_disable_device(dev);
> > + pci_disable_dev_caps(dev);
> > }
> >
> > /**
> > --
> > 1.6.0.rc1
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists