Re: [PATCH v1 1/2] PCI: ATS: Add function to check ATS page aligned request status.

[sathyanarayanan kuppuswamy]

On 2/7/19 12:07 PM, Bjorn Helgaas wrote:
> Hi Kuppuswamy,
>
> Previous changes to ats.c used subject lines starting with just
> "PCI:".
>
> I think it does make sense to include "ATS", but please do it in
> the way we do it for other PCI features, e.g.,
>
>    PCI/ATS: Add pci_ats_page_aligned() interface
Got it. I will follow PCI/ATS format.
> On Thu, Feb 07, 2019 at 10:41:13AM -0800, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> >
> > Add a new function to return the status of ATS page aligned request
> > bit in ATS capability register. This function will be used by
> > drivers like IOMMU, if it is required to enforce page-aligned
> > requests in ATS.
> "return the Page Aligned Request bit in the ATS Capability Register"
>
> This is just to make the terminology match the PCIe spec exactly so
> it's easier to look up.
Will fix it in next version.
> > Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
> > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > Cc: Keith Busch <keith.busch@xxxxxxxxx>
> > Suggested-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/pci/ats.c             | 22 ++++++++++++++++++++++
> >   include/linux/pci.h           |  2 ++
> >   include/uapi/linux/pci_regs.h |  1 +
> >   3 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index 5b78f3b1b918..7d14b9a1981e 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -142,6 +142,28 @@ int pci_ats_queue_depth(struct pci_dev *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
> >   
> > +/**
> > + * pci_ats_page_aligned - Return ATS page aligned request bit status.
> Capitalize name of bit as above.
Agreed.
> > + * @pdev: the PCI device
> > + *
> > + * Returns  value > 0 if address is aligned or 0 otherwise.
> s/  / /
>
> "if Untranslated Addresses generated by the device are always
> aligned or ..."
>
> > + *
> > + * As per PCI spec, If page aligned request bit is set, it indicates
> > + * the untranslated address is always aligned to a 4096 byte boundary.
> "Per PCIe r4.0, sec 10.5.1.2, if the Page Aligned Request bit,
> Untranslated Addresses generated by the device are always aligned to a
> 4096 byte boundary."
>
> > + */
> > +int pci_ats_page_aligned(struct pci_dev *pdev)
> > +{
> > +	u16 cap;
> > +
> > +	if (!pdev->ats_cap)
> > +		return 0;
> > +
> > +	pci_read_config_word(pdev, pdev->ats_cap + PCI_ATS_CAP, &cap);
> > +
> > +	return PCI_ATS_CAP_PAGE_ALIGNED(cap);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ats_page_aligned);
> > +
> >   #ifdef CONFIG_PCI_PRI
> >   /**
> >    * pci_enable_pri - Enable PRI capability
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 65f1d8c2f082..9724a8c0496b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1524,11 +1524,13 @@ void pci_ats_init(struct pci_dev *dev);
> >   int pci_enable_ats(struct pci_dev *dev, int ps);
> >   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);
> >   #else
> >   static inline void pci_ats_init(struct pci_dev *d) { }
> >   static inline int pci_enable_ats(struct pci_dev *d, int ps) { return -ENODEV; }
> >   static inline void pci_disable_ats(struct pci_dev *d) { }
> >   static inline int pci_ats_queue_depth(struct pci_dev *d) { return -ENODEV; }
> > +static inline int pci_ats_page_aligned(struct pci_dev *dev) { return 0; }
> >   #endif
> >   
> >   #ifdef CONFIG_PCIE_PTM
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index e1e9888c85e6..d42a759867b8 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -866,6 +866,7 @@
> >   #define PCI_ATS_CAP		0x04	/* ATS Capability Register */
> >   #define  PCI_ATS_CAP_QDEP(x)	((x) & 0x1f)	/* Invalidate Queue Depth */
> >   #define  PCI_ATS_MAX_QDEP	32	/* Max Invalidate Queue Depth */
> > +#define  PCI_ATS_CAP_PAGE_ALIGNED(x)	0x0020 /* Page Aligned Request */
> This is wrong because it *always* returns "true", regardless of the
> value of the ATS Capability register.
>
> I would prefer this:
>
>    #define  PCI_ATS_CAP_PAGE_ALIGNED   0x0020
Good catch. it looks like I messed it when I did some cleanup. Sorry 
about it. Initially it was (x & 0x0020).
> and then test it like this:
>
>    if (cap & PCI_ATS_CAP_PAGE_ALIGNED)
>      return 1;
>    return 0;
Ok.
> >   #define PCI_ATS_CTRL		0x06	/* ATS Control Register */
> >   #define  PCI_ATS_CTRL_ENABLE	0x8000	/* ATS Enable */
> >   #define  PCI_ATS_CTRL_STU(x)	((x) & 0x1f)	/* Smallest Translation Unit */
> > --
> > 2.20.1
--
Sathyanarayanan Kuppuswamy
Linux kernel developer