| Message ID | 20171019112645.GD6332@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote: > On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote: > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > > > > version 7: > > - update patch description accordingly to review comments; > > - split for two patches: for ACS mask change and device id match change; > > - remove macro #define of ACS flags, put it back to quirk function; > > - remove '__inline_' from device id matching function; > > - wrap code comments to fit into 80 columns; > > - comment fix: change 0xa00 to 0xf800 for matching function description; > > > > version 6: > > - comment typo fix: change 0xa00 to 0xa000; > > > > version 5: > > - update code comments accordingly to review comments; > > > > version 4: > > - update ACS mask (remove TB and TD bits), update commit message (remove > > ACS register printout); > > > > version 3: > > - update subject: remove CN8XXX from subject line, replace it with ThunderX; > > > > version 2: > > - update match function in order to filter only ThunderX devices by device > > ids to properly filter CN8XXX devices, update subject & description with > > ACS register info (v2 was rejected by maillist due to triple X in subject); > > > > Vadim Lomovtsev (2): > > PCI: quirks: Set Cavium ACS capability quirk flags to assert > > RR/CR/SV/UF. > > PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports > > > > drivers/pci/quirks.c | 26 ++++++++++++++++++++------ > > 1 file changed, 20 insertions(+), 6 deletions(-) > > If I'm reading this correctly, the first patch is basically fixing a bug in > the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all > Cavium devices")). Right? Yes, first patch fixes improper ACS flags in the patch you mentioned. The second one fixes devid check from another patch (b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices). > > I put them on pci/virtualization for v4.15 as follows (patches unchanged, > changelogs wordsmithed). > Good, thank you. > You mentioned stable backports. b404bcfbf035 ("PCI: Add ACS quirk for all > Cavium devices") appeared in v4.6. b77d537d00d0 ("PCI: Apply Cavium ACS > quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12. I assume > you would ideally want all this backported as far as v4.6? We'd have to > figure out how to express that in stable tags. Ideally yes, I think we would need to have them backported onto stable(s), but so far I didn't see any bug-reports which could require these fixes. Anyway, in term of backporting them - are there anything I can help with ? WBR, Vadim > > > commit 60f6d5f7ebc4bba83d73115f58b805f2f2618667 > Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > Date: Tue Oct 17 05:47:38 2017 -0700 > > PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF > > The Cavium ThunderX (CN8XXX) family of PCIe Root Ports does not advertise > an ACS capability. However, the RTL internally implements similar > protection as if ACS had Request Redirection, Completion Redirection, > Source Validation, and Upstream Forwarding features enabled. > > Change Cavium ACS capabilities quirk flags accordingly. > > Fixes: b404bcfbf035 ("PCI: Add ACS quirk for all Cavium devices") > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index a4d33619a7bb..c23650cfd5a1 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4214,12 +4214,14 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > 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. > + * Cavium root ports don't advertise an ACS capability. However, > + * the RTL internally implements similar protection as if ACS had > + * Request Redirection, Completion Redirection, Source Validation, > + * and Upstream Forwarding features enabled. Assert that the > + * hardware implements and enables equivalent ACS functionality for > + * these flags. > */ > - acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > - PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > + acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF); > > if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff))) > return -ENOTTY; > > commit 35da518fcad24f7be515976238a4667b5ccd1711 > Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > Date: Tue Oct 17 05:47:39 2017 -0700 > > PCI: Apply Cavium ThunderX ACS quirk to more Root Ports > > Extend the Cavium ThunderX ACS quirk to cover more device IDs and restrict > it to only Root Ports. > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > [bhelgaas: changelog] > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index c23650cfd5a1..0e22cce05742 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4211,6 +4211,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > #endif > } > > +static bool pci_quirk_cavium_acs_match(struct pci_dev *dev) > +{ > + /* > + * Effectively selects all downstream ports for whole ThunderX 1 > + * family by 0xf800 mask (which represents 8 SoCs), while the lower > + * bits of device ID are used to indicate which subdevice is used > + * within the SoC. > + */ > + return (pci_is_pcie(dev) && > + (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) && > + ((dev->device & 0xf800) == 0xa000)); > +} > + > static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > { > /* > @@ -4223,7 +4236,7 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > */ > acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF); > > - if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff))) > + if (!pci_quirk_cavium_acs_match(dev)) > return -ENOTTY; > > return acs_flags ? 0 : 1;
On Thu, Oct 19, 2017 at 04:59:21AM -0700, Vadim Lomovtsev wrote: > On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote: > > On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote: > > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > > > > > > version 7: > > > - update patch description accordingly to review comments; > > > - split for two patches: for ACS mask change and device id match change; > > > - remove macro #define of ACS flags, put it back to quirk function; > > > - remove '__inline_' from device id matching function; > > > - wrap code comments to fit into 80 columns; > > > - comment fix: change 0xa00 to 0xf800 for matching function description; > > > > > > version 6: > > > - comment typo fix: change 0xa00 to 0xa000; > > > > > > version 5: > > > - update code comments accordingly to review comments; > > > > > > version 4: > > > - update ACS mask (remove TB and TD bits), update commit message (remove > > > ACS register printout); > > > > > > version 3: > > > - update subject: remove CN8XXX from subject line, replace it with ThunderX; > > > > > > version 2: > > > - update match function in order to filter only ThunderX devices by device > > > ids to properly filter CN8XXX devices, update subject & description with > > > ACS register info (v2 was rejected by maillist due to triple X in subject); > > > > > > Vadim Lomovtsev (2): > > > PCI: quirks: Set Cavium ACS capability quirk flags to assert > > > RR/CR/SV/UF. > > > PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports > > > > > > drivers/pci/quirks.c | 26 ++++++++++++++++++++------ > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > If I'm reading this correctly, the first patch is basically fixing a bug in > > the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all > > Cavium devices")). Right? > > Yes, first patch fixes improper ACS flags in the patch you mentioned. > > The second one fixes devid check from another patch > (b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices). > > > > > I put them on pci/virtualization for v4.15 as follows (patches unchanged, > > changelogs wordsmithed). > > > > Good, thank you. > > > You mentioned stable backports. b404bcfbf035 ("PCI: Add ACS quirk for all > > Cavium devices") appeared in v4.6. b77d537d00d0 ("PCI: Apply Cavium ACS > > quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12. I assume > > you would ideally want all this backported as far as v4.6? We'd have to > > figure out how to express that in stable tags. > > Ideally yes, I think we would need to have them backported onto stable(s), > but so far I didn't see any bug-reports which could require these fixes. > > Anyway, in term of backporting them - are there anything I can help with ? I added the following tags, which I *think* should be sufficient: PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF Cc: stable@vger.kernel.org # v4.6+: b77d537d00d0: PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices PCI: Apply Cavium ThunderX ACS quirk to more Root Ports Cc: stable@vger.kernel.org # v4.12+
On Thu, Oct 19, 2017 at 01:50:10PM -0500, Bjorn Helgaas wrote: > On Thu, Oct 19, 2017 at 04:59:21AM -0700, Vadim Lomovtsev wrote: > > On Thu, Oct 19, 2017 at 06:26:45AM -0500, Bjorn Helgaas wrote: > > > On Tue, Oct 17, 2017 at 05:47:37AM -0700, Vadim Lomovtsev wrote: > > > > From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> > > > > > > > > version 7: > > > > - update patch description accordingly to review comments; > > > > - split for two patches: for ACS mask change and device id match change; > > > > - remove macro #define of ACS flags, put it back to quirk function; > > > > - remove '__inline_' from device id matching function; > > > > - wrap code comments to fit into 80 columns; > > > > - comment fix: change 0xa00 to 0xf800 for matching function description; > > > > > > > > version 6: > > > > - comment typo fix: change 0xa00 to 0xa000; > > > > > > > > version 5: > > > > - update code comments accordingly to review comments; > > > > > > > > version 4: > > > > - update ACS mask (remove TB and TD bits), update commit message (remove > > > > ACS register printout); > > > > > > > > version 3: > > > > - update subject: remove CN8XXX from subject line, replace it with ThunderX; > > > > > > > > version 2: > > > > - update match function in order to filter only ThunderX devices by device > > > > ids to properly filter CN8XXX devices, update subject & description with > > > > ACS register info (v2 was rejected by maillist due to triple X in subject); > > > > > > > > Vadim Lomovtsev (2): > > > > PCI: quirks: Set Cavium ACS capability quirk flags to assert > > > > RR/CR/SV/UF. > > > > PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports > > > > > > > > drivers/pci/quirks.c | 26 ++++++++++++++++++++------ > > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > If I'm reading this correctly, the first patch is basically fixing a bug in > > > the original Cavium ACS quirk (b404bcfbf035 ("PCI: Add ACS quirk for all > > > Cavium devices")). Right? > > > > Yes, first patch fixes improper ACS flags in the patch you mentioned. > > > > The second one fixes devid check from another patch > > (b77d537d00d0 PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices). > > > > > > > > I put them on pci/virtualization for v4.15 as follows (patches unchanged, > > > changelogs wordsmithed). > > > > > > > Good, thank you. > > > > > You mentioned stable backports. b404bcfbf035 ("PCI: Add ACS quirk for all > > > Cavium devices") appeared in v4.6. b77d537d00d0 ("PCI: Apply Cavium ACS > > > quirk only to CN81xx/CN83xx/CN88xx devices") appeared in v4.12. I assume > > > you would ideally want all this backported as far as v4.6? We'd have to > > > figure out how to express that in stable tags. > > > > Ideally yes, I think we would need to have them backported onto stable(s), > > but so far I didn't see any bug-reports which could require these fixes. > > > > Anyway, in term of backporting them - are there anything I can help with ? > > I added the following tags, which I *think* should be sufficient: > > PCI: Set Cavium ACS capability quirk flags to assert RR/CR/SV/UF > Cc: stable@vger.kernel.org # v4.6+: b77d537d00d0: PCI: Apply Cavium ACS quirk only to CN81xx/CN83xx/CN88xx devices > > PCI: Apply Cavium ThunderX ACS quirk to more Root Ports > Cc: stable@vger.kernel.org # v4.12+ Looks good. Thank you. -Vadim
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index a4d33619a7bb..c23650cfd5a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4214,12 +4214,14 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) 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. + * Cavium root ports don't advertise an ACS capability. However, + * the RTL internally implements similar protection as if ACS had + * Request Redirection, Completion Redirection, Source Validation, + * and Upstream Forwarding features enabled. Assert that the + * hardware implements and enables equivalent ACS functionality for + * these flags. */ - acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | - PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); + acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF); if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff))) return -ENOTTY; commit 35da518fcad24f7be515976238a4667b5ccd1711 Author: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> Date: Tue Oct 17 05:47:39 2017 -0700 PCI: Apply Cavium ThunderX ACS quirk to more Root Ports Extend the Cavium ThunderX ACS quirk to cover more device IDs and restrict it to only Root Ports. Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com> [bhelgaas: changelog] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index c23650cfd5a1..0e22cce05742 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4211,6 +4211,19 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) #endif } +static bool pci_quirk_cavium_acs_match(struct pci_dev *dev) +{ + /* + * Effectively selects all downstream ports for whole ThunderX 1 + * family by 0xf800 mask (which represents 8 SoCs), while the lower + * bits of device ID are used to indicate which subdevice is used + * within the SoC. + */ + return (pci_is_pcie(dev) && + (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) && + ((dev->device & 0xf800) == 0xa000)); +} + static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) { /* @@ -4223,7 +4236,7 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) */ acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF); - if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff))) + if (!pci_quirk_cavium_acs_match(dev)) return -ENOTTY; return acs_flags ? 0 : 1;