diff mbox series

PCI/ATS: Allow to enable ATS on VFs even if it is not enabled on PF

Message ID 20230208184321.867666-1-gankulkarni@os.amperecomputing.com
State New
Headers
Series PCI/ATS: Allow to enable ATS on VFs even if it is not enabled on PF |

Commit Message

Ganapatrao Kulkarni Feb. 8, 2023, 6:43 p.m. UTC 
  As per PCIe specification(section 10.5), If a VF implements an
ATS capability, its associated PF must implement an ATS capability.
The ATS Capabilities in VFs and their associated PFs are permitted to
be enabled independently.
Also, it states that the Smallest Translation Unit (STU) for VFs must be
hardwired to Zero and the associated PF's value applies to VFs STU.

The current code allows to enable ATS on VFs only if it is already
enabled on associated PF, which is not necessary as per the specification.

It is only required to have valid STU programmed on PF to enable
ATS on VFs. Adding code to write the first VFs STU to a PF's STU
when PFs ATS is not enabled.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 drivers/pci/ats.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Leon Romanovsky Feb. 12, 2023, 6:14 p.m. UTC | #1 
On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> As per PCIe specification(section 10.5), If a VF implements an
> ATS capability, its associated PF must implement an ATS capability.
> The ATS Capabilities in VFs and their associated PFs are permitted to
> be enabled independently.
> Also, it states that the Smallest Translation Unit (STU) for VFs must be
> hardwired to Zero and the associated PF's value applies to VFs STU.
> 
> The current code allows to enable ATS on VFs only if it is already
> enabled on associated PF, which is not necessary as per the specification.
> 
> It is only required to have valid STU programmed on PF to enable
> ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> when PFs ATS is not enabled.

Can you please add here quotes from the spec and its version? I don't see
anything like this in my version of PCIe specification.

Thanks

> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  drivers/pci/ats.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..a97ec67201d1 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  	if (ps < PCI_ATS_MIN_STU)
>  		return -EINVAL;
>  
> -	/*
> -	 * Note that enabling ATS on a VF fails unless it's already enabled
> -	 * with the same STU on the PF.
> -	 */
>  	ctrl = PCI_ATS_CTRL_ENABLE;
>  	if (dev->is_virtfn) {
>  		pdev = pci_physfn(dev);
> +
> +		if (!pdev->ats_enabled &&
> +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
> +			u16 ctrl2;
> +
> +			/* Associated PF's STU value applies to VFs. */
> +			pdev->ats_stu = ps;
> +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
> +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
> +		}
> +
>  		if (pdev->ats_stu != ps)
>  			return -EINVAL;
>  	} else {
> -- 
> 2.39.1
>
Bjorn Helgaas Feb. 15, 2023, 8:57 p.m. UTC | #2 
[+cc Will, Robin, Joerg for arm-smmu-v3 page size question]

On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > As per PCIe specification(section 10.5), If a VF implements an
> > ATS capability, its associated PF must implement an ATS capability.
> > The ATS Capabilities in VFs and their associated PFs are permitted to
> > be enabled independently.
> > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > hardwired to Zero and the associated PF's value applies to VFs STU.
> > 
> > The current code allows to enable ATS on VFs only if it is already
> > enabled on associated PF, which is not necessary as per the specification.
> > 
> > It is only required to have valid STU programmed on PF to enable
> > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > when PFs ATS is not enabled.
>
> Can you please add here quotes from the spec and its version? I don't see
> anything like this in my version of PCIe specification.

See PCIe r6.0, sec 10.5.1.

> > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > ---
> >  drivers/pci/ats.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index f9cc2e10b676..a97ec67201d1 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> >  	if (ps < PCI_ATS_MIN_STU)
> >  		return -EINVAL;
> >  
> > -	/*
> > -	 * Note that enabling ATS on a VF fails unless it's already enabled
> > -	 * with the same STU on the PF.
> > -	 */
> >  	ctrl = PCI_ATS_CTRL_ENABLE;
> >  	if (dev->is_virtfn) {
> >  		pdev = pci_physfn(dev);
> > +
> > +		if (!pdev->ats_enabled &&
> > +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
> > +			u16 ctrl2;
> > +
> > +			/* Associated PF's STU value applies to VFs. */
> > +			pdev->ats_stu = ps;
> > +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
> > +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
> > +		}

For reference, it is this way because of edc90fee916b ("PCI: Allocate
ATS struct during enumeration").  The rationale was that since the PF
STU applies to all VFs, we should require that the PF STU be
programmed before enabling ATS on any of the VFs.

This patch relaxes that so the PF STU would be set either by (a)
enabling ATS on the PF or (b) enabling ATS on the first VF.

This looks racy because theoretically drivers for VF A and VF B could
independently call pci_enable_ats() with different IOMMU page sizes,
and we don't know which will get there first.

Most callers supply a compile-time constant (PAGE_SHIFT or
VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
fancier, but I *assume* it would still supply the same IOMMU page size
for all VFs of a given PF.

But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
muck with the configuration of dev_B.  Maybe we should configure the
PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
Is there some way to get the IOMMU page size at that time?

> >  		if (pdev->ats_stu != ps)
> >  			return -EINVAL;
> >  	} else {
> > -- 
> > 2.39.1
> >
Leon Romanovsky Feb. 16, 2023, 7:26 a.m. UTC | #3 
On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> 
> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > As per PCIe specification(section 10.5), If a VF implements an
> > > ATS capability, its associated PF must implement an ATS capability.
> > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > be enabled independently.
> > > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > 
> > > The current code allows to enable ATS on VFs only if it is already
> > > enabled on associated PF, which is not necessary as per the specification.
> > > 
> > > It is only required to have valid STU programmed on PF to enable
> > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > when PFs ATS is not enabled.
> >
> > Can you please add here quotes from the spec and its version? I don't see
> > anything like this in my version of PCIe specification.
> 
> See PCIe r6.0, sec 10.5.1.

Awesome, I have old versions.

Thanks
Leon Romanovsky Feb. 16, 2023, 7:46 a.m. UTC | #4 
On Thu, Feb 16, 2023 at 09:26:15AM +0200, Leon Romanovsky wrote:
> On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> > [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> > 
> > On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > > As per PCIe specification(section 10.5), If a VF implements an
> > > > ATS capability, its associated PF must implement an ATS capability.
> > > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > > be enabled independently.
> > > > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > > 
> > > > The current code allows to enable ATS on VFs only if it is already
> > > > enabled on associated PF, which is not necessary as per the specification.
> > > > 
> > > > It is only required to have valid STU programmed on PF to enable
> > > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > > when PFs ATS is not enabled.
> > >
> > > Can you please add here quotes from the spec and its version? I don't see
> > > anything like this in my version of PCIe specification.
> > 
> > See PCIe r6.0, sec 10.5.1.
> 
> Awesome, I have old versions.

OK, where should I read about this sentence?

"It is only required to have valid STU programmed on PF to enable
 ATS on VFs. Adding code to write the first VFs STU to a PF's STU
 when PFs ATS is not enabled."

From spec:
"Smallest Translation Unit (STU) - This value indicates to the Function the minimum number of
4096-byte blocks that is indicated in a Translation Completions or Invalidate Requests. This is a power of
2 multiplier and the number of blocks is 2STU. A value of 0 0000b indicates one block and a value of
1 1111b indicates 231 blocks (or 8 TB total)

For VFs, this field must be hardwired to Zero. The associated PF's value applies.
Default value is 0 0000b"

And enable bit doesn't have any sentence about STU.

Thanks
Joerg Roedel Feb. 16, 2023, 10:23 a.m. UTC | #5 
On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> 
> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > As per PCIe specification(section 10.5), If a VF implements an
> > > ATS capability, its associated PF must implement an ATS capability.
> > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > be enabled independently.

Well, the spec is one thing, existing hardware the other. Have you
checked the history of the PF-before-VF requirement before making that
change?

It is possible that early PASID-capable hardware actually required
PF-before-VF enablement of ATS.

Regards,

	Joerg
Jean-Philippe Brucker Feb. 16, 2023, 10:47 a.m. UTC | #6 
On Thu, Feb 16, 2023 at 09:46:51AM +0200, Leon Romanovsky wrote:
> On Thu, Feb 16, 2023 at 09:26:15AM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> > > [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> > > 
> > > On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > > > As per PCIe specification(section 10.5), If a VF implements an
> > > > > ATS capability, its associated PF must implement an ATS capability.
> > > > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > > > be enabled independently.
> > > > > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > > > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > > > 
> > > > > The current code allows to enable ATS on VFs only if it is already
> > > > > enabled on associated PF, which is not necessary as per the specification.
> > > > > 
> > > > > It is only required to have valid STU programmed on PF to enable
> > > > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > > > when PFs ATS is not enabled.
> > > >
> > > > Can you please add here quotes from the spec and its version? I don't see
> > > > anything like this in my version of PCIe specification.

In PCIe r6.0, 10.5.1 ATS Extended Capability:

"The ATS Capabilities in VFs and their associated PFs are permitted to be
enabled independently."

> For VFs, this field must be hardwired to Zero. The associated PF's value applies.
> Default value is 0 0000b"

And this sentence indicates that the PF's STU should be configured
appropriately in order to use ATS in the VF.

So a driver is permitted to enable the VF ATS capability without enabling
the PF ATS cap, though the STU value of the PF cap still applies. But the
first sentence is weak ("permitted" instead of "required"), so as Joerg
said, some device implementations may still require to enable the PF cap
in order to enable the VF cap.

Maybe we could have a list of vendor:device IDs which allow enabling the
VF cap independently?

Thanks,
Jean
Jean-Philippe Brucker Feb. 16, 2023, 11:12 a.m. UTC | #7 
On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> 
> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > As per PCIe specification(section 10.5), If a VF implements an
> > > ATS capability, its associated PF must implement an ATS capability.
> > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > be enabled independently.
> > > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > 
> > > The current code allows to enable ATS on VFs only if it is already
> > > enabled on associated PF, which is not necessary as per the specification.
> > > 
> > > It is only required to have valid STU programmed on PF to enable
> > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > when PFs ATS is not enabled.
> >
> > Can you please add here quotes from the spec and its version? I don't see
> > anything like this in my version of PCIe specification.
> 
> See PCIe r6.0, sec 10.5.1.
> 
> > > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > > ---
> > >  drivers/pci/ats.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > index f9cc2e10b676..a97ec67201d1 100644
> > > --- a/drivers/pci/ats.c
> > > +++ b/drivers/pci/ats.c
> > > @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > >  	if (ps < PCI_ATS_MIN_STU)
> > >  		return -EINVAL;
> > >  
> > > -	/*
> > > -	 * Note that enabling ATS on a VF fails unless it's already enabled
> > > -	 * with the same STU on the PF.
> > > -	 */
> > >  	ctrl = PCI_ATS_CTRL_ENABLE;
> > >  	if (dev->is_virtfn) {
> > >  		pdev = pci_physfn(dev);
> > > +
> > > +		if (!pdev->ats_enabled &&
> > > +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
> > > +			u16 ctrl2;
> > > +
> > > +			/* Associated PF's STU value applies to VFs. */
> > > +			pdev->ats_stu = ps;
> > > +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
> > > +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
> > > +		}
> 
> For reference, it is this way because of edc90fee916b ("PCI: Allocate
> ATS struct during enumeration").  The rationale was that since the PF
> STU applies to all VFs, we should require that the PF STU be
> programmed before enabling ATS on any of the VFs.
> 
> This patch relaxes that so the PF STU would be set either by (a)
> enabling ATS on the PF or (b) enabling ATS on the first VF.
> 
> This looks racy because theoretically drivers for VF A and VF B could
> independently call pci_enable_ats() with different IOMMU page sizes,
> and we don't know which will get there first.
> 
> Most callers supply a compile-time constant (PAGE_SHIFT or
> VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
> fancier, but I *assume* it would still supply the same IOMMU page size
> for all VFs of a given PF.
> 
> But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
> muck with the configuration of dev_B.  Maybe we should configure the
> PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
> Is there some way to get the IOMMU page size at that time?

Not really, on Arm the supported page sizes are discovered when probing
the SMMU registers, which may happen later than enumeration, during module
loading.

What this patch is trying to solve is:
* want the PF to bypass SMMU translation, and the VF to undergo SMMU
  translation (in order to assign it to a VM)
* SMMU forbids enabling ATS for a configuration that bypasses translation.
  So the PF ATS capability must be left disabled.

For this situation I wonder if we could do: the SMMU driver, seeing that
the PF is configured to bypass, calls a new function "pci_configure_ats()"
instead of pci_enable_ats(), which would only set the STU but leave the
cap disabled. Then when setting up translation for the VF, the SMMU driver
calls pci_enable_ats() as usual, which sees the PF's STU set appropriately
and succeeds.

Thanks,
Jean
Ganapatrao Kulkarni Feb. 16, 2023, 11:36 a.m. UTC | #8 
On 16-02-2023 04:42 pm, Jean-Philippe Brucker wrote:
> On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
>> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
>>
>> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
>>> On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
>>>> As per PCIe specification(section 10.5), If a VF implements an
>>>> ATS capability, its associated PF must implement an ATS capability.
>>>> The ATS Capabilities in VFs and their associated PFs are permitted to
>>>> be enabled independently.
>>>> Also, it states that the Smallest Translation Unit (STU) for VFs must be
>>>> hardwired to Zero and the associated PF's value applies to VFs STU.
>>>>
>>>> The current code allows to enable ATS on VFs only if it is already
>>>> enabled on associated PF, which is not necessary as per the specification.
>>>>
>>>> It is only required to have valid STU programmed on PF to enable
>>>> ATS on VFs. Adding code to write the first VFs STU to a PF's STU
>>>> when PFs ATS is not enabled.
>>>
>>> Can you please add here quotes from the spec and its version? I don't see
>>> anything like this in my version of PCIe specification.
>>
>> See PCIe r6.0, sec 10.5.1.
>>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>   drivers/pci/ats.c | 15 +++++++++++----
>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>> index f9cc2e10b676..a97ec67201d1 100644
>>>> --- a/drivers/pci/ats.c
>>>> +++ b/drivers/pci/ats.c
>>>> @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>>   	if (ps < PCI_ATS_MIN_STU)
>>>>   		return -EINVAL;
>>>>   
>>>> -	/*
>>>> -	 * Note that enabling ATS on a VF fails unless it's already enabled
>>>> -	 * with the same STU on the PF.
>>>> -	 */
>>>>   	ctrl = PCI_ATS_CTRL_ENABLE;
>>>>   	if (dev->is_virtfn) {
>>>>   		pdev = pci_physfn(dev);
>>>> +
>>>> +		if (!pdev->ats_enabled &&
>>>> +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
>>>> +			u16 ctrl2;
>>>> +
>>>> +			/* Associated PF's STU value applies to VFs. */
>>>> +			pdev->ats_stu = ps;
>>>> +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
>>>> +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
>>>> +		}
>>
>> For reference, it is this way because of edc90fee916b ("PCI: Allocate
>> ATS struct during enumeration").  The rationale was that since the PF
>> STU applies to all VFs, we should require that the PF STU be
>> programmed before enabling ATS on any of the VFs.
>>
>> This patch relaxes that so the PF STU would be set either by (a)
>> enabling ATS on the PF or (b) enabling ATS on the first VF.
>>
>> This looks racy because theoretically drivers for VF A and VF B could
>> independently call pci_enable_ats() with different IOMMU page sizes,
>> and we don't know which will get there first.
>>
>> Most callers supply a compile-time constant (PAGE_SHIFT or
>> VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
>> fancier, but I *assume* it would still supply the same IOMMU page size
>> for all VFs of a given PF.
>>
>> But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
>> muck with the configuration of dev_B.  Maybe we should configure the
>> PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
>> Is there some way to get the IOMMU page size at that time?
> 
> Not really, on Arm the supported page sizes are discovered when probing
> the SMMU registers, which may happen later than enumeration, during module
> loading.
> 
> What this patch is trying to solve is:
> * want the PF to bypass SMMU translation, and the VF to undergo SMMU
>    translation (in order to assign it to a VM)
> * SMMU forbids enabling ATS for a configuration that bypasses translation.
>    So the PF ATS capability must be left disabled.
> 
> For this situation I wonder if we could do: the SMMU driver, seeing that
> the PF is configured to bypass, calls a new function "pci_configure_ats()"

IMO, This seems to be feasible solution for this situation, which 
addresses SMMU-ATS expectation in bypass and we could avoid PCI VFs 
race. pci_configure_ats() can be called to program/configure STU of a PF 
in smmu-bypass mode.

> instead of pci_enable_ats(), which would only set the STU but leave the
> cap disabled. Then when setting up translation for the VF, the SMMU driver
> calls pci_enable_ats() as usual, which sees the PF's STU set appropriately
> and succeeds.
> 
> Thanks,
> Jean

Thanks,
Ganapat
Ganapatrao Kulkarni Feb. 21, 2023, 9:13 a.m. UTC | #9 
Hi Bjorn,

On 16-02-2023 05:06 pm, Ganapatrao Kulkarni wrote:
> 
> 
> On 16-02-2023 04:42 pm, Jean-Philippe Brucker wrote:
>> On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
>>> [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
>>>
>>> On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
>>>> On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
>>>>> As per PCIe specification(section 10.5), If a VF implements an
>>>>> ATS capability, its associated PF must implement an ATS capability.
>>>>> The ATS Capabilities in VFs and their associated PFs are permitted to
>>>>> be enabled independently.
>>>>> Also, it states that the Smallest Translation Unit (STU) for VFs 
>>>>> must be
>>>>> hardwired to Zero and the associated PF's value applies to VFs STU.
>>>>>
>>>>> The current code allows to enable ATS on VFs only if it is already
>>>>> enabled on associated PF, which is not necessary as per the 
>>>>> specification.
>>>>>
>>>>> It is only required to have valid STU programmed on PF to enable
>>>>> ATS on VFs. Adding code to write the first VFs STU to a PF's STU
>>>>> when PFs ATS is not enabled.
>>>>
>>>> Can you please add here quotes from the spec and its version? I 
>>>> don't see
>>>> anything like this in my version of PCIe specification.
>>>
>>> See PCIe r6.0, sec 10.5.1.
>>>
>>>>> Signed-off-by: Ganapatrao Kulkarni 
>>>>> <gankulkarni@os.amperecomputing.com>
>>>>> ---
>>>>>   drivers/pci/ats.c | 15 +++++++++++----
>>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>>> index f9cc2e10b676..a97ec67201d1 100644
>>>>> --- a/drivers/pci/ats.c
>>>>> +++ b/drivers/pci/ats.c
>>>>> @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>>>       if (ps < PCI_ATS_MIN_STU)
>>>>>           return -EINVAL;
>>>>> -    /*
>>>>> -     * Note that enabling ATS on a VF fails unless it's already 
>>>>> enabled
>>>>> -     * with the same STU on the PF.
>>>>> -     */
>>>>>       ctrl = PCI_ATS_CTRL_ENABLE;
>>>>>       if (dev->is_virtfn) {
>>>>>           pdev = pci_physfn(dev);
>>>>> +
>>>>> +        if (!pdev->ats_enabled &&
>>>>> +                (pdev->ats_stu < PCI_ATS_MIN_STU)) {
>>>>> +            u16 ctrl2;
>>>>> +
>>>>> +            /* Associated PF's STU value applies to VFs. */
>>>>> +            pdev->ats_stu = ps;
>>>>> +            ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - 
>>>>> PCI_ATS_MIN_STU);
>>>>> +            pci_write_config_word(pdev, pdev->ats_cap + 
>>>>> PCI_ATS_CTRL, ctrl2);
>>>>> +        }
>>>
>>> For reference, it is this way because of edc90fee916b ("PCI: Allocate
>>> ATS struct during enumeration").  The rationale was that since the PF
>>> STU applies to all VFs, we should require that the PF STU be
>>> programmed before enabling ATS on any of the VFs.
>>>
>>> This patch relaxes that so the PF STU would be set either by (a)
>>> enabling ATS on the PF or (b) enabling ATS on the first VF.
>>>
>>> This looks racy because theoretically drivers for VF A and VF B could
>>> independently call pci_enable_ats() with different IOMMU page sizes,
>>> and we don't know which will get there first.
>>>
>>> Most callers supply a compile-time constant (PAGE_SHIFT or
>>> VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
>>> fancier, but I *assume* it would still supply the same IOMMU page size
>>> for all VFs of a given PF.
>>>
>>> But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
>>> muck with the configuration of dev_B.  Maybe we should configure the
>>> PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
>>> Is there some way to get the IOMMU page size at that time?
>>
>> Not really, on Arm the supported page sizes are discovered when probing
>> the SMMU registers, which may happen later than enumeration, during 
>> module
>> loading.
>>
>> What this patch is trying to solve is:
>> * want the PF to bypass SMMU translation, and the VF to undergo SMMU
>>    translation (in order to assign it to a VM)
>> * SMMU forbids enabling ATS for a configuration that bypasses 
>> translation.
>>    So the PF ATS capability must be left disabled.
>>
>> For this situation I wonder if we could do: the SMMU driver, seeing that
>> the PF is configured to bypass, calls a new function 
>> "pci_configure_ats()"
> 
> IMO, This seems to be feasible solution for this situation, which 
> addresses SMMU-ATS expectation in bypass and we could avoid PCI VFs 
> race. pci_configure_ats() can be called to program/configure STU of a PF 
> in smmu-bypass mode.
> 

Can we add pci_configure_ats/pci_configure_ats_stu helper?

>> instead of pci_enable_ats(), which would only set the STU but leave the
>> cap disabled. Then when setting up translation for the VF, the SMMU 
>> driver
>> calls pci_enable_ats() as usual, which sees the PF's STU set 
>> appropriately
>> and succeeds.
>>
>> Thanks,
>> Jean
> 
> Thanks,
> Ganapat

Thanks,
Ganapat
Bjorn Helgaas Feb. 21, 2023, 3:46 p.m. UTC | #10 
On Thu, Feb 16, 2023 at 11:12:24AM +0000, Jean-Philippe Brucker wrote:
> On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > > As per PCIe specification(section 10.5), If a VF implements an
> > > > ATS capability, its associated PF must implement an ATS capability.
> > > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > > be enabled independently.
> > > > Also, it states that the Smallest Translation Unit (STU) for VFs must be
> > > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > > 
> > > > The current code allows to enable ATS on VFs only if it is already
> > > > enabled on associated PF, which is not necessary as per the specification.
> > > > 
> > > > It is only required to have valid STU programmed on PF to enable
> > > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > > when PFs ATS is not enabled.
> > >
> > > Can you please add here quotes from the spec and its version? I don't see
> > > anything like this in my version of PCIe specification.
> > 
> > See PCIe r6.0, sec 10.5.1.
> > 
> > > > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > > > ---
> > > >  drivers/pci/ats.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > index f9cc2e10b676..a97ec67201d1 100644
> > > > --- a/drivers/pci/ats.c
> > > > +++ b/drivers/pci/ats.c
> > > > @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > > >  	if (ps < PCI_ATS_MIN_STU)
> > > >  		return -EINVAL;
> > > >  
> > > > -	/*
> > > > -	 * Note that enabling ATS on a VF fails unless it's already enabled
> > > > -	 * with the same STU on the PF.
> > > > -	 */
> > > >  	ctrl = PCI_ATS_CTRL_ENABLE;
> > > >  	if (dev->is_virtfn) {
> > > >  		pdev = pci_physfn(dev);
> > > > +
> > > > +		if (!pdev->ats_enabled &&
> > > > +				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
> > > > +			u16 ctrl2;
> > > > +
> > > > +			/* Associated PF's STU value applies to VFs. */
> > > > +			pdev->ats_stu = ps;
> > > > +			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
> > > > +			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
> > > > +		}
> > 
> > For reference, it is this way because of edc90fee916b ("PCI: Allocate
> > ATS struct during enumeration").  The rationale was that since the PF
> > STU applies to all VFs, we should require that the PF STU be
> > programmed before enabling ATS on any of the VFs.
> > 
> > This patch relaxes that so the PF STU would be set either by (a)
> > enabling ATS on the PF or (b) enabling ATS on the first VF.
> > 
> > This looks racy because theoretically drivers for VF A and VF B could
> > independently call pci_enable_ats() with different IOMMU page sizes,
> > and we don't know which will get there first.
> > 
> > Most callers supply a compile-time constant (PAGE_SHIFT or
> > VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
> > fancier, but I *assume* it would still supply the same IOMMU page size
> > for all VFs of a given PF.
> > 
> > But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
> > muck with the configuration of dev_B.  Maybe we should configure the
> > PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
> > Is there some way to get the IOMMU page size at that time?
> 
> Not really, on Arm the supported page sizes are discovered when probing
> the SMMU registers, which may happen later than enumeration, during module
> loading.
>
> What this patch is trying to solve is:
> * want the PF to bypass SMMU translation, and the VF to undergo SMMU
>   translation (in order to assign it to a VM)
> * SMMU forbids enabling ATS for a configuration that bypasses translation.
>   So the PF ATS capability must be left disabled.
> 
> For this situation I wonder if we could do: the SMMU driver, seeing that
> the PF is configured to bypass, calls a new function "pci_configure_ats()"
> instead of pci_enable_ats(), which would only set the STU but leave the
> cap disabled. Then when setting up translation for the VF, the SMMU driver
> calls pci_enable_ats() as usual, which sees the PF's STU set appropriately
> and succeeds.

Seems reasonable.

It's weird to me that the SMMU is between PCI and memory, but the
driver seems to insert itself in the middle after PCI enumeration.
And maybe even after some PCI device driver binding?  But I guess if
you arrange for the SMMU driver to configure the PF before the VF
driver gets started, that's all we need from a PCI perspective.

Bjorn
Jean-Philippe Brucker Feb. 23, 2023, 3:45 p.m. UTC | #11 
On Tue, Feb 21, 2023 at 09:46:24AM -0600, Bjorn Helgaas wrote:
> It's weird to me that the SMMU is between PCI and memory, but the
> driver seems to insert itself in the middle after PCI enumeration.
> And maybe even after some PCI device driver binding?

No this shouldn't happen, because device drivers expect DMA to be
operational in their probe() function, so at that point the IOMMU must be
configured. The core and IOMMU subsystems enforce probe dependency between
the SMMU and the PCI device, using links described by ACPI or device tree.

Thanks,
Jean
diff mbox series

Patch

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..a97ec67201d1 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -67,13 +67,20 @@  int pci_enable_ats(struct pci_dev *dev, int ps)
 	if (ps < PCI_ATS_MIN_STU)
 		return -EINVAL;
 
-	/*
-	 * Note that enabling ATS on a VF fails unless it's already enabled
-	 * with the same STU on the PF.
-	 */
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
 		pdev = pci_physfn(dev);
+
+		if (!pdev->ats_enabled &&
+				(pdev->ats_stu < PCI_ATS_MIN_STU)) {
+			u16 ctrl2;
+
+			/* Associated PF's STU value applies to VFs. */
+			pdev->ats_stu = ps;
+			ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - PCI_ATS_MIN_STU);
+			pci_write_config_word(pdev, pdev->ats_cap + PCI_ATS_CTRL, ctrl2);
+		}
+
 		if (pdev->ats_stu != ps)
 			return -EINVAL;
 	} else {