Message-ID: <2bffb6ca-e5f5-1a24-a47a-b0238e184ad6@os.amperecomputing.com>
Date:   Tue, 7 Feb 2023 15:20:59 +0530
From:   Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        iommu@...ts.linux.dev, robin.murphy@....com, will@...nel.org,
        joro@...tes.org, darren@...amperecomputing.com,
        scott@...amperecomputing.com
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/20190409165245.26500-1-jean-philippe.brucker@arm.com/
>>>
>>
>> Thanks,
>> Ganapat

Powered by blists - more mailing lists