On 2/27/23 6:53 PM, Ganapatrao Kulkarni wrote: > > > On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote: >> Hi, >> >> On 2/27/23 5:21 AM, 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. >> >> Why not enable ATS in PF before enabling it in VF? Just updating STU of >> PF and not enabling it seem odd. > > More details are in PATCH 0/2 and 2/2. > > Also, This was discussed at > https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/ > I agree with Bjorn's comments. It is incorrect to directly configure PF registers from VF enable function. It is a buggy fix. My question is, why not ensure PF ATS is configured and enabled before enabling ATS for VF. Anyway, PF device will be enumerated before VF, right? >> >>> >>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> >>> --- >>> drivers/pci/ats.c | 32 ++++++++++++++++++++++++++++++-- >>> include/linux/pci-ats.h | 1 + >>> 2 files changed, 31 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c >>> index f9cc2e10b676..70e1982efdb4 100644 >>> --- a/drivers/pci/ats.c >>> +++ b/drivers/pci/ats.c >>> @@ -46,6 +46,34 @@ 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; >>> + >>> + if (!pci_ats_supported(dev)) >>> + return -EINVAL; >>> + >>> + if (ps < PCI_ATS_MIN_STU) >>> + return -EINVAL; >>> + >>> + dev->ats_stu = ps; >>> + ctrl = PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU); >>> + pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl); >> >> If you just want to update the STU, don't overwrite other fields. > > Can be read modify write, but felt not necessary, since all other fields are at default value zero. It may not always be true. If there is a case where AMAD and AMAE attribute values are configured independently, then your change can overwrite it. IMO, since your intention is to update STU, I recommend just updating it. > >> >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure); >>> + >>> /** >>> * pci_enable_ats - enable the ATS capability >>> * @dev: the PCI device >>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps) >>> return -EINVAL; >>> /* >>> - * Note that enabling ATS on a VF fails unless it's already enabled >>> - * with the same STU on the PF. >>> + * Note that enabling ATS on a VF fails unless it's already >>> + * configured with the same STU on the PF. >>> */ >>> ctrl = PCI_ATS_CTRL_ENABLE; >>> if (dev->is_virtfn) { >>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h >>> index df54cd5b15db..9b40eb555124 100644 >>> --- a/include/linux/pci-ats.h >>> +++ b/include/linux/pci-ats.h >>> @@ -8,6 +8,7 @@ >>> /* Address Translation Service */ >>> bool pci_ats_supported(struct pci_dev *dev); >>> int pci_enable_ats(struct pci_dev *dev, int ps); >>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps); >> >> What about dummy declaration for !CONFIG_PCI_ATS case? > > Thanks, I overlooked else case. >> >>> void pci_disable_ats(struct pci_dev *dev); >>> int pci_ats_queue_depth(struct pci_dev *dev); >>> int pci_ats_page_aligned(struct pci_dev *dev); >> > > > Thanks, > Ganapat -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
