| Message ID | 1470909809-11007-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | Changes Requested |
| Headers | show |
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@ozlabs.ru> > --- > 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)); > +} > + > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, > + PCI_ANY_ID, > + quirk_chelsio_extend_vpd); Do you really want this for *all* Chelsio devices? If you only need it for certain devices, the quirk could probably go in the driver. Can you use pci_set_vpd_size() instead? There's already a use of that in cxgb4. > + > #ifdef CONFIG_ACPI > /* > * Apple: Shutdown Cactus Ridge Thunderbolt controller. > -- > 2.5.0.rc3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 6, 2016 at 8:48 AM, Bjorn Helgaas <helgaas@kernel.org> 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@ozlabs.ru> >> --- >> 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. >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, >> + PCI_ANY_ID, >> + quirk_chelsio_extend_vpd); > > Do you really want this for *all* Chelsio devices? If you only need > it for certain devices, the quirk could probably go in the driver. > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/16 04:30, Alexander Duyck wrote: > On Tue, Sep 6, 2016 at 8:48 AM, Bjorn Helgaas <helgaas@kernel.org> 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@ozlabs.ru> >>> --- >>> 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.
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)); +} + +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, + PCI_ANY_ID, + quirk_chelsio_extend_vpd); + #ifdef CONFIG_ACPI /* * Apple: Shutdown Cactus Ridge Thunderbolt controller.
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@ozlabs.ru> --- drivers/pci/quirks.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)