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

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

[Feng Kan]

The APM X-Gene PCIe root port does not support ACS at this point.
Since the root does not allow peer to peer transactions, mask out
ACS capability flag bits.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/pci/quirks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 085fb78..951064d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4368,6 +4368,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_mf_endpoint_acs },
 	{ 0 }
 };
 
-- 
1.8.3.1

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

[Alex Williamson]

On Mon, 17 Jul 2017 17:45:52 -0700
Feng Kan <fkan@apm.com> wrote:

> The APM X-Gene PCIe root port does not support ACS at this point.
> Since the root does not allow peer to peer transactions, mask out
> ACS capability flag bits.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  drivers/pci/quirks.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 085fb78..951064d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4368,6 +4368,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_mf_endpoint_acs },
>  	{ 0 }
>  };


You're using the "mf_enpoint" quirk for something that is not an
endpoint and may or may not be multi-function.  Downstream ports have
different rules than multi-function endpoints and the quirk function is
commented to reflect why certain fields are relevant or not to
multi-function endpoints.  I don't think it's valid to use this quirk
for other device types, we'd only need to accidentally test the assumed
device type in that function to break you in the future. Additionally,
lack of peer to peer does not necessarily imply functionality like
Source Validation, without which a device can spoof the requester ID.
Thanks,

Alex

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

[Feng Kan]

On Mon, Jul 17, 2017 at 7:23 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 17 Jul 2017 17:45:52 -0700
> Feng Kan <fkan@apm.com> wrote:
>
>> The APM X-Gene PCIe root port does not support ACS at this point.
>> Since the root does not allow peer to peer transactions, mask out
>> ACS capability flag bits.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  drivers/pci/quirks.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 085fb78..951064d 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -4368,6 +4368,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_mf_endpoint_acs },
>>       { 0 }
>>  };
>
>
> You're using the "mf_enpoint" quirk for something that is not an
> endpoint and may or may not be multi-function.  Downstream ports have
> different rules than multi-function endpoints and the quirk function is
> commented to reflect why certain fields are relevant or not to
> multi-function endpoints.  I don't think it's valid to use this quirk
> for other device types, we'd only need to accidentally test the assumed
> device type in that function to break you in the future.
Would creating another function like what Cavium did be sufficient?


Additionally,
> lack of peer to peer does not necessarily imply functionality like
> Source Validation, without which a device can spoof the requester ID.
> Thanks,
I can remove the statement.
>
> Alex
Alex, the goal here is to enable virtualization to work correctly.
Please let me know if the
above is sufficient. Much thanks.
>

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

[Alex Williamson]

On Tue, 18 Jul 2017 10:42:40 -0700
Feng Kan <fkan@apm.com> wrote:

> On Mon, Jul 17, 2017 at 7:23 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Mon, 17 Jul 2017 17:45:52 -0700
> > Feng Kan <fkan@apm.com> wrote:
> >  
> >> The APM X-Gene PCIe root port does not support ACS at this point.
> >> Since the root does not allow peer to peer transactions, mask out
> >> ACS capability flag bits.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >>  drivers/pci/quirks.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> index 085fb78..951064d 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -4368,6 +4368,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_mf_endpoint_acs },
> >>       { 0 }
> >>  };  
> >
> >
> > You're using the "mf_enpoint" quirk for something that is not an
> > endpoint and may or may not be multi-function.  Downstream ports have
> > different rules than multi-function endpoints and the quirk function is
> > commented to reflect why certain fields are relevant or not to
> > multi-function endpoints.  I don't think it's valid to use this quirk
> > for other device types, we'd only need to accidentally test the assumed
> > device type in that function to break you in the future.  
> Would creating another function like what Cavium did be sufficient?

A function that exposes the actual isolation capabilities of the
hardware would be better, yes.

> Additionally,
> > lack of peer to peer does not necessarily imply functionality like
> > Source Validation, without which a device can spoof the requester ID.
> > Thanks,  
> I can remove the statement.

I don't know what that means, does the hardware support an equivalent
to source validation or not?  What's the response of the root port if
the downstream device issues a transaction spoofing devices not within
the bus number ranges of the bridge?

> Alex, the goal here is to enable virtualization to work correctly.
> Please let me know if the
> above is sufficient. Much thanks.

Of course, but that means that the hardware vendor is vouching that
this device provides the equivalent isolation for each of the missing
components of ACS.  Claiming to have isolation capabilities that don't
exist would be irresponsible and put users of that hardware at risk.
Thanks,

Alex

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

[Feng Kan]

>
> I don't know what that means, does the hardware support an equivalent
> to source validation or not?

Yes, source validation is done through the smmu.

  What's the response of the root port if
> the downstream device issues a transaction spoofing devices not within
> the bus number ranges of the bridge?
HW guys informs me there is way to disable transactions between root port.
I will confirm later.

>
>> Alex, the goal here is to enable virtualization to work correctly.
>> Please let me know if the
>> above is sufficient. Much thanks.
>
> Of course, but that means that the hardware vendor is vouching that
> this device provides the equivalent isolation for each of the missing
> components of ACS.  Claiming to have isolation capabilities that don't
> exist would be irresponsible and put users of that hardware at risk.
Agreed, I believe we do have isolation in our case based on the conference
we had today.

> Thanks,
>
> Alex

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

[Alex Williamson]

On Tue, 18 Jul 2017 22:37:00 -0700
Feng Kan <fkan@apm.com> wrote:

> >
> > I don't know what that means, does the hardware support an equivalent
> > to source validation or not?  
> 
> Yes, source validation is done through the smmu.

The SMMU does a context lookup based on the bdf, but if the root port
does not support SV, what is it that prevents the device from spoofing
a different bdf?  How does the smmu intercept this?  Thanks,

Alex
 
>   What's the response of the root port if
> > the downstream device issues a transaction spoofing devices not within
> > the bus number ranges of the bridge?  
> HW guys informs me there is way to disable transactions between root port.
> I will confirm later.
> 
> >  
> >> Alex, the goal here is to enable virtualization to work correctly.
> >> Please let me know if the
> >> above is sufficient. Much thanks.  
> >
> > Of course, but that means that the hardware vendor is vouching that
> > this device provides the equivalent isolation for each of the missing
> > components of ACS.  Claiming to have isolation capabilities that don't
> > exist would be irresponsible and put users of that hardware at risk.  
> Agreed, I believe we do have isolation in our case based on the conference
> we had today.
> 
> > Thanks,
> >
> > Alex