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