[PATCH] PCI: Add cavium acs pci quirk

[PATCH] PCI: Add cavium acs pci quirk

[Manish Jaggi]

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.

Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7e32730..a300fa6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3814,6 +3814,19 @@ 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)
+{
+	/*
+	 * 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.
+	 */
+	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
@@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
 	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
 	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
+	/* Cavium ThunderX */
+	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
 	{ 0 }
 };

-- 
1.9.1

Re: [PATCH] PCI: Add cavium acs pci quirk

[Bjorn Helgaas]

On Sat, Jan 30, 2016 at 01:33:58AM +0530, Manish Jaggi wrote:
> 
> 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.
> 
> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>

Applied as follows to pci/virtualization for v4.6, thanks, Manish!

Note that this quirk applies to *all* Cavium devices.  I assume that's
what you want; I only mention it because your comment says "Cavium
devices matching this quirk ...", which is all of them because the
code says PCI_ANY_ID.

Bjorn


commit b404bcfbf035413dcce539c8ba2c9986d220d8ed
Author: Manish Jaggi <mjaggi@caviumnetworks.com>
Date:   Sat Jan 30 01:33:58 2016 +0530

    PCI: Add ACS quirk for all Cavium devices
    
    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.
    
    Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0575a1e..85fa6a2a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3832,6 +3832,19 @@ 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)
+{
+	/*
+	 * 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.
+	 */
+	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
@@ -3984,6 +3997,8 @@ static const struct pci_dev_acs_enabled {
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
 	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
 	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
+	/* Cavium ThunderX */
+	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
 	{ 0 }
 };
 

Re: [PATCH] PCI: Add cavium acs pci quirk

[Alex Williamson]

On Sat, 30 Jan 2016 01:33:58 +0530
Manish Jaggi <mjaggi@caviumnetworks.com> wrote:

> 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.
> 
> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
> ---
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7e32730..a300fa6 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3814,6 +3814,19 @@ 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)
> +{
> +	/*
> +	 * 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.
> +	 */
> +	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
> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
>  	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
>  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
> +	/* Cavium ThunderX */
> +	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
>  	{ 0 }
>  };
> 

Apologies for not catching this, but what sort of crystal ball do you
have that can predict not only current devices, but future devices will
not support peer-to-peer features?  Is there an internal design
guidelines reference specification for Cavium that we can realistically
expect this to remain consistent, or is this just an attempt to never
think about ACS again at the customer's peril?  What about the existing
non-ThunderX products with Cavium vendor ID, does this really apply to
those?  I would strongly suggest taking the device ID into account.
See examples like the pci_quirk_intel_pch_acs quirk where the initial
filter is PCI_ANY_ID, but specific device types and ranges of device
IDs are identified by the function for evaluation.  This seems reckless
to me and I'd advise that it be reverted.  Thanks,

Alex

Re: [PATCH] PCI: Add cavium acs pci quirk

[Bjorn Helgaas]

On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote:
> On Sat, 30 Jan 2016 01:33:58 +0530
> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
> 
> > 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.
> > 
> > Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
> > Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
> > ---
> >  drivers/pci/quirks.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 7e32730..a300fa6 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3814,6 +3814,19 @@ 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)
> > +{
> > +	/*
> > +	 * 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.
> > +	 */
> > +	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
> > @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
> >  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
> >  	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
> >  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
> > +	/* Cavium ThunderX */
> > +	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
> >  	{ 0 }
> >  };
> > 
> 
> Apologies for not catching this, but what sort of crystal ball do you
> have that can predict not only current devices, but future devices will
> not support peer-to-peer features?  Is there an internal design
> guidelines reference specification for Cavium that we can realistically
> expect this to remain consistent, or is this just an attempt to never
> think about ACS again at the customer's peril?  What about the existing
> non-ThunderX products with Cavium vendor ID, does this really apply to
> those?  I would strongly suggest taking the device ID into account.
> See examples like the pci_quirk_intel_pch_acs quirk where the initial
> filter is PCI_ANY_ID, but specific device types and ranges of device
> IDs are identified by the function for evaluation.  This seems reckless
> to me and I'd advise that it be reverted.  Thanks,

I'd be happy to revert this, but it would be easier if somebody sent a
patch and a changelog.

Re: [PATCH] PCI: Add cavium acs pci quirk

[David Daney]

On 02/14/2017 07:07 AM, Bjorn Helgaas wrote:
> On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote:
>> On Sat, 30 Jan 2016 01:33:58 +0530
>> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
>>
>>> 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.
>>>
>>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
>>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
>>> ---
>>>  drivers/pci/quirks.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index 7e32730..a300fa6 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3814,6 +3814,19 @@ 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)
>>> +{
>>> +	/*
>>> +	 * 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.
>>> +	 */
>>> +	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
>>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
>>>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
>>>  	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
>>>  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
>>> +	/* Cavium ThunderX */
>>> +	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
>>>  	{ 0 }
>>>  };
>>>
>>
>> Apologies for not catching this, but what sort of crystal ball do you
>> have that can predict not only current devices, but future devices will
>> not support peer-to-peer features?  Is there an internal design
>> guidelines reference specification for Cavium that we can realistically
>> expect this to remain consistent, or is this just an attempt to never
>> think about ACS again at the customer's peril?  What about the existing
>> non-ThunderX products with Cavium vendor ID, does this really apply to
>> those?  I would strongly suggest taking the device ID into account.
>> See examples like the pci_quirk_intel_pch_acs quirk where the initial
>> filter is PCI_ANY_ID, but specific device types and ranges of device
>> IDs are identified by the function for evaluation.  This seems reckless
>> to me and I'd advise that it be reverted.  Thanks,
>
> I'd be happy to revert this, but it would be easier if somebody sent a
> patch and a changelog.
>

I agree that it should be reverted.

I was hoping that Manish or Tirumalesh would fix this properly by only 
activating the quirk on the faulty hardware, but such a fix has never 
appeared.

David Daney

Re: [PATCH] PCI: Add cavium acs pci quirk

[Chalamarla, Tirumalesh]

On 2/27/17, 11:02 AM, "David Daney" <ddaney.cavm@gmail.com> wrote:

    On 02/14/2017 07:07 AM, Bjorn Helgaas wrote:
    > On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote:
    >> On Sat, 30 Jan 2016 01:33:58 +0530
    >> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
    >>
    >>> 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.
    >>>
    >>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
    >>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
    >>> ---
    >>>  drivers/pci/quirks.c | 15 +++++++++++++++
    >>>  1 file changed, 15 insertions(+)
    >>>
    >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
    >>> index 7e32730..a300fa6 100644
    >>> --- a/drivers/pci/quirks.c
    >>> +++ b/drivers/pci/quirks.c
    >>> @@ -3814,6 +3814,19 @@ 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)
    >>> +{
    >>> +	/*
    >>> +	 * 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.
    >>> +	 */
    >>> +	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
    >>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
    >>>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
    >>>  	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
    >>>  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
    >>> +	/* Cavium ThunderX */
    >>> +	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
    >>>  	{ 0 }
    >>>  };
    >>>
    >>
    >> Apologies for not catching this, but what sort of crystal ball do you
    >> have that can predict not only current devices, but future devices will
    >> not support peer-to-peer features?  Is there an internal design
    >> guidelines reference specification for Cavium that we can realistically
    >> expect this to remain consistent, or is this just an attempt to never
    >> think about ACS again at the customer's peril?  What about the existing
    >> non-ThunderX products with Cavium vendor ID, does this really apply to
    >> those?  I would strongly suggest taking the device ID into account.
    >> See examples like the pci_quirk_intel_pch_acs quirk where the initial
    >> filter is PCI_ANY_ID, but specific device types and ranges of device
    >> IDs are identified by the function for evaluation.  This seems reckless
    >> to me and I'd advise that it be reverted.  Thanks,
    >
    > I'd be happy to revert this, but it would be easier if somebody sent a
    > patch and a changelog.
    >
    
    I agree that it should be reverted.
    
    I was hoping that Manish or Tirumalesh would fix this properly by only 
    activating the quirk on the faulty hardware, but such a fix has never 
    appeared.

