diff mbox series

iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well

Message ID 20230202124053.848792-1-gankulkarni@os.amperecomputing.com
State New
Headers
Series iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well |

Commit Message

Ganapatrao Kulkarni Feb. 2, 2023, 12:40 p.m. UTC 
  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 <gankulkarni@os.amperecomputing.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Robin Murphy Feb. 2, 2023, 1:22 p.m. UTC | #1 
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 <gankulkarni@os.amperecomputing.com>
> ---
>   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);
>
Ganapatrao Kulkarni Feb. 3, 2023, 10:44 a.m. UTC | #2 
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 <gankulkarni@os.amperecomputing.com>
>> ---
>>   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
Jean-Philippe Brucker Feb. 3, 2023, noon UTC | #3 
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/20190409165245.26500-1-jean-philippe.brucker@arm.com/
Will Deacon Feb. 3, 2023, 12:05 p.m. UTC | #4 
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
Robin Murphy Feb. 3, 2023, 12:12 p.m. UTC | #5 
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 <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   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.
Ganapatrao Kulkarni Feb. 6, 2023, 5:20 p.m. UTC | #6 
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/20190409165245.26500-1-jean-philippe.brucker@arm.com/
> 

Thanks,
Ganapat
Jean-Philippe Brucker Feb. 6, 2023, 7:45 p.m. UTC | #7 
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/20190409165245.26500-1-jean-philippe.brucker@arm.com/
> > 
> 
> Thanks,
> Ganapat
Ganapatrao Kulkarni Feb. 7, 2023, 9:50 a.m. UTC | #8 
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/20190409165245.26500-1-jean-philippe.brucker@arm.com/
>>>
>>
>> Thanks,
>> Ganapat
diff mbox series

Patch

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);