Re: [PATCH v2 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF

[Sathyanarayanan Kuppuswamy]

On 3/14/23 2:17 PM, Bjorn Helgaas wrote:
> On Tue, Mar 14, 2023 at 11:12:11AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> On 3/14/23 10:10 AM, Bjorn Helgaas wrote:
>>> On Tue, Mar 14, 2023 at 09:50:06AM -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> On 3/14/23 9:02 AM, Bjorn Helgaas wrote:
>>>>> On Tue, Mar 14, 2023 at 08:06:07PM +0530, Ganapatrao Kulkarni wrote:
>>>>>> On 14-03-2023 06:22 pm, Sathyanarayanan Kuppuswamy wrote:
>>>>>>> On 3/14/23 3:08 AM, Ganapatrao Kulkarni wrote:
>>>>>>>> On 14-03-2023 04:00 am, Sathyanarayanan Kuppuswamy wrote:
>>>>>>>>> On 3/13/23 2:12 PM, Bjorn Helgaas wrote:
>>>>>>>>>> On Mon, Feb 27, 2023 at 08:21:36PM -0800, Ganapatrao Kulkarni wrote:
>>>>>>>>>>> As per PCI specification (PCI Express Base Specification
>>>>>>>>>>> Revision 6.0, Section 10.5) both PF and VFs of a PCI EP
>>>>>>>>>>> are permitted to be enabled independently for ATS
>>>>>>>>>>> capability, however the STU(Smallest Translation Unit) is
>>>>>>>>>>> shared between PF and VFs. For VFs, it is hardwired to
>>>>>>>>>>> Zero and the associated PF's value applies to VFs.
>>>>>>>>>>>
>>>>>>>>>>> In the current code, the STU is being configured while
>>>>>>>>>>> enabling the PF ATS.  Hence, it is not able to enable ATS
>>>>>>>>>>> for VFs, if it is not enabled on the associated PF
>>>>>>>>>>> already.
>>>>>>>>>>>
>>>>>>>>>>> Adding a function pci_ats_stu_configure(), which can be
>>>>>>>>>>> called to configure the STU during PF enumeration.  Latter
>>>>>>>>>>> enumerations of VFs can successfully enable ATS
>>>>>>>>>>> independently.
>>>>>
>>>>>>>>>>> @@ -46,6 +46,35 @@ bool pci_ats_supported(struct pci_dev *dev)
>>>>>>>>>>>    }
>>>>>>>>>>>    EXPORT_SYMBOL_GPL(pci_ats_supported);
>>>>>>>>>>>    +/**
>>>>>>>>>>> + * pci_ats_stu_configure - Configure STU of a PF.
>>>>>>>>>>> + * @dev: the PCI device
>>>>>>>>>>> + * @ps: the IOMMU page shift
>>>>>>>>>>> + *
>>>>>>>>>>> + * Returns 0 on success, or negative on failure.
>>>>>>>>>>> + */
>>>>>>>>>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>>>>>>>>>> +{
>>>>>>>>>>> +    u16 ctrl;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (dev->ats_enabled || dev->is_virtfn)
>>>>>>>>>>> +        return 0;
>>>>>>>>>>
>>>>>>>>>> I might return an error for the VF case on the assumption
>>>>>>>>>> that it's likely an error in the caller.  I guess one could
>>>>>>>>>> argue that it simplifies the caller if it doesn't have to
>>>>>>>>>> check for PF vs VF.  But the fact that STU is shared between
>>>>>>>>>> PF and VFs is an important part of understanding how ATS
>>>>>>>>>> works, so the caller should be aware of the distinction
>>>>>>>>>> anyway.
>>>>>>>>>
>>>>>>>>> I have already asked this question. But let me repeat it.
>>>>>>>>>
>>>>>>>>> We don't have any checks for the PF case here. That means you
>>>>>>>>> can re-configure the STU as many times as you want until ATS
>>>>>>>>> is enabled in PF. So, if there are active VFs which uses this
>>>>>>>>> STU, can PF re-configure the STU at will?
>>>>>>>>
>>>>>>>> IMO, Since STU is shared, programming it multiple times is not expected from callers code do it, however we can add below check to allow to program STU once from a PF.
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>>>>>> index 1611bfa1d5da..f7bb01068e18 100644
>>>>>>>> --- a/drivers/pci/ats.c
>>>>>>>> +++ b/drivers/pci/ats.c
>>>>>>>> @@ -60,6 +60,10 @@ int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>>>>>>>          if (dev->ats_enabled || dev->is_virtfn)
>>>>>>>>                  return 0;
>>>>>>>>
>>>>>>>> +       /* Configured already */
>>>>>>>> +       if (dev->ats_stu)
>>>>>>>> +               return 0;
>>>>>>>
>>>>>>> Theoretically, you can re-configure STU as long as no one is using
>>>>>>> it. Instead of this check, is there a way to check whether there
>>>>>>> are active VMs which enables ATS?
>>>>>>
>>>>>> Yes I agree, there is no limitation on how many times you write STU
>>>>>> bits, but practically it is happening while PF is enumerated.
>>>>>>
>>>>>> The usage of function pci_ats_stu_configure is almost
>>>>>> similar(subset) to pci_enable_ats and only difference is one does
>>>>>> ATS enable + STU program and another does only STU program.
>>>>>
>>>>> What would you think of removing the STU update feature from
>>>>> pci_enable_ats() so it always fails if pci_ats_stu_configure() has not
>>>>> been called, even when called on the PF, e.g.,
>>>>>
>>>>>   if (ps != pci_physfn(dev)->ats_stu)
>>>>>     return -EINVAL;
>>>>
>>>> If we are removing the STU update from pci_enable_ats(), why
>>>> even allow passing "ps (page shift)" parameter? IMO, we can assume that
>>>> for STU reconfigure, users will call pci_ats_stu_configure().
>>>
>>> The reason to pass "ps" would be to verify that the STU the caller
>>> plans to use matches the actual STU.
>>
>> Do we really need to verify it? My thinking is, by introducing
>> pci_ats_stu_configure() we are already trying to decouple the STU config
>> from pci_enable_ats(). So why again check for it when enabling ATS?
> 
> Yeah, maybe we don't need to.  I was thinking that STU would be
> configured by the host, while the caller of pci_enable_ats() for a VF
> might be in a guest, but I guess that's not the case, right?

I think the idea is to configure the STU during PF enumeration in the
host. So, when VF enables ATS in the VM, it does not need to worry about
the STU configuration. So I don't think we need to re-check the STU in
the VF case.

> 
> Bjorn

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer