2023-02-02 12:43:40

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

The current smmu-v3 driver does not enable PCI ATS for physical functions
of ATS capable End Points when booted in smmu bypass mode
(iommu.passthrough=1). This will not allow virtual functions to enable
ATS(even though EP supports it) while they are attached to a VM using
VFIO driver.

This patch adds changes to enable ATS support for physical functions
in passthrough/bypass mode as well.

Also, adding check to avoid disabling of ATS if it is not enabled,
to avoid unnecessary call-traces.

Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6d5df91c5c46..5a605cb5ccef 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2313,11 +2313,16 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
static void arm_smmu_disable_ats(struct arm_smmu_master *master)
{
struct arm_smmu_domain *smmu_domain = master->domain;
+ struct pci_dev *pdev;

if (!master->ats_enabled)
return;

- pci_disable_ats(to_pci_dev(master->dev));
+ pdev = to_pci_dev(master->dev);
+
+ if (pdev->ats_enabled)
+ pci_disable_ats(pdev);
+
/*
* Ensure ATS is disabled at the endpoint before we issue the
* ATC invalidation via the SMMU.
@@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)

master->domain = smmu_domain;

- if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
- master->ats_enabled = arm_smmu_ats_supported(master);
+ master->ats_enabled = arm_smmu_ats_supported(master);

arm_smmu_install_ste_for_dev(master);

--
2.37.3



2023-02-02 13:22:41

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

On 2023-02-02 12:40, Ganapatrao Kulkarni wrote:
> The current smmu-v3 driver does not enable PCI ATS for physical functions
> of ATS capable End Points when booted in smmu bypass mode
> (iommu.passthrough=1). This will not allow virtual functions to enable
> ATS(even though EP supports it) while they are attached to a VM using
> VFIO driver.
>
> This patch adds changes to enable ATS support for physical functions
> in passthrough/bypass mode as well.
>
> Also, adding check to avoid disabling of ATS if it is not enabled,
> to avoid unnecessary call-traces.
>
> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6d5df91c5c46..5a605cb5ccef 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2313,11 +2313,16 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
> static void arm_smmu_disable_ats(struct arm_smmu_master *master)
> {
> struct arm_smmu_domain *smmu_domain = master->domain;
> + struct pci_dev *pdev;
>
> if (!master->ats_enabled)
> return;
>
> - pci_disable_ats(to_pci_dev(master->dev));
> + pdev = to_pci_dev(master->dev);
> +
> + if (pdev->ats_enabled)

If the master->ats_enabled check above passes when ATS isn't actually
enabled, surely that's a bug?

Robin.

> + pci_disable_ats(pdev);
> +
> /*
> * Ensure ATS is disabled at the endpoint before we issue the
> * ATC invalidation via the SMMU.
> @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>
> master->domain = smmu_domain;
>
> - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> - master->ats_enabled = arm_smmu_ats_supported(master);
> + master->ats_enabled = arm_smmu_ats_supported(master);
>
> arm_smmu_install_ste_for_dev(master);
>

2023-02-03 10:44:35

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well



On 02-02-2023 06:52 pm, Robin Murphy wrote:
> On 2023-02-02 12:40, Ganapatrao Kulkarni wrote:
>> The current smmu-v3 driver does not enable PCI ATS for physical functions
>> of ATS capable End Points when booted in smmu bypass mode
>> (iommu.passthrough=1). This will not allow virtual functions to enable
>> ATS(even though EP supports it) while they are attached to a VM using
>> VFIO driver.
>>
>> This patch adds changes to enable ATS support for physical functions
>> in passthrough/bypass mode as well.
>>
>> Also, adding check to avoid disabling of ATS if it is not enabled,
>> to avoid unnecessary call-traces.
>>
>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 6d5df91c5c46..5a605cb5ccef 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2313,11 +2313,16 @@ static void arm_smmu_enable_ats(struct
>> arm_smmu_master *master)
>>   static void arm_smmu_disable_ats(struct arm_smmu_master *master)
>>   {
>>       struct arm_smmu_domain *smmu_domain = master->domain;
>> +    struct pci_dev *pdev;
>>       if (!master->ats_enabled)
>>           return;
>> -    pci_disable_ats(to_pci_dev(master->dev));
>> +    pdev = to_pci_dev(master->dev);
>> +
>> +    if (pdev->ats_enabled)
>
> If the master->ats_enabled check above passes when ATS isn't actually
> enabled, surely that's a bug?

IIUC, It means ATS feature is supported (just check for existence of ATS
extended capability and smmu capability) and not necessarily enabled.
Function pci_enable_ats(called by arm_smmu_enable_ats) enables the ATS
by setting bit 15 of ATS Control Register (Offset 06h).
If pci_enable_ats is not successful, it will not set dev->ats_enabled
flag. So calling pci_disable_ats later results in call-trace, if
dev->ats_enabled is not set.

Function arm_smmu_enable_ats already prints error message if ATS enable
is failed.

>
> Robin.
>
>> +        pci_disable_ats(pdev);
>> +
>>       /*
>>        * Ensure ATS is disabled at the endpoint before we issue the
>>        * ATC invalidation via the SMMU.
>> @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct
>> iommu_domain *domain, struct device *dev)
>>       master->domain = smmu_domain;
>> -    if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>> -        master->ats_enabled = arm_smmu_ats_supported(master);
>> +    master->ats_enabled = arm_smmu_ats_supported(master);
>>       arm_smmu_install_ste_for_dev(master);

Thanks,
Ganapat

2023-02-03 12:00:29

by Jean-Philippe Brucker

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
> The current smmu-v3 driver does not enable PCI ATS for physical functions
> of ATS capable End Points when booted in smmu bypass mode
> (iommu.passthrough=1). This will not allow virtual functions to enable
> ATS(even though EP supports it) while they are attached to a VM using
> VFIO driver.
>
> This patch adds changes to enable ATS support for physical functions
> in passthrough/bypass mode as well.
[...]
> @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>
> master->domain = smmu_domain;
>
> - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> - master->ats_enabled = arm_smmu_ats_supported(master);
> + master->ats_enabled = arm_smmu_ats_supported(master);

I should have added a comment for this. Only found the reason in an old
cover letter [1]:

"When no translation stages are enabled (0b100), ATS Translation Requests
(and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
F_TRANSL_FORBIDDEN."

(See 3.9.1.1 "Responses to ATS Translation Requests and Translated
transactions" and 5.2 "Stream Table Entry")

So I don't think we can enable ATS for bypass domains :/ The PF needs to
be in translated mode in that case.

I can send a patch adding the comment next cycle.

Thanks,
Jean

[1] https://lore.kernel.org/linux-iommu/[email protected]/


2023-02-03 12:05:25

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

On Fri, Feb 03, 2023 at 12:00:16PM +0000, Jean-Philippe Brucker wrote:
> On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
> > The current smmu-v3 driver does not enable PCI ATS for physical functions
> > of ATS capable End Points when booted in smmu bypass mode
> > (iommu.passthrough=1). This will not allow virtual functions to enable
> > ATS(even though EP supports it) while they are attached to a VM using
> > VFIO driver.
> >
> > This patch adds changes to enable ATS support for physical functions
> > in passthrough/bypass mode as well.
> [...]
> > @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >
> > master->domain = smmu_domain;
> >
> > - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> > - master->ats_enabled = arm_smmu_ats_supported(master);
> > + master->ats_enabled = arm_smmu_ats_supported(master);
>
> I should have added a comment for this. Only found the reason in an old
> cover letter [1]:
>
> "When no translation stages are enabled (0b100), ATS Translation Requests
> (and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
> EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
> Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
> F_TRANSL_FORBIDDEN."
>
> (See 3.9.1.1 "Responses to ATS Translation Requests and Translated
> transactions" and 5.2 "Stream Table Entry")
>
> So I don't think we can enable ATS for bypass domains :/ The PF needs to
> be in translated mode in that case.
>
> I can send a patch adding the comment next cycle.

Yes, please!

Will

2023-02-03 12:12:34

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

On 2023-02-03 10:44, Ganapatrao Kulkarni wrote:
>
>
> On 02-02-2023 06:52 pm, Robin Murphy wrote:
>> On 2023-02-02 12:40, Ganapatrao Kulkarni wrote:
>>> The current smmu-v3 driver does not enable PCI ATS for physical
>>> functions
>>> of ATS capable End Points when booted in smmu bypass mode
>>> (iommu.passthrough=1). This will not allow virtual functions to enable
>>> ATS(even though EP supports it) while they are attached to a VM using
>>> VFIO driver.
>>>
>>> This patch adds changes to enable ATS support for physical functions
>>> in passthrough/bypass mode as well.
>>>
>>> Also, adding check to avoid disabling of ATS if it is not enabled,
>>> to avoid unnecessary call-traces.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>>> ---
>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +++++++---
>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 6d5df91c5c46..5a605cb5ccef 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -2313,11 +2313,16 @@ static void arm_smmu_enable_ats(struct
>>> arm_smmu_master *master)
>>>   static void arm_smmu_disable_ats(struct arm_smmu_master *master)
>>>   {
>>>       struct arm_smmu_domain *smmu_domain = master->domain;
>>> +    struct pci_dev *pdev;
>>>       if (!master->ats_enabled)
>>>           return;
>>> -    pci_disable_ats(to_pci_dev(master->dev));
>>> +    pdev = to_pci_dev(master->dev);
>>> +
>>> +    if (pdev->ats_enabled)
>>
>> If the master->ats_enabled check above passes when ATS isn't actually
>> enabled, surely that's a bug?
>
> IIUC, It means ATS feature is supported (just check for existence of ATS
> extended capability and smmu capability) and not necessarily enabled.
> Function pci_enable_ats(called by arm_smmu_enable_ats) enables the ATS
> by setting bit 15 of ATS Control Register (Offset 06h).
> If pci_enable_ats is not successful, it will not set dev->ats_enabled
> flag. So calling pci_disable_ats later results in call-trace, if
> dev->ats_enabled is not set.

And like I say, that appears to be a bug, or at least something
deserving of improvement. If we *know* that enabling ATS failed, leaving
it hanging in some half-enabled state seems wrong. I know the PCI spec
says that functions must accept ATS invalidate requests even when ATS is
disabled, but that still doesn't make it a great idea for the driver to
spend time and effort sending them when it should know they are
unnecessarily (especially given the infamous 90-second maximum timeout).

> Function arm_smmu_enable_ats already prints error message if ATS enable
> is failed.

Printing a message hardly constitutes robust error handling...

Thanks,
Robin.

2023-02-06 17:20:23

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well



On 03-02-2023 05:30 pm, Jean-Philippe Brucker wrote:
> On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
>> The current smmu-v3 driver does not enable PCI ATS for physical functions
>> of ATS capable End Points when booted in smmu bypass mode
>> (iommu.passthrough=1). This will not allow virtual functions to enable
>> ATS(even though EP supports it) while they are attached to a VM using
>> VFIO driver.
>>
>> This patch adds changes to enable ATS support for physical functions
>> in passthrough/bypass mode as well.
> [...]
>> @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>
>> master->domain = smmu_domain;
>>
>> - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>> - master->ats_enabled = arm_smmu_ats_supported(master);
>> + master->ats_enabled = arm_smmu_ats_supported(master);
>
> I should have added a comment for this. Only found the reason in an old
> cover letter [1]:
>
> "When no translation stages are enabled (0b100), ATS Translation Requests
> (and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
> EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
> Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
> F_TRANSL_FORBIDDEN."
>
> (See 3.9.1.1 "Responses to ATS Translation Requests and Translated
> transactions" and 5.2 "Stream Table Entry")
>
> So I don't think we can enable ATS for bypass domains :/ The PF needs to
> be in translated mode in that case.

Are you intending to say smmu-v3 driver/spec will not support ATS to a
VF, if it's PF is in bypass?

>
> I can send a patch adding the comment next cycle.

I am more keen to know, how we enable ATS to a VF of ATS capable EP when
it's PF is in bypass?
or it is mandatory to have a PF also translated? then that should be
captured somewhere in documentation.


>
> Thanks,
> Jean
>
> [1] https://lore.kernel.org/linux-iommu/[email protected]/
>

Thanks,
Ganapat

2023-02-06 19:45:15

by Jean-Philippe Brucker

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

On Mon, Feb 06, 2023 at 10:50:00PM +0530, Ganapatrao Kulkarni wrote:
>
>
> On 03-02-2023 05:30 pm, Jean-Philippe Brucker wrote:
> > On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
> > > The current smmu-v3 driver does not enable PCI ATS for physical functions
> > > of ATS capable End Points when booted in smmu bypass mode
> > > (iommu.passthrough=1). This will not allow virtual functions to enable
> > > ATS(even though EP supports it) while they are attached to a VM using
> > > VFIO driver.
> > >
> > > This patch adds changes to enable ATS support for physical functions
> > > in passthrough/bypass mode as well.
> > [...]
> > > @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > > master->domain = smmu_domain;
> > > - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> > > - master->ats_enabled = arm_smmu_ats_supported(master);
> > > + master->ats_enabled = arm_smmu_ats_supported(master);
> >
> > I should have added a comment for this. Only found the reason in an old
> > cover letter [1]:
> >
> > "When no translation stages are enabled (0b100), ATS Translation Requests
> > (and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
> > EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
> > Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
> > F_TRANSL_FORBIDDEN."
> >
> > (See 3.9.1.1 "Responses to ATS Translation Requests and Translated
> > transactions" and 5.2 "Stream Table Entry")
> >
> > So I don't think we can enable ATS for bypass domains :/ The PF needs to
> > be in translated mode in that case.
>
> Are you intending to say smmu-v3 driver/spec will not support ATS to a VF,
> if it's PF is in bypass?

What I meant was that if, in order to enable the VF ATS capability, the
PCIe device requires to first enable the PF ATS capability, then given
that the SMMU does not allow enabling ATS for streams in bypass, we need
to enable translation on both PF and VF SMMU configurations.

But reading the PCIe spec, it looks like the capabilities are not
necessarily tied together:

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

That wording doesn't indicate that all implementations must allow enabling
the caps independently, only that they are allowed to do so. If a device
provides independent capabilities, then I don't think you need to enable
ATS in the PF.

Then the ATS Control Register seems to contradict this with the STU field:

"For VFs, this field must be hardwired to Zero. The associated PF's value
applies." 10.5.1.3 ATS Control Register (Offset 06h)

So pci_enable_ats() requires that pdev->ats_stu is set in order to enable
ATS on the VF:

/*
* Note that enabling ATS on a VF fails unless it's already enabled
* with the same STU on the PF.
*/
...
if (dev->is_virtfn) {
pdev = pci_physfn(dev);
if (pdev->ats_stu != ps)
return -EINVAL;

But I suspect it's done this way only because it's easier to implement.
The spec doesn't require the PF ATS capability to be enabled, it just says
that the PF's STU value counts as the VF's one. It looks like we're
allowed to enable ATS on the VF without enabling it on the PF, right?
If you rework the PCI driver to only write the PF's STU without enabling
the capability, then you could enable it in the VF.

Thanks,
Jean

>
> >
> > I can send a patch adding the comment next cycle.
>
> I am more keen to know, how we enable ATS to a VF of ATS capable EP when
> it's PF is in bypass?
> or it is mandatory to have a PF also translated? then that should be
> captured somewhere in documentation.
>
>
> >
> > Thanks,
> > Jean
> >
> > [1] https://lore.kernel.org/linux-iommu/[email protected]/
> >
>
> Thanks,
> Ganapat

2023-02-07 09:51:19

by Ganapatrao Kulkarni

[permalink] [raw]
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well



On 07-02-2023 01:15 am, Jean-Philippe Brucker wrote:
> On Mon, Feb 06, 2023 at 10:50:00PM +0530, Ganapatrao Kulkarni wrote:
>>
>>
>> On 03-02-2023 05:30 pm, Jean-Philippe Brucker wrote:
>>> On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
>>>> The current smmu-v3 driver does not enable PCI ATS for physical functions
>>>> of ATS capable End Points when booted in smmu bypass mode
>>>> (iommu.passthrough=1). This will not allow virtual functions to enable
>>>> ATS(even though EP supports it) while they are attached to a VM using
>>>> VFIO driver.
>>>>
>>>> This patch adds changes to enable ATS support for physical functions
>>>> in passthrough/bypass mode as well.
>>> [...]
>>>> @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>> master->domain = smmu_domain;
>>>> - if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>>>> - master->ats_enabled = arm_smmu_ats_supported(master);
>>>> + master->ats_enabled = arm_smmu_ats_supported(master);
>>>
>>> I should have added a comment for this. Only found the reason in an old
>>> cover letter [1]:
>>>
>>> "When no translation stages are enabled (0b100), ATS Translation Requests
>>> (and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
>>> EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
>>> Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
>>> F_TRANSL_FORBIDDEN."
>>>
>>> (See 3.9.1.1 "Responses to ATS Translation Requests and Translated
>>> transactions" and 5.2 "Stream Table Entry")
>>>
>>> So I don't think we can enable ATS for bypass domains :/ The PF needs to
>>> be in translated mode in that case.
>>
>> Are you intending to say smmu-v3 driver/spec will not support ATS to a VF,
>> if it's PF is in bypass?
>
> What I meant was that if, in order to enable the VF ATS capability, the
> PCIe device requires to first enable the PF ATS capability, then given
> that the SMMU does not allow enabling ATS for streams in bypass, we need
> to enable translation on both PF and VF SMMU configurations.
>
> But reading the PCIe spec, it looks like the capabilities are not
> necessarily tied together:
>
> "The ATS Capabilities in VFs and their associated PFs are permitted to be
> enabled independently." 10.5.1 ATS Extended Capability
>
> That wording doesn't indicate that all implementations must allow enabling
> the caps independently, only that they are allowed to do so. If a device
> provides independent capabilities, then I don't think you need to enable
> ATS in the PF.
Yes makes sense, thanks.
>
> Then the ATS Control Register seems to contradict this with the STU field:
>
> "For VFs, this field must be hardwired to Zero. The associated PF's value
> applies." 10.5.1.3 ATS Control Register (Offset 06h)
>
> So pci_enable_ats() requires that pdev->ats_stu is set in order to enable
> ATS on the VF:
>
> /*
> * Note that enabling ATS on a VF fails unless it's already enabled
> * with the same STU on the PF.
> */

This comment was misleading!
> ...
> if (dev->is_virtfn) {
> pdev = pci_physfn(dev);
> if (pdev->ats_stu != ps)
> return -EINVAL;
>
> But I suspect it's done this way only because it's easier to implement.
> The spec doesn't require the PF ATS capability to be enabled, it just says
> that the PF's STU value counts as the VF's one. It looks like we're
> allowed to enable ATS on the VF without enabling it on the PF, right?

OK thanks, as per spec ATS enable on VF and PF are independent. only STU
and Capability are hardwired to be same.

> If you rework the PCI driver to only write the PF's STU without enabling
> the capability, then you could enable it in the VF.

Makes sense, It should be feasible to write to STU of a PF without
enabling it.

Below diff is more appropriate fix for this case.

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..f064c2be8593 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -74,6 +74,15 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
ctrl = PCI_ATS_CTRL_ENABLE;
if (dev->is_virtfn) {
pdev = pci_physfn(dev);
+
+ if (!pdev->ats_enabled) {
+ u16 ctrl2;
+
+ 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 {


Thanks,
Ganapat


>
> Thanks,
> Jean
>
>>
>>>
>>> I can send a patch adding the comment next cycle.
>>
>> I am more keen to know, how we enable ATS to a VF of ATS capable EP when
>> it's PF is in bypass?
>> or it is mandatory to have a PF also translated? then that should be
>> captured somewhere in documentation.
>>
>>
>>>
>>> Thanks,
>>> Jean
>>>
>>> [1] https://lore.kernel.org/linux-iommu/[email protected]/
>>>
>>
>> Thanks,
>> Ganapat