Re: [PATCH V3] pci: quirk: Apply APM ACS quirk to XGene devices

[Alex Williamson]

On Fri, 28 Jul 2017 11:50:43 -0700
Feng Kan <fkan@xxxxxxx> wrote:

> The APM X-Gene PCIe root port does not support ACS at this point.
> However, the hw provides isolation and source validation through
> the SMMU. The stream ID generated by the PCIe ports contain both
> the BDF as well as the port ID in its 3 most significant bits.
> Turn on ACS but disable all the peer to peer features.
> 
> Signed-off-by: Feng Kan <fkan@xxxxxxx>
> ---
> 	V3 Change: Add comment regarding unique port id in stream ID
> 	V2 Change: Move XGene ACS quirk to unique XGene function
> 
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 085fb78..0f8f1cd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4120,6 +4120,19 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  	return acs_flags ? 0 : 1;
>  }
>  
> +static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	/*
> +	 * XGene root matching this quirk do not allow peer-to-peer
> +	 * transactions with others, 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
> @@ -4368,6 +4381,8 @@ static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
>  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
>  	/* Cavium ThunderX */
>  	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
> +	/* APM XGene */
> +	{ PCI_VENDOR_ID_AMCC, 0xE004, pci_quirk_xgene_acs },
>  	{ 0 }
>  };
>  

Hi Feng,

Sorry, I have one more question as I happened to spend some time
looking at PCI_ACS_DT this week.  DT specifies that peer-to-peer should
occur normally between egress ports for transactions which are
pre-translated by an ATS unit on the endpoint.  Therefore if a root
port doesn't allow peer-to-peer, it seems like it should not claim to
support PCI_ACS_DT.  I know your quirk is just a copy of the Cavium
one, but we should also go back and verify this question with them, or
perhaps I'm misinterpreting this capability.  AIUI this is also a
performance capability, not an isolation capability, so it shouldn't
affect the ability to consider a device isolated, it only gets
confusing if we expect a performance benefit from this but don't
actually see one.  Does your root port have this ability to
selectively allow peer-to-peer of pre-translated transactions?  Thanks,

Alex