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

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

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