On 08/09/2016 08:12 PM, Alexander Duyck wrote: > On Tue, Aug 9, 2016 at 5:54 AM, Alexey Kardashevskiy <aik@xxxxxxxxx> wrote: >> On 10/02/16 08:04, Bjorn Helgaas wrote: >>> On Wed, Jan 13, 2016 at 12:25:34PM +0100, Hannes Reinecke wrote: >>>> PCI-2.2 VPD entries have a maximum size of 32k, but might actually >>>> be smaller than that. To figure out the actual size one has to read >>>> the VPD area until the 'end marker' is reached. >>>> Trying to read VPD data beyond that marker results in 'interesting' >>>> effects, from simple read errors to crashing the card. And to make >>>> matters worse not every PCI card implements this properly, leaving >>>> us with no 'end' marker or even completely invalid data. >>>> This path tries to determine the size of the VPD data. >>>> If no valid data can be read an I/O error will be returned when >>>> reading the sysfs attribute. >> >> >> I have a problem with this particular feature as today VFIO uses this >> pci_vpd_xxxx API to virtualize access to VPD and the existing code assumes >> there is just one VPD block with 0x2 start and 0xf end. However I have at >> least one device where this is not true - "10 Gigabit Ethernet-SR PCI >> Express Adapter" - it has 2 blocks (made a script to read/parse it as >> /sys/bus/pci/devices/0001\:03\:00.0/vpd shows it wrong): > > The PCI spec is what essentially assumes that there is only one block. > If I am not mistaken in the case of this device the second block here > actually contains device configuration data, not actual VPD data. The > issue here is that the second block is being accessed as VPD when it > isn't. > >> #0000 Large item 42 bytes; name 0x2 Identifier String >> #002d Large item 74 bytes; name 0x10 >> #007a Small item 1 bytes; name 0xf End Tag >> --- >> #0c00 Large item 16 bytes; name 0x2 Identifier String >> #0c13 Large item 234 bytes; name 0x10 >> #0d00 Large item 252 bytes; name 0x11 >> #0dff Small item 0 bytes; name 0xf End Tag > > The second block here is driver proprietary setup bits. > >> The cxgb3 driver is reading the second bit starting from 0xc00 but since >> the size is wrongly detected as 0x7c, VFIO blocks access beyond it and the >> guest driver fails to probe. >> >> I also cannot find a clause in the PCI 3.0 spec saying that there must be >> just a single block, is it there? > > The problem is we need to be able to parse it. The spec defines a > series of tags that can be used starting at offset 0. That is how we > are supposed to get around through the VPD data. The problem is we > can't have more than one end tag and what appears to be happening here > is that we are defining a second block of data which uses the same > formatting as VPD but is not VPD. > >> What would the correct fix be? Scanning all 32k of VPD is not an option I >> suppose as this is what this patch is trying to avoid. Thanks. > > I adding the current cxgb3 maintainer and netdev list to the Cc. This > is something that can probably be addressed via a PCI quirk as what > needs to happen is that we need to extend the VPD in the case of this > part in order to include this second block. As long as we can read > the VPD data all the way out to 0xdff odds are we could probably just > have the size arbitrarily increased to 0xe00 via the quirk and then > you would be able to access all of the VPD for the device. We already > have code making other modifications to drivers/pci/quirks.c for > several Broadcom devices and probably just need something similar to > allow extended access in the case of these devices. > Yes, that's what I think, too. The Broadcom quirk should work here, too. (Didn't we do that already?) Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@xxxxxxx +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