This was supposed to be true for all Cavium chips. We got such answer from Hardware architects, but it might change in the future. 
I will be happy to replace this with DEVICE_ID.
Manish,
Could you please take this u, if not I will send a patch to do this.

Tirumalesh. 
    
    David Daney
    
    
    
    

Re: [PATCH] PCI: Add cavium acs pci quirk

[David Daney]

On 02/27/2017 11:13 AM, Chalamarla, Tirumalesh wrote:
>
>
> On 2/27/17, 11:02 AM, "David Daney" <ddaney.cavm@gmail.com> wrote:
>
>     On 02/14/2017 07:07 AM, Bjorn Helgaas wrote:
>     > On Mon, Feb 13, 2017 at 09:44:57PM -0700, Alex Williamson wrote:
>     >> On Sat, 30 Jan 2016 01:33:58 +0530
>     >> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
>     >>
>     >>> 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.
>     >>>
>     >>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
>     >>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
>     >>> ---
>     >>>  drivers/pci/quirks.c | 15 +++++++++++++++
>     >>>  1 file changed, 15 insertions(+)
>     >>>
>     >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>     >>> index 7e32730..a300fa6 100644
>     >>> --- a/drivers/pci/quirks.c
>     >>> +++ b/drivers/pci/quirks.c
>     >>> @@ -3814,6 +3814,19 @@ 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)
>     >>> +{

Perhaps add a check here for dev->device and/or dev->subsystem_device to 
see if we need the workaround.  There may be many that match, so ...


>     >>> +	/*
>     >>> +	 * 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.
>     >>> +	 */
>     >>> +	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
>     >>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
>     >>>  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
>     >>>  	{ 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
>     >>>  	{ 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
>     >>> +	/* Cavium ThunderX */
>     >>> +	{ PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },

.. Leaving PCI_ANY_ID here may keep the table more manageable.

>     >>>  	{ 0 }
>     >>>  };
>     >>>
>     >>
>     >> Apologies for not catching this, but what sort of crystal ball do you
>     >> have that can predict not only current devices, but future devices will
>     >> not support peer-to-peer features?  Is there an internal design
>     >> guidelines reference specification for Cavium that we can realistically
>     >> expect this to remain consistent, or is this just an attempt to never
>     >> think about ACS again at the customer's peril?  What about the existing
>     >> non-ThunderX products with Cavium vendor ID, does this really apply to
>     >> those?  I would strongly suggest taking the device ID into account.
>     >> See examples like the pci_quirk_intel_pch_acs quirk where the initial
>     >> filter is PCI_ANY_ID, but specific device types and ranges of device
>     >> IDs are identified by the function for evaluation.  This seems reckless
>     >> to me and I'd advise that it be reverted.  Thanks,
>     >
>     > I'd be happy to revert this, but it would be easier if somebody sent a
>     > patch and a changelog.
>     >
>
>     I agree that it should be reverted.
>
>     I was hoping that Manish or Tirumalesh would fix this properly by only
>     activating the quirk on the faulty hardware, but such a fix has never
>     appeared.
>
> This was supposed to be true for all Cavium chips. We got such answer from Hardware architects, but it might change in the future.
> I will be happy to replace this with DEVICE_ID.
> Manish,
> Could you please take this u, if not I will send a patch to do this.

I think several device ids or subsystem_device may be involved.

See above.


>
> Tirumalesh.
>
>     David Daney
>
>
>
>
>

Re: [PATCH] PCI: Add cavium acs pci quirk

[Sunil Kovvuri]

On Tue, Feb 14, 2017 at 10:14 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Sat, 30 Jan 2016 01:33:58 +0530
> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
>
>> 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.
>>
>> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
>> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
>> ---
>>  drivers/pci/quirks.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 7e32730..a300fa6 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3814,6 +3814,19 @@ 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)
>> +{
>> +     /*
>> +      * 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.
>> +      */
>> +     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
>> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
>>       { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
>>       { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
>>       { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
>> +     /* Cavium ThunderX */
>> +     { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
>>       { 0 }
>>  };
>>
>
> Apologies for not catching this, but what sort of crystal ball do you
> have that can predict not only current devices, but future devices will
> not support peer-to-peer features?  Is there an internal design
> guidelines reference specification for Cavium that we can realistically
> expect this to remain consistent, or is this just an attempt to never
> think about ACS again at the customer's peril?  What about the existing
> non-ThunderX products with Cavium vendor ID, does this really apply to
> those?  I would strongly suggest taking the device ID into account.
> See examples like the pci_quirk_intel_pch_acs quirk where the initial
> filter is PCI_ANY_ID, but specific device types and ranges of device
> IDs are identified by the function for evaluation.  This seems reckless
> to me and I'd advise that it be reverted.  Thanks,
>
> Alex

Just a thought, even if Cavium considers to support ACS for future devices,
wouldn't it be better to add exception list inside the quirk on a need basis
rather than adding big list of devices that don't. Especially when currently
almost all Cavium PCI devices don't support ACS.

Thanks,
Sunil.

Re: [PATCH] PCI: Add cavium acs pci quirk

[Alex Williamson]

On Sun, 5 Mar 2017 12:37:31 +0530
Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:

> On Tue, Feb 14, 2017 at 10:14 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Sat, 30 Jan 2016 01:33:58 +0530
> > Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
> >  
> >> 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.
> >>
> >> Acked-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>
> >> Signed-off-by: Manish Jaggi <mjaggi@caviumnetworks.com>
> >> ---
> >>  drivers/pci/quirks.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> index 7e32730..a300fa6 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -3814,6 +3814,19 @@ 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)
> >> +{
> >> +     /*
> >> +      * 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.
> >> +      */
> >> +     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
> >> @@ -3966,6 +3979,8 @@ static const struct pci_dev_acs_enabled {
> >>       { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
> >>       { 0x19a2, 0x710, pci_quirk_mf_endpoint_acs }, /* Emulex BE3-R */
> >>       { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
> >> +     /* Cavium ThunderX */
> >> +     { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
> >>       { 0 }
> >>  };
> >>  
> >
> > Apologies for not catching this, but what sort of crystal ball do you
> > have that can predict not only current devices, but future devices will
> > not support peer-to-peer features?  Is there an internal design
> > guidelines reference specification for Cavium that we can realistically
> > expect this to remain consistent, or is this just an attempt to never
> > think about ACS again at the customer's peril?  What about the existing
> > non-ThunderX products with Cavium vendor ID, does this really apply to
> > those?  I would strongly suggest taking the device ID into account.
> > See examples like the pci_quirk_intel_pch_acs quirk where the initial
> > filter is PCI_ANY_ID, but specific device types and ranges of device
> > IDs are identified by the function for evaluation.  This seems reckless
> > to me and I'd advise that it be reverted.  Thanks,
> >
> > Alex  
> 
> Just a thought, even if Cavium considers to support ACS for future devices,
> wouldn't it be better to add exception list inside the quirk on a need basis
> rather than adding big list of devices that don't. Especially when currently
> almost all Cavium PCI devices don't support ACS.

The same argument can be made the other way, which is the more
concerning problem.  What if Cavium releases a device that does not
support ACS nor has ACS equivalent isolation?  This quirk would assume
isolation exists.  Now think about the compatibility problem of trying
to negate this quirk for that device on any kernel that has shipped
with this open-ended quirk.  What's the support situation if a user
relies on that isolation that maybe doesn't exist?  Any solution that
assumes unknown future devices have isolation is dangerous.  Perhaps
it's justifiable if the company has strong internal design guidelines
about this sort of thing, but even those get lost or change over time
without necessarily evaluating all the implications.  IMO, the only
workable solution is to white-list *only* the devices where we know ACS
equivalent isolation is present.  We're still waiting on Cavium's
patch to make that happen.  Thanks,

Alex