* [PATCH] PCI: Add cavium acs pci quirk @ 2016-01-29 20:03 Manish Jaggi 2016-02-04 22:44 ` Bjorn Helgaas 2017-02-14 4:44 ` Alex Williamson 0 siblings, 2 replies; 9+ messages in thread From: Manish Jaggi @ 2016-01-29 20:03 UTC (permalink / raw) To: linux-kernel, linux-pci; +Cc: Tirumalesh Chalamarla, Richter, Robert Cavium devices matching this quirk do not perform peer-to-peer with other functions, allowing masking out these bits as if they were unimplemented in the ACS capability. Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> --- drivers/pci/quirks.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 7e32730..a300fa6 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) #endif } +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) +{ + /* + * Cavium devices matching this quirk do not perform + * peer-to-peer with other functions, allowing masking out + * these bits as if they were unimplemented in the ACS capability. + */ + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); + + return acs_flags ? 0 : 1; +} + /* * Many Intel PCH root ports do provide ACS-like features to disable peer * transactions and validate bus numbers in requests, but do not provide an @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ + /* Cavium ThunderX */ + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, { 0 } }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2016-01-29 20:03 [PATCH] PCI: Add cavium acs pci quirk Manish Jaggi @ 2016-02-04 22:44 ` Bjorn Helgaas 2017-02-14 4:44 ` Alex Williamson 1 sibling, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-02-04 22:44 UTC (permalink / raw) To: Manish Jaggi Cc: linux-kernel, linux-pci, Tirumalesh Chalamarla, Richter, Robert On Sat, Jan 30, 2016 at 01:33:58AM +0530, Manish Jaggi wrote: > > Cavium devices matching this quirk do not perform > peer-to-peer with other functions, allowing masking out > these bits as if they were unimplemented in the ACS capability. > > Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> > Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> Applied as follows to pci/virtualization for v4.6, thanks, Manish! Note that this quirk applies to *all* Cavium devices. I assume that's what you want; I only mention it because your comment says "Cavium devices matching this quirk ...", which is all of them because the code says PCI_ANY_ID. Bjorn commit b404bcfbf035413dcce539c8ba2c9986d220d8ed Author: Manish Jaggi <mjaggi@caviumnetworks.com> Date: Sat Jan 30 01:33:58 2016 +0530 PCI: Add ACS quirk for all Cavium devices Cavium devices matching this quirk do not perform peer-to-peer with other functions, allowing masking out these bits as if they were unimplemented in the ACS capability. Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 0575a1e..85fa6a2a 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3832,6 +3832,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) #endif } +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) +{ + /* + * Cavium devices matching this quirk do not perform peer-to-peer + * with other functions, allowing masking out these bits as if they + * were unimplemented in the ACS capability. + */ + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); + + return acs_flags ? 0 : 1; +} + /* * Many Intel PCH root ports do provide ACS-like features to disable peer * transactions and validate bus numbers in requests, but do not provide an @@ -3984,6 +3997,8 @@ static const struct pci_dev_acs_enabled { { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ + /* Cavium ThunderX */ + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, { 0 } }; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2016-01-29 20:03 [PATCH] PCI: Add cavium acs pci quirk Manish Jaggi 2016-02-04 22:44 ` Bjorn Helgaas @ 2017-02-14 4:44 ` Alex Williamson 2017-02-14 15:07 ` Bjorn Helgaas 2017-03-05 7:07 ` Sunil Kovvuri 1 sibling, 2 replies; 9+ messages in thread From: Alex Williamson @ 2017-02-14 4:44 UTC (permalink / raw) To: Manish Jaggi Cc: linux-kernel, linux-pci, Tirumalesh Chalamarla, Richter, Robert, Bjorn Helgaas On Sat, 30 Jan 2016 01:33:58 +0530 Manish Jaggi <mjaggi@caviumnetworks.com> wrote: > Cavium devices matching this quirk do not perform > peer-to-peer with other functions, allowing masking out > these bits as if they were unimplemented in the ACS capability. > > Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> > Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> > --- > drivers/pci/quirks.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 7e32730..a300fa6 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > #endif > } > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > +{ > + /* > + * Cavium devices matching this quirk do not perform > + * peer-to-peer with other functions, allowing masking out > + * these bits as if they were unimplemented in the ACS capability. > + */ > + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > + > + return acs_flags ? 0 : 1; > +} > + > /* > * Many Intel PCH root ports do provide ACS-like features to disable peer > * transactions and validate bus numbers in requests, but do not provide an > @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { > { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, > { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ > { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ > + /* Cavium ThunderX */ > + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, > { 0 } > }; > Apologies for not catching this, but what sort of crystal ball do you have that can predict not only current devices, but future devices will not support peer-to-peer features? Is there an internal design guidelines reference specification for Cavium that we can realistically expect this to remain consistent, or is this just an attempt to never think about ACS again at the customer's peril? What about the existing non-ThunderX products with Cavium vendor ID, does this really apply to those? I would strongly suggest taking the device ID into account. See examples like the pci_quirk_intel_pch_acs quirk where the initial filter is PCI_ANY_ID, but specific device types and ranges of device IDs are identified by the function for evaluation. This seems reckless to me and I'd advise that it be reverted. Thanks, Alex ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-02-14 4:44 ` Alex Williamson @ 2017-02-14 15:07 ` Bjorn Helgaas 2017-02-27 19:02 ` David Daney 2017-03-05 7:07 ` Sunil Kovvuri 1 sibling, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2017-02-14 15:07 UTC (permalink / raw) To: Alex Williamson Cc: Manish Jaggi, linux-kernel, linux-pci, Tirumalesh Chalamarla, Richter, Robert, Bjorn Helgaas On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote: > On Sat, 30 Jan 2016 01:33:58 +0530 > Manish Jaggi <mjaggi@caviumnetworks.com> wrote: > > > Cavium devices matching this quirk do not perform > > peer-to-peer with other functions, allowing masking out > > these bits as if they were unimplemented in the ACS capability. > > > > Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> > > Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> > > --- > > drivers/pci/quirks.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 7e32730..a300fa6 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > > #endif > > } > > > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > > +{ > > + /* > > + * Cavium devices matching this quirk do not perform > > + * peer-to-peer with other functions, allowing masking out > > + * these bits as if they were unimplemented in the ACS capability. > > + */ > > + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > > + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > > + > > + return acs_flags ? 0 : 1; > > +} > > + > > /* > > * Many Intel PCH root ports do provide ACS-like features to disable peer > > * transactions and validate bus numbers in requests, but do not provide an > > @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { > > { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, > > { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ > > { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ > > + /* Cavium ThunderX */ > > + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, > > { 0 } > > }; > > > > Apologies for not catching this, but what sort of crystal ball do you > have that can predict not only current devices, but future devices will > not support peer-to-peer features? Is there an internal design > guidelines reference specification for Cavium that we can realistically > expect this to remain consistent, or is this just an attempt to never > think about ACS again at the customer's peril? What about the existing > non-ThunderX products with Cavium vendor ID, does this really apply to > those? I would strongly suggest taking the device ID into account. > See examples like the pci_quirk_intel_pch_acs quirk where the initial > filter is PCI_ANY_ID, but specific device types and ranges of device > IDs are identified by the function for evaluation. This seems reckless > to me and I'd advise that it be reverted. Thanks, I'd be happy to revert this, but it would be easier if somebody sent a patch and a changelog. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-02-14 15:07 ` Bjorn Helgaas @ 2017-02-27 19:02 ` David Daney 2017-02-27 19:13 ` Chalamarla, Tirumalesh 0 siblings, 1 reply; 9+ messages in thread From: David Daney @ 2017-02-27 19:02 UTC (permalink / raw) To: Bjorn Helgaas, Alex Williamson, Manish Jaggi, Tirumalesh Chalamarla Cc: linux-kernel, linux-pci, Richter, Robert, Bjorn Helgaas On 02/14/2017 07:07 AM, Bjorn Helgaas wrote: > On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote: >> On Sat, 30 Jan 2016 01:33:58 +0530 >> Manish Jaggi <mjaggi@caviumnetworks.com> wrote: >> >>> Cavium devices matching this quirk do not perform >>> peer-to-peer with other functions, allowing masking out >>> these bits as if they were unimplemented in the ACS capability. >>> >>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> >>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> >>> --- >>> drivers/pci/quirks.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >>> index 7e32730..a300fa6 100644 >>> --- a/drivers/pci/quirks.c >>> +++ b/drivers/pci/quirks.c >>> @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) >>> #endif >>> } >>> >>> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) >>> +{ >>> + /* >>> + * Cavium devices matching this quirk do not perform >>> + * peer-to-peer with other functions, allowing masking out >>> + * these bits as if they were unimplemented in the ACS capability. >>> + */ >>> + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | >>> + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); >>> + >>> + return acs_flags ? 0 : 1; >>> +} >>> + >>> /* >>> * Many Intel PCH root ports do provide ACS-like features to disable peer >>> * transactions and validate bus numbers in requests, but do not provide an >>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { >>> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, >>> { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ >>> { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ >>> + /* Cavium ThunderX */ >>> + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, >>> { 0 } >>> }; >>> >> >> Apologies for not catching this, but what sort of crystal ball do you >> have that can predict not only current devices, but future devices will >> not support peer-to-peer features? Is there an internal design >> guidelines reference specification for Cavium that we can realistically >> expect this to remain consistent, or is this just an attempt to never >> think about ACS again at the customer's peril? What about the existing >> non-ThunderX products with Cavium vendor ID, does this really apply to >> those? I would strongly suggest taking the device ID into account. >> See examples like the pci_quirk_intel_pch_acs quirk where the initial >> filter is PCI_ANY_ID, but specific device types and ranges of device >> IDs are identified by the function for evaluation. This seems reckless >> to me and I'd advise that it be reverted. Thanks, > > I'd be happy to revert this, but it would be easier if somebody sent a > patch and a changelog. > I agree that it should be reverted. I was hoping that Manish or Tirumalesh would fix this properly by only activating the quirk on the faulty hardware, but such a fix has never appeared. David Daney ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-02-27 19:02 ` David Daney @ 2017-02-27 19:13 ` Chalamarla, Tirumalesh 2017-02-27 19:25 ` David Daney 0 siblings, 1 reply; 9+ messages in thread From: Chalamarla, Tirumalesh @ 2017-02-27 19:13 UTC (permalink / raw) To: David Daney, Bjorn Helgaas, Alex Williamson, Jaggi, Manish Cc: linux-kernel, linux-pci, Richter, Robert, Bjorn Helgaas DQoNCk9uIDIvMjcvMTcsIDExOjAyIEFNLCAiRGF2aWQgRGFuZXkiIDxkZGFuZXkuY2F2bUBnbWFp bC5jb20+IHdyb3RlOg0KDQogICAgT24gMDIvMTQvMjAxNyAwNzowNyBBTSwgQmpvcm4gSGVsZ2Fh cyB3cm90ZToNCiAgICA+IE9uIE1vbiwgRmViIDEzLCAyMDE3IGF0IDA5OjQ0OjU3UE0gLTA3MDAs IEFsZXggV2lsbGlhbXNvbiB3cm90ZToNCiAgICA+PiBPbiBTYXQsIDMwIEphbiAyMDE2IDAxOjMz OjU4ICswNTMwDQogICAgPj4gTWFuaXNoIEphZ2dpIDxtamFnZ2lAY2F2aXVtbmV0d29ya3MuY29t PiB3cm90ZToNCiAgICA+Pg0KICAgID4+PiBDYXZpdW0gZGV2aWNlcyBtYXRjaGluZyB0aGlzIHF1 aXJrIGRvIG5vdCBwZXJmb3JtDQogICAgPj4+IHBlZXItdG8tcGVlciB3aXRoIG90aGVyIGZ1bmN0 aW9ucywgYWxsb3dpbmcgbWFza2luZyBvdXQNCiAgICA+Pj4gdGhlc2UgYml0cyBhcyBpZiB0aGV5 IHdlcmUgdW5pbXBsZW1lbnRlZCBpbiB0aGUgQUNTIGNhcGFiaWxpdHkuDQogICAgPj4+DQogICAg Pj4+IEFja2VkLWJ5OiBUaXJ1bWFsZXNoIENoYWxhbWFybGEgPHRjaGFsYW1hcmxhQGNhdml1bS5j b20+DQogICAgPj4+IFNpZ25lZC1vZmYtYnk6IE1hbmlzaCBKYWdnaSA8bWphZ2dpQGNhdml1bW5l dHdvcmtzLmNvbT4NCiAgICA+Pj4gLS0tDQogICAgPj4+ICBkcml2ZXJzL3BjaS9xdWlya3MuYyB8 IDE1ICsrKysrKysrKysrKysrKw0KICAgID4+PiAgMSBmaWxlIGNoYW5nZWQsIDE1IGluc2VydGlv bnMoKykNCiAgICA+Pj4NCiAgICA+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL3F1aXJrcy5j IGIvZHJpdmVycy9wY2kvcXVpcmtzLmMNCiAgICA+Pj4gaW5kZXggN2UzMjczMC4uYTMwMGZhNiAx MDA2NDQNCiAgICA+Pj4gLS0tIGEvZHJpdmVycy9wY2kvcXVpcmtzLmMNCiAgICA+Pj4gKysrIGIv ZHJpdmVycy9wY2kvcXVpcmtzLmMNCiAgICA+Pj4gQEAgLTM4MTQsNiArMzgxNCwxOSBAQCBzdGF0 aWMgaW50IHBjaV9xdWlya19hbWRfc2JfYWNzKHN0cnVjdCBwY2lfZGV2ICpkZXYsIHUxNiBhY3Nf ZmxhZ3MpDQogICAgPj4+ICAjZW5kaWYNCiAgICA+Pj4gIH0NCiAgICA+Pj4NCiAgICA+Pj4gK3N0 YXRpYyBpbnQgcGNpX3F1aXJrX2Nhdml1bV9hY3Moc3RydWN0IHBjaV9kZXYgKmRldiwgdTE2IGFj c19mbGFncykNCiAgICA+Pj4gK3sNCiAgICA+Pj4gKwkvKg0KICAgID4+PiArCSAqIENhdml1bSBk ZXZpY2VzIG1hdGNoaW5nIHRoaXMgcXVpcmsgZG8gbm90IHBlcmZvcm0NCiAgICA+Pj4gKwkgKiBw ZWVyLXRvLXBlZXIgd2l0aCBvdGhlciBmdW5jdGlvbnMsIGFsbG93aW5nIG1hc2tpbmcgb3V0DQog ICAgPj4+ICsJICogdGhlc2UgYml0cyBhcyBpZiB0aGV5IHdlcmUgdW5pbXBsZW1lbnRlZCBpbiB0 aGUgQUNTIGNhcGFiaWxpdHkuDQogICAgPj4+ICsJICovDQogICAgPj4+ICsJYWNzX2ZsYWdzICY9 IH4oUENJX0FDU19TViB8IFBDSV9BQ1NfVEIgfCBQQ0lfQUNTX1JSIHwNCiAgICA+Pj4gKwkJICAg ICAgIFBDSV9BQ1NfQ1IgfCBQQ0lfQUNTX1VGIHwgUENJX0FDU19EVCk7DQogICAgPj4+ICsNCiAg ICA+Pj4gKwlyZXR1cm4gYWNzX2ZsYWdzID8gMCA6IDE7DQogICAgPj4+ICt9DQogICAgPj4+ICsN CiAgICA+Pj4gIC8qDQogICAgPj4+ICAgKiBNYW55IEludGVsIFBDSCByb290IHBvcnRzIGRvIHBy b3ZpZGUgQUNTLWxpa2UgZmVhdHVyZXMgdG8gZGlzYWJsZSBwZWVyDQogICAgPj4+ICAgKiB0cmFu c2FjdGlvbnMgYW5kIHZhbGlkYXRlIGJ1cyBudW1iZXJzIGluIHJlcXVlc3RzLCBidXQgZG8gbm90 IHByb3ZpZGUgYW4NCiAgICA+Pj4gQEAgLTM5NjYsNiArMzk3OSw4IEBAIHN0YXRpYyBjb25zdCBz dHJ1Y3QgcGNpX2Rldl9hY3NfZW5hYmxlZCB7DQogICAgPj4+ICAJeyBQQ0lfVkVORE9SX0lEX0lO VEVMLCBQQ0lfQU5ZX0lELCBwY2lfcXVpcmtfaW50ZWxfcGNoX2FjcyB9LA0KICAgID4+PiAgCXsg MHgxOWEyLCAweDcxMCwgcGNpX3F1aXJrX21mX2VuZHBvaW50X2FjcyB9LCAvKiBFbXVsZXggQkUz LVIgKi8NCiAgICA+Pj4gIAl7IDB4MTBkZiwgMHg3MjAsIHBjaV9xdWlya19tZl9lbmRwb2ludF9h Y3MgfSwgLyogRW11bGV4IFNreWhhd2stUiAqLw0KICAgID4+PiArCS8qIENhdml1bSBUaHVuZGVy WCAqLw0KICAgID4+PiArCXsgUENJX1ZFTkRPUl9JRF9DQVZJVU0sIFBDSV9BTllfSUQsIHBjaV9x dWlya19jYXZpdW1fYWNzIH0sDQogICAgPj4+ICAJeyAwIH0NCiAgICA+Pj4gIH07DQogICAgPj4+ DQogICAgPj4NCiAgICA+PiBBcG9sb2dpZXMgZm9yIG5vdCBjYXRjaGluZyB0aGlzLCBidXQgd2hh dCBzb3J0IG9mIGNyeXN0YWwgYmFsbCBkbyB5b3UNCiAgICA+PiBoYXZlIHRoYXQgY2FuIHByZWRp Y3Qgbm90IG9ubHkgY3VycmVudCBkZXZpY2VzLCBidXQgZnV0dXJlIGRldmljZXMgd2lsbA0KICAg ID4+IG5vdCBzdXBwb3J0IHBlZXItdG8tcGVlciBmZWF0dXJlcz8gIElzIHRoZXJlIGFuIGludGVy bmFsIGRlc2lnbg0KICAgID4+IGd1aWRlbGluZXMgcmVmZXJlbmNlIHNwZWNpZmljYXRpb24gZm9y IENhdml1bSB0aGF0IHdlIGNhbiByZWFsaXN0aWNhbGx5DQogICAgPj4gZXhwZWN0IHRoaXMgdG8g cmVtYWluIGNvbnNpc3RlbnQsIG9yIGlzIHRoaXMganVzdCBhbiBhdHRlbXB0IHRvIG5ldmVyDQog ICAgPj4gdGhpbmsgYWJvdXQgQUNTIGFnYWluIGF0IHRoZSBjdXN0b21lcidzIHBlcmlsPyAgV2hh dCBhYm91dCB0aGUgZXhpc3RpbmcNCiAgICA+PiBub24tVGh1bmRlclggcHJvZHVjdHMgd2l0aCBD YXZpdW0gdmVuZG9yIElELCBkb2VzIHRoaXMgcmVhbGx5IGFwcGx5IHRvDQogICAgPj4gdGhvc2U/ ICBJIHdvdWxkIHN0cm9uZ2x5IHN1Z2dlc3QgdGFraW5nIHRoZSBkZXZpY2UgSUQgaW50byBhY2Nv dW50Lg0KICAgID4+IFNlZSBleGFtcGxlcyBsaWtlIHRoZSBwY2lfcXVpcmtfaW50ZWxfcGNoX2Fj cyBxdWlyayB3aGVyZSB0aGUgaW5pdGlhbA0KICAgID4+IGZpbHRlciBpcyBQQ0lfQU5ZX0lELCBi dXQgc3BlY2lmaWMgZGV2aWNlIHR5cGVzIGFuZCByYW5nZXMgb2YgZGV2aWNlDQogICAgPj4gSURz IGFyZSBpZGVudGlmaWVkIGJ5IHRoZSBmdW5jdGlvbiBmb3IgZXZhbHVhdGlvbi4gIFRoaXMgc2Vl bXMgcmVja2xlc3MNCiAgICA+PiB0byBtZSBhbmQgSSdkIGFkdmlzZSB0aGF0IGl0IGJlIHJldmVy dGVkLiAgVGhhbmtzLA0KICAgID4NCiAgICA+IEknZCBiZSBoYXBweSB0byByZXZlcnQgdGhpcywg YnV0IGl0IHdvdWxkIGJlIGVhc2llciBpZiBzb21lYm9keSBzZW50IGENCiAgICA+IHBhdGNoIGFu ZCBhIGNoYW5nZWxvZy4NCiAgICA+DQogICAgDQogICAgSSBhZ3JlZSB0aGF0IGl0IHNob3VsZCBi ZSByZXZlcnRlZC4NCiAgICANCiAgICBJIHdhcyBob3BpbmcgdGhhdCBNYW5pc2ggb3IgVGlydW1h bGVzaCB3b3VsZCBmaXggdGhpcyBwcm9wZXJseSBieSBvbmx5IA0KICAgIGFjdGl2YXRpbmcgdGhl IHF1aXJrIG9uIHRoZSBmYXVsdHkgaGFyZHdhcmUsIGJ1dCBzdWNoIGEgZml4IGhhcyBuZXZlciAN CiAgICBhcHBlYXJlZC4NCg0KVGhpcyB3YXMgc3VwcG9zZWQgdG8gYmUgdHJ1ZSBmb3IgYWxsIENh dml1bSBjaGlwcy4gV2UgZ290IHN1Y2ggYW5zd2VyIGZyb20gSGFyZHdhcmUgYXJjaGl0ZWN0cywg YnV0IGl0IG1pZ2h0IGNoYW5nZSBpbiB0aGUgZnV0dXJlLiANCkkgd2lsbCBiZSBoYXBweSB0byBy ZXBsYWNlIHRoaXMgd2l0aCBERVZJQ0VfSUQuDQpNYW5pc2gsDQpDb3VsZCB5b3UgcGxlYXNlIHRh a2UgdGhpcyB1LCBpZiBub3QgSSB3aWxsIHNlbmQgYSBwYXRjaCB0byBkbyB0aGlzLg0KDQpUaXJ1 bWFsZXNoLiANCiAgICANCiAgICBEYXZpZCBEYW5leQ0KICAgIA0KICAgIA0KICAgIA0KICAgIA0K DQo= ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-02-27 19:13 ` Chalamarla, Tirumalesh @ 2017-02-27 19:25 ` David Daney 0 siblings, 0 replies; 9+ messages in thread From: David Daney @ 2017-02-27 19:25 UTC (permalink / raw) To: Chalamarla, Tirumalesh, Bjorn Helgaas, Alex Williamson, Jaggi, Manish Cc: linux-kernel, linux-pci, Richter, Robert, Bjorn Helgaas On 02/27/2017 11:13 AM, Chalamarla, Tirumalesh wrote: > > > On 2/27/17, 11:02 AM, "David Daney" <ddaney.cavm@gmail.com> wrote: > > On 02/14/2017 07:07 AM, Bjorn Helgaas wrote: > > On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote: > >> On Sat, 30 Jan 2016 01:33:58 +0530 > >> Manish Jaggi <mjaggi@caviumnetworks.com> wrote: > >> > >>> Cavium devices matching this quirk do not perform > >>> peer-to-peer with other functions, allowing masking out > >>> these bits as if they were unimplemented in the ACS capability. > >>> > >>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> > >>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> > >>> --- > >>> drivers/pci/quirks.c | 15 +++++++++++++++ > >>> 1 file changed, 15 insertions(+) > >>> > >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > >>> index 7e32730..a300fa6 100644 > >>> --- a/drivers/pci/quirks.c > >>> +++ b/drivers/pci/quirks.c > >>> @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > >>> #endif > >>> } > >>> > >>> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > >>> +{ Perhaps add a check here for dev->device and/or dev->subsystem_device to see if we need the workaround. There may be many that match, so ... > >>> + /* > >>> + * Cavium devices matching this quirk do not perform > >>> + * peer-to-peer with other functions, allowing masking out > >>> + * these bits as if they were unimplemented in the ACS capability. > >>> + */ > >>> + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > >>> + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > >>> + > >>> + return acs_flags ? 0 : 1; > >>> +} > >>> + > >>> /* > >>> * Many Intel PCH root ports do provide ACS-like features to disable peer > >>> * transactions and validate bus numbers in requests, but do not provide an > >>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { > >>> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, > >>> { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ > >>> { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ > >>> + /* Cavium ThunderX */ > >>> + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, .. Leaving PCI_ANY_ID here may keep the table more manageable. > >>> { 0 } > >>> }; > >>> > >> > >> Apologies for not catching this, but what sort of crystal ball do you > >> have that can predict not only current devices, but future devices will > >> not support peer-to-peer features? Is there an internal design > >> guidelines reference specification for Cavium that we can realistically > >> expect this to remain consistent, or is this just an attempt to never > >> think about ACS again at the customer's peril? What about the existing > >> non-ThunderX products with Cavium vendor ID, does this really apply to > >> those? I would strongly suggest taking the device ID into account. > >> See examples like the pci_quirk_intel_pch_acs quirk where the initial > >> filter is PCI_ANY_ID, but specific device types and ranges of device > >> IDs are identified by the function for evaluation. This seems reckless > >> to me and I'd advise that it be reverted. Thanks, > > > > I'd be happy to revert this, but it would be easier if somebody sent a > > patch and a changelog. > > > > I agree that it should be reverted. > > I was hoping that Manish or Tirumalesh would fix this properly by only > activating the quirk on the faulty hardware, but such a fix has never > appeared. > > This was supposed to be true for all Cavium chips. We got such answer from Hardware architects, but it might change in the future. > I will be happy to replace this with DEVICE_ID. > Manish, > Could you please take this u, if not I will send a patch to do this. I think several device ids or subsystem_device may be involved. See above. > > Tirumalesh. > > David Daney > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-02-14 4:44 ` Alex Williamson 2017-02-14 15:07 ` Bjorn Helgaas @ 2017-03-05 7:07 ` Sunil Kovvuri 2017-03-06 17:09 ` Alex Williamson 1 sibling, 1 reply; 9+ messages in thread From: Sunil Kovvuri @ 2017-03-05 7:07 UTC (permalink / raw) To: Alex Williamson Cc: Manish Jaggi, LKML, linux-pci, Tirumalesh Chalamarla, Richter, Robert, Bjorn Helgaas On Tue, Feb 14, 2017 at 10:14 AM, Alex Williamson <alex.williamson@redhat.com> wrote: > On Sat, 30 Jan 2016 01:33:58 +0530 > Manish Jaggi <mjaggi@caviumnetworks.com> wrote: > >> Cavium devices matching this quirk do not perform >> peer-to-peer with other functions, allowing masking out >> these bits as if they were unimplemented in the ACS capability. >> >> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> >> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> >> --- >> drivers/pci/quirks.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index 7e32730..a300fa6 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) >> #endif >> } >> >> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) >> +{ >> + /* >> + * Cavium devices matching this quirk do not perform >> + * peer-to-peer with other functions, allowing masking out >> + * these bits as if they were unimplemented in the ACS capability. >> + */ >> + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | >> + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); >> + >> + return acs_flags ? 0 : 1; >> +} >> + >> /* >> * Many Intel PCH root ports do provide ACS-like features to disable peer >> * transactions and validate bus numbers in requests, but do not provide an >> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { >> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, >> { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ >> { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ >> + /* Cavium ThunderX */ >> + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, >> { 0 } >> }; >> > > Apologies for not catching this, but what sort of crystal ball do you > have that can predict not only current devices, but future devices will > not support peer-to-peer features? Is there an internal design > guidelines reference specification for Cavium that we can realistically > expect this to remain consistent, or is this just an attempt to never > think about ACS again at the customer's peril? What about the existing > non-ThunderX products with Cavium vendor ID, does this really apply to > those? I would strongly suggest taking the device ID into account. > See examples like the pci_quirk_intel_pch_acs quirk where the initial > filter is PCI_ANY_ID, but specific device types and ranges of device > IDs are identified by the function for evaluation. This seems reckless > to me and I'd advise that it be reverted. Thanks, > > Alex Just a thought, even if Cavium considers to support ACS for future devices, wouldn't it be better to add exception list inside the quirk on a need basis rather than adding big list of devices that don't. Especially when currently almost all Cavium PCI devices don't support ACS. Thanks, Sunil. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Add cavium acs pci quirk 2017-03-05 7:07 ` Sunil Kovvuri @ 2017-03-06 17:09 ` Alex Williamson 0 siblings, 0 replies; 9+ messages in thread From: Alex Williamson @ 2017-03-06 17:09 UTC (permalink / raw) To: Sunil Kovvuri Cc: Manish Jaggi, LKML, linux-pci, Tirumalesh Chalamarla, Richter, Robert, Bjorn Helgaas On Sun, 5 Mar 2017 12:37:31 +0530 Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > On Tue, Feb 14, 2017 at 10:14 AM, Alex Williamson > <alex.williamson@redhat.com> wrote: > > On Sat, 30 Jan 2016 01:33:58 +0530 > > Manish Jaggi <mjaggi@caviumnetworks.com> wrote: > > > >> Cavium devices matching this quirk do not perform > >> peer-to-peer with other functions, allowing masking out > >> these bits as if they were unimplemented in the ACS capability. > >> > >> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com> > >> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com> > >> --- > >> drivers/pci/quirks.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > >> index 7e32730..a300fa6 100644 > >> --- a/drivers/pci/quirks.c > >> +++ b/drivers/pci/quirks.c > >> @@ -3814,6 +3814,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > >> #endif > >> } > >> > >> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > >> +{ > >> + /* > >> + * Cavium devices matching this quirk do not perform > >> + * peer-to-peer with other functions, allowing masking out > >> + * these bits as if they were unimplemented in the ACS capability. > >> + */ > >> + acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > >> + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > >> + > >> + return acs_flags ? 0 : 1; > >> +} > >> + > >> /* > >> * Many Intel PCH root ports do provide ACS-like features to disable peer > >> * transactions and validate bus numbers in requests, but do not provide an > >> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled { > >> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs }, > >> { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */ > >> { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */ > >> + /* Cavium ThunderX */ > >> + { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs }, > >> { 0 } > >> }; > >> > > > > Apologies for not catching this, but what sort of crystal ball do you > > have that can predict not only current devices, but future devices will > > not support peer-to-peer features? Is there an internal design > > guidelines reference specification for Cavium that we can realistically > > expect this to remain consistent, or is this just an attempt to never > > think about ACS again at the customer's peril? What about the existing > > non-ThunderX products with Cavium vendor ID, does this really apply to > > those? I would strongly suggest taking the device ID into account. > > See examples like the pci_quirk_intel_pch_acs quirk where the initial > > filter is PCI_ANY_ID, but specific device types and ranges of device > > IDs are identified by the function for evaluation. This seems reckless > > to me and I'd advise that it be reverted. Thanks, > > > > Alex > > Just a thought, even if Cavium considers to support ACS for future devices, > wouldn't it be better to add exception list inside the quirk on a need basis > rather than adding big list of devices that don't. Especially when currently > almost all Cavium PCI devices don't support ACS. The same argument can be made the other way, which is the more concerning problem. What if Cavium releases a device that does not support ACS nor has ACS equivalent isolation? This quirk would assume isolation exists. Now think about the compatibility problem of trying to negate this quirk for that device on any kernel that has shipped with this open-ended quirk. What's the support situation if a user relies on that isolation that maybe doesn't exist? Any solution that assumes unknown future devices have isolation is dangerous. Perhaps it's justifiable if the company has strong internal design guidelines about this sort of thing, but even those get lost or change over time without necessarily evaluating all the implications. IMO, the only workable solution is to white-list *only* the devices where we know ACS equivalent isolation is present. We're still waiting on Cavium's patch to make that happen. Thanks, Alex ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-03-06 17:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-29 20:03 [PATCH] PCI: Add cavium acs pci quirk Manish Jaggi 2016-02-04 22:44 ` Bjorn Helgaas 2017-02-14 4:44 ` Alex Williamson 2017-02-14 15:07 ` Bjorn Helgaas 2017-02-27 19:02 ` David Daney 2017-02-27 19:13 ` Chalamarla, Tirumalesh 2017-02-27 19:25 ` David Daney 2017-03-05 7:07 ` Sunil Kovvuri 2017-03-06 17:09 ` Alex Williamson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).