On Tue, Sep 12, 2017 at 10:15:45AM -0600, Alex Williamson wrote:
> On Tue, 12 Sep 2017 04:55:16 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@xxxxxxxxxxxxxxxxxx> wrote:
>
> > This commit makes PIC ACS quirk applicable only to Cavium PCIE devices
> > and Cavium PCIE Root Ports which has limited PCI capabilities in terms
> > of no ACS support. Match function checks for ACS support and exact ACS
> > bits set at the device capabilities.
> > Also by this commit we get rid off device ID range values checkings.
> >
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/pci/quirks.c | 30 +++++++++++++++++++++++++-----
> > 1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..11ca951 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,6 +4211,29 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> > #endif
> > }
> >
> > +#define CAVIUM_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | \
> > + PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT)
> > +
> > +static __inline__ bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> > +{
> > + int pos = 0;
> > + u32 caps = 0;
> > +
> > + /* Filter out a few obvious non-matches first */
> > + if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> > + return false;
> > +
> > + /* Get the ACS caps offset */
> > + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> > + if (pos) {
> > + pci_read_config_dword(dev, pos + PCI_ACS_CAP, &caps);
> > + /* If we have no such bits set, then we will need a quirk */
> > + return ((caps & CAVIUM_ACS_FLAGS) != CAVIUM_ACS_FLAGS);
> > + }
> > +
> > + return true;
> > +}
> > +
> > static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > {
> > /*
> > @@ -4218,13 +4241,10 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > * 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);
> > -
> > - if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > + if (!pci_quirk_cavium_acs_match(dev))
> > return -ENOTTY;
> >
> > - return acs_flags ? 0 : 1;
> > + return acs_flags & ~(CAVIUM_ACS_FLAGS) ? 0 : 1;
> > }
> >
> > static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>
> No please. As I read it, this is assuming that any Cavium PCIe root
> port supports the equivalent isolation flags. Do you have a crystal
> ball to know about all the future PCIe root ports that Cavium is going
> to ship?
Well, yes, my bad then.
Would the check for exact device id (or some range) of pcie device/root
port be more suitable here (as it is implemented for other vendors) ?
> Quirk the devices you can verify support the equivalent
> isolation capabilities and solve this problem automatically for future
> devices by implementing ACS in hardware. No free pass for all future
> hardware, especially not one that overrides the hardware potentially
> implementing ACS in the future and ignoring it if it's not sufficient.
> We're actually trying to be diligent to test for isolation and this
> entirely ignores that.
>
> Also, as we've been through with APM, how do you justify each of these
> ACS flags? Claiming that a device does not support peer-to-peer does
> not automatically justify Source Validation. What feature of your
> hardware allows you to claim that? How does a root port that does not
> support P2P imply anything about Transaction Blocking? What about
> Direct Translated P2P? If the device doesn't support P2P, doesn't that
> mean it shouldn't claim DT? Like the attempted APM quirk, I think this
> original quirk here has just taken and misapplied the mask we use for
> multifunction devices where downstream ports have much different
> requirements for ACS. Thanks,
My understanding that CN81xx/83xx/88xx pcie bridges/root ports has no ACS support.
And the original mask was constructed in that way erroneously copied I guess.
Would the resetting of RR/CR/UF/SV bits be more correct here ?
>
> Alex
WBR,
Vadim
