Messages in this thread | | | Date | Thu, 16 Feb 2023 17:06:49 +0530 | Subject | Re: [PATCH] PCI/ATS: Allow to enable ATS on VFs even if it is not enabled on PF | From | Ganapatrao Kulkarni <> |
| |
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
| |