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

Ganapatrao Kulkarni
2023-02-16T11:38:06Z

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