linux-kernel - RE: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Aug 2012 01:00:40 +0000
From:	"Cui, Dexuan" <dexuan.cui@...el.com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
CC:	Jiang Liu <liuj97@...il.com>, Don Dutile <ddutile@...hat.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Taku Izumi <izumi.taku@...fujitsu.com>,
	"Rafael J . Wysocki" <rjw@...k.pl>,
	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
	Yijing Wang <wangyijing@...wei.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: RE: [PATCH v3 00/32] provide interfaces to access PCIe capabilities
 registers

Bjorn Helgaas wrote on 2012-08-23:
> On Mon, Aug 20, 2012 at 9:40 PM, Cui, Dexuan <dexuan.cui@...el.com>
> wrote:
>> Bjorn Helgaas wrote on 2012-08-21:
> 
>>> I am still concerned about reset_intel_82599_sfp_virtfn().  It looks
>>> wrong and possibly unnecessary.  It looks wrong because it sets
>>> PCI_EXP_DEVCTL_BCR_FLR and blindly clears all other bits in
>>> PCI_EXP_DEVCTL.  Most of the bits are probably cleared by the FLR
>>> anyway, but Aux Power PM Enable is RWS ("sticky"), so it is *not*
>>> modified by FLR.  Therefore, using reset_intel_82599_sfp_virtfn() has
>>> the probably unintended side effect of clearing Aux Power PM Enable.
>>> 
>>> It looks possibly unnecessary because the generic pcie_flr() does
>>> essentially the same thing, so it's not clear why we need a special
>>> case for 82599.
> 
>> I think reset_intel_82599_sfp_virtfn() is correct AND necessary.
>> (pcie_flr() doesn't work here)
>> 
>> Please note the 82599 VF is a special PCIe device that doesn't report
>> PCIe FLR capability though actually it does support that.
>> That is why we put it in quirks.c. :-)
>> 
>> The PCI_EXP_DEVCTL_AUX_PME bit of the 82599 VF is read-only and
>> always zero.
>> 
>> Please see
>> 
> http://www.intel.com/content/dam/doc/datasheet/82599-10-gbe-controller-
> datasheet.pdf
>> 9.5 Virtual Functions Configuration Space
>>   Table 9.7 VF PCIe Configuration Space
>>   9.5.2.2.1 VF Device Control Register (0xA8; RW)
> 
> Thanks, Dexuan!
> 
> The 82599 does report FLR in the DEVCAP for the PF (sec 9.3.10.4), but
> not in the DEVCAP for the VF (sec 9.5), which indeed means we can't
> use pcie_flr() on the VFs.  I wonder whether this error appears in any
> other devices.
> 
> The VF DEVCTL register (sec 9.5.2.2.1) is RO and zero except for
> "Initiate FLR" unlike the PF DEVCTL (sec 9.3.10.5).  The
> read/modify/write done by pcie_flr() would work on the VF but is not
> necessary.
> 
> The VF DEVSTA register (sec 9.5.2.2.2) does have an active
> "Transaction Pending" bit.  That suggests to me that we should wait
> for it to be clear, as pcie_flr() does.
I agree.

> 
> What would you think of a patch like the following?  My idea is to
> make it the same as pcie_flr() except for the absolutely necessary
> differences.  With this patch, the only difference is that we don't
> look at the 82599 DEVCAP FLR bit.
Hi Bjorn,
Thanks for the patch!
It seems good to me (BTW, I don't have such a device at
hand to test)

Thanks,
-- Dexuan

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