[v5] PCI: quirks: update Cavium ThunderX ACS quirk implementation

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion/request redirection,
upstream forwarding and validation features enabled.

Current quirk implementation doesn't take into account PCIERCs which
also needs to be quirked. So the pci device id check mask is updated
and check of device ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
---
 drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

Hi guys,

Could you please consider to review following patch?

For v5 changes:
- ACS mask comment has been updated.
- comment has been added for device id filtering mask at match
  function to provide explantion of CN8xxx devid scheme.

WBR,
Vadim

On Mon, Sep 25, 2017 at 06:08:40AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..0fd2e15 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ 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)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * 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.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.9.5
>

On Mon, 25 Sep 2017 06:08:40 -0700
Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:

> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)


Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Thanks for making the updates


> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..0fd2e15 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ 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)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * 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.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

On Tue, Sep 26, 2017 at 09:43:43AM -0600, Alex Williamson wrote:
> On Mon, 25 Sep 2017 06:08:40 -0700
> Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > ---
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Thanks for making the updates
>

Thank you for your patience and for your time, Alex.



Bjorn,

Would you mind to pick up this patch ?

WBR,
Vadim

> 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..0fd2e15 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ 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)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * 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.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>

On Tue, Sep 26, 2017 at 09:00:26AM -0700, Vadim Lomovtsev wrote:
> On Tue, Sep 26, 2017 at 09:43:43AM -0600, Alex Williamson wrote:
> > On Mon, 25 Sep 2017 06:08:40 -0700
> > Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> > 
> > > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > > in terms of no ACS support advertisement. However, the RTL internally
> > > implements similar protection as if ACS had completion/request redirection,
> > > upstream forwarding and validation features enabled.
> > > 
> > > Current quirk implementation doesn't take into account PCIERCs which
> > > also needs to be quirked. So the pci device id check mask is updated
> > > and check of device ID moved into separate function.
> > > 
> > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> > > ---
> > >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> > >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > 
> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Thanks for making the updates
> >
> 
> Thank you for your patience and for your time, Alex.
> 
> 
> 
> Bjorn,
> 
> Would you mind to pick up this patch ?
> 
> WBR,
> Vadim
> 
> > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index a4d3361..0fd2e15 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -4211,20 +4211,33 @@ 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)
> > > +/*
> > > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > > + * However, the RTL internally implements similar protection as if
> > > + * ACS had completion redirection, forwarding and validation features enabled.
> > > + * So by this flags we're asserting that the hardware implements and
> > > + * enables equivalent ACS functionality for these flags.
> > > + */
> > > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > > +
> > > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> > >  {
> > >  	/*
> > > -	 * 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.
> > > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > > +	 * by 0xa00 mask (which represents 8 SoCs), while the lower bits of device ID

Bjorn,

Please ignore this since I found one more typo here: it should be 0xa000, sorry. Will post v6.
Sorry for wasting your time, guys.

WBR,
Vadim


> > > +	 * are used to indicate which subdevice is used within the SoC.
> > >  	 */
> > > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > > +	return (pci_is_pcie(dev) &&
> > > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > > +		((dev->device & 0xf800) == 0xa000));
> > > +}
> > >  
> > > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > > +{
> > > +	if (!pci_quirk_cavium_acs_match(dev))
> > >  		return -ENOTTY;
> > >  
> > > -	return acs_flags ? 0 : 1;
> > > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> > >  }
> > >  
> > >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> >