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

[sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> 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@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Keith Busch <keith.busch@intel.com>
>> Suggested-by: Ashok Raj <ashok.raj@intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   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