Re: [RFC PATCH kernel] PCI: Enable access to custom VPD for Chelsio devices (cxgb3)

[Alexey Kardashevskiy]

On 07/09/16 04:30, Alexander Duyck wrote:
> On Tue, Sep 6, 2016 at 8:48 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>> Hi Alexey,
>>
>> On Thu, Aug 11, 2016 at 08:03:29PM +1000, Alexey Kardashevskiy wrote:
>>> There is at least one Chelsio 10Gb card which uses VPD area to store
>>> some custom blocks (example below). However pci_vpd_size() returns
>>> the length of the first block only assuming that there can be only
>>> one VPD "End Tag" and VFIO blocks access beyond that offset
>>> (since 4e1a63555) which leads to the situation when the guest "cxgb3"
>>> driver fails to probe the device. The host system does not have this
>>> problem as the drives accesses the config space directly without
>>> pci_read_vpd()/...
>>>
>>> This adds a quirk to override the VPD size to a bigger value.
>>>
>>> This is the controller:
>>> Ethernet controller [0200]: Chelsio Communications Inc T310 10GbE Single Port Adapter [1425:0030]
>>>
>>> This is its VPD:
>>> #0000 Large item 42 bytes; name 0x2 Identifier String
>>>       b'10 Gigabit Ethernet-SR PCI Express Adapter'
>>> #002d Large item 74 bytes; name 0x10
>>>       #00 [EC] len=7: b'D76809 '
>>>       #0a [FN] len=7: b'46K7897'
>>>       #14 [PN] len=7: b'46K7897'
>>>       #1e [MN] len=4: b'1037'
>>>       #25 [FC] len=4: b'5769'
>>>       #2c [SN] len=12: b'YL102035603V'
>>>       #3b [NA] len=12: b'00145E992ED1'
>>> #007a Small item 1 bytes; name 0xf End Tag
>>> ---
>>> #0c00 Large item 16 bytes; name 0x2 Identifier String
>>>       b'S310E-SR-X      '
>>> #0c13 Large item 234 bytes; name 0x10
>>>       #00 [PN] len=16: b'TBD             '
>>>       #13 [EC] len=16: b'110107730D2     '
>>>       #26 [SN] len=16: b'97YL102035603V  '
>>>       #39 [NA] len=12: b'00145E992ED1'
>>>       #48 [V0] len=6: b'175000'
>>>       #51 [V1] len=6: b'266666'
>>>       #5a [V2] len=6: b'266666'
>>>       #63 [V3] len=6: b'2000  '
>>>       #6c [V4] len=2: b'1 '
>>>       #71 [V5] len=6: b'c2    '
>>>       #7a [V6] len=6: b'0     '
>>>       #83 [V7] len=2: b'1 '
>>>       #88 [V8] len=2: b'0 '
>>>       #8d [V9] len=2: b'0 '
>>>       #92 [VA] len=2: b'0 '
>>>       #97 [RV] len=80: b's\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'...
>>> #0d00 Large item 252 bytes; name 0x11
>>>       #00 [VC] len=16: b'122310_1222 dp  '
>>>       #13 [VD] len=16: b'610-0001-00 H1\x00\x00'
>>>       #26 [VE] len=16: b'122310_1353 fp  '
>>>       #39 [VF] len=16: b'610-0001-00 H1\x00\x00'
>>>       #4c [RW] len=173: b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'...
>>> #0dff Small item 0 bytes; name 0xf End Tag
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
>>> ---
>>>  drivers/pci/quirks.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index ee72ebe1..94d3fb5 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3241,6 +3241,18 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
>>>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
>>>                       quirk_thunderbolt_hotplug_msi);
>>>
>>> +static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>>> +{
>>> +     if (!dev->vpd || !dev->vpd->ops || !dev->vpd->ops->set_size)
>>> +             return;
>>> +
>>> +     dev->vpd->ops->set_size(dev, max_t(unsigned int, dev->vpd->len, 0xe00));
>>> +}
>>> +
> 
> So one thing you might want to look at doing is actually validating
> there is something there before increasing the size.  If you look at
> the get_vpd_params function from the cxgb3 driver you will see what
> they do is verify the first tag located at 0xC00 is 0x82 before they
> do any further reads.  You might do something similar just to verify
> there is something there before you open it up to access by anyone.
> 
> One option would be to modify pci_vpd_size so that you can use it
> outside of access.c and can pass it an offset.  Then you could update
> your quirk so that you call pci_vpd_size and pass it the offset of
> 0xC00.  It should then be able to walk from that starting point and
> reach the end of the list.  If you do then pci_vpd_size will return
> the total size, else it returns 0.  So if size comes back as a
> non-zero value then you could pass that into pci_set_vpd_size,
> otherwise we can assume the starting offset is 0 and let the existing
> code run its course.


I could do this but I can predict endless argument whether a generic
pci_vpd_size() should scan for something which looks like VPD but it is
not, etc. I'd read at 0xc00, if it is 0x82, then pci_set_vpd_size(0xe00)
and that's it.


> 
>>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO,
>>> +                     PCI_ANY_ID,
>>> +                     quirk_chelsio_extend_vpd);
>>
>> Do you really want this for *all* Chelsio devices?  

Well, no. When I did this, I just wanted opinions :) I'll have a list here
in next revision.


> If you only need it for certain devices, the quirk could probably go in the driver.

It cannot, the cxgb3 driver is not running on the host, vfio-pci controls it
.

>
>>
>> Can you use pci_set_vpd_size() instead?  There's already a use of that
>> in cxgb4.
>>
> 
> I would assume this quirk needs to support the same device IDs as
> supported by the cxgb3 driver.  If so you might just clone the ID list
> from cxgb3_pci_tbl for this quirk.
> 
> Also from the looks of it the cxgb3 driver probably needs to be
> updated to use the VPD accessor functions instead of just open coding
> it themselves.  Otherwise we run the risk of the driver having issues
> if it attempts to access the EEPROM at the same time as other
> applications attempting to access the VPD for the device.


In this particular case cxgb3 driver is not running, vfio-pci controls the
device on the host side and does all the filtering so I have to add a quirk
to  drivers/pci/quirks.c with all 13 PCI IDs of cxgb3 hardware OOOOOR
implement quirks in vfio-pci (it has one already but I seriously doubt we
should extend it).

Fixing cxgb3 in the guest won't make any difference.

So where should I put the quirks? Thanks.


-- 
Alexey