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

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

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