| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3cd33a40-8653-fe01-dd99-cf9b62b4327d@linux.intel.com>
Date: Wed, 9 Oct 2019 15:51:47 -0700
From: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
ashok.raj@...el.com, keith.busch@...el.com
Subject: Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required()
function
Hi Bjorn,
Thanks for the review.
On 10/9/19 3:30 PM, Bjorn Helgaas wrote:
> On Mon, Oct 07, 2019 at 04:32:42PM -0700, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>>
>> Currently, pci_prg_resp_pasid_required() function reads the
>> PASID Required bit status from register every time we call
>> the function. Since PASID Required bit is a read-only value,
>> instead of reading it from register every time, read it once and
>> cache it in struct pci_dev.
>>
>> Also, since we are caching PASID Required bit in pci_pri_init()
>> function, move the caching of PRI Capability check result to the same
>> function. This will group all PRI related caching at one place.
>>
>> Since "pasid_required" structure member is protected by CONFIG_PRI,
>> its users should also be protected by same #ifdef. So correct the #ifdef
>> dependency of pci_prg_resp_pasid_required() function.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>> Cc: Ashok Raj <ashok.raj@...el.com>
>> Cc: Keith Busch <keith.busch@...el.com>
>> ---
>> drivers/pci/ats.c | 50 ++++++++++++++++++++++++---------------------
>> include/linux/pci.h | 1 +
>> 2 files changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index cb4f62da7b8a..2b5df5ea208f 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -16,6 +16,24 @@
>>
>> #include "pci.h"
>>
>> +static void pci_pri_init(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_PRI
>> + int pos;
>> + u16 status;
>> +
>> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>> + if (!pos)
>> + return;
>> +
>> + pdev->pri_cap = pos;
>> +
>> + pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
>> + if (status & PCI_PRI_STATUS_PASID)
>> + pdev->pasid_required = 1;
>> +#endif
>> +}
> I like this patch a lot but:
>
> 1) You started out with a pci_pri_init(), and I screwed up when I
> suggested that you remove it. I think it makes a lot of sense to have
> it and call it directly from pci_init_capabilities() just like we do
> for other capabilities.
Yes, we could do that. But, I thought may be there is a history for grouping
all PRI/PASID related code in ats.c. That's why I did not move PRI/PASID
related init code outside ats.c. If its not an issue, having it in
pci_init_capabiliies() is
more appropriate.
>
> 2) The PCI_PRI/PCI_PASID #ifdef thing is still a mess. Despite the
> fact that its name contains "pasid", pci_prg_resp_pasid_required()
> only looks at the PRI capability, so I think it should be under #ifdef
> CONFIG_PCI_PRI along with the other PRI stuff.
>
> 3) I'm not sure why pci_prg_resp_pasid_required() was under
> CONFIG_PCI_PASID, but it might be related to the fact that it's called
> from code in intel-iommu.c where CONFIG_PCI_PASID is always defined,
> but CONFIG_PCI_PRI may not be.
Main reason is, this function will only be used if PASID is enabled.
Please check the following
code snippet from intel-iommu.c
1418 if (info->pasid_supported && !pci_enable_pasid(pdev,
info->pasid_supported & ~1))
1419 info->pasid_enabled = 1;
1420
1421 if (info->pri_supported &&
1422 (info->pasid_enabled ?
pci_prg_resp_pasid_required(pdev) : 1) &&
1423 !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
1424 info->pri_enabled = 1;
After looking at this code again, since there is no compile time
dependency, we could just move
the definition of pri_prg_resp_pasid_required() function under
CONFIG_PCI_PRI. it should not be a
problem.
> I think this is an intel-iommu Kconfig
> defect. I'll post patches to change the Kconfig and to move
> pci_prg_resp_pasid_required() under CONFIG_PCI_PRI.
>
> So I fiddled with all this and updated my pci/virtualization branch
> with the result.
>
> The interdiff from the v8 series is below. This includes the
> intel-iommu Kconfig and pci_prg_resp_pasid_required() changes (which I
> haven't posted yet), and the branch includes some unrelated follow-on
> patches (which I also haven't posted yet). Let me know what you
> think.
It looks good to me. Please go ahead.
>
> Bjorn
>
>> void pci_ats_init(struct pci_dev *dev)
>> {
>> int pos;
>> @@ -28,6 +46,8 @@ void pci_ats_init(struct pci_dev *dev)
>> return;
>>
>> dev->ats_cap = pos;
>> +
>> + pci_pri_init(dev);
>> }
>>
>> /**
>> @@ -185,12 +205,8 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
>> if (WARN_ON(pdev->pri_enabled))
>> return -EBUSY;
>>
>> - if (!pri) {
>> - pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>> - if (!pri)
>> - return -EINVAL;
>> - pdev->pri_cap = pri;
>> - }
>> + if (!pri)
>> + return -EINVAL;
>>
>> pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
>> if (!(status & PCI_PRI_STATUS_STOPPED))
>> @@ -425,6 +441,7 @@ int pci_pasid_features(struct pci_dev *pdev)
>> }
>> EXPORT_SYMBOL_GPL(pci_pasid_features);
>>
>> +#ifdef CONFIG_PCI_PRI
>> /**
>> * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
>> * status.
>> @@ -432,31 +449,18 @@ EXPORT_SYMBOL_GPL(pci_pasid_features);
>> *
>> * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
>> *
>> - * Even though the PRG response PASID status is read from PRI Status
>> - * Register, since this API will mainly be used by PASID users, this
>> - * function is defined within #ifdef CONFIG_PCI_PASID instead of
>> - * CONFIG_PCI_PRI.
>> + * Since this API has dependency on both PRI and PASID, protect it
>> + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID.
>> */
>> int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>> {
>> - u16 status;
>> - int pri;
>> -
>> if (pdev->is_virtfn)
>> pdev = pci_physfn(pdev);
>>
>> - pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>> - if (!pri)
>> - return 0;
>> -
>> - pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
>> -
>> - if (status & PCI_PRI_STATUS_PASID)
>> - return 1;
>> -
>> - return 0;
>> + return pdev->pasid_required;
>> }
>> EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>> +#endif /* CONFIG_PCI_PRI */
>>
>> #define PASID_NUMBER_SHIFT 8
>> #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 6542100bd2dd..f1131fee7fcd 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -456,6 +456,7 @@ struct pci_dev {
>> #ifdef CONFIG_PCI_PRI
>> u16 pri_cap; /* PRI Capability offset */
>> u32 pri_reqs_alloc; /* Number of PRI requests allocated */
>> + unsigned int pasid_required:1; /* PRG Response PASID Required bit status */
>> #endif
>> #ifdef CONFIG_PCI_PASID
>> u16 pasid_cap; /* PASID Capability offset */
>> --
>> 2.21.0
>>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index e3842eabcfdd..b183c9f916b0 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -207,6 +207,7 @@ config INTEL_IOMMU_SVM
> bool "Support for Shared Virtual Memory with Intel IOMMU"
> depends on INTEL_IOMMU && X86
> select PCI_PASID
> + select PCI_PRI
> select MMU_NOTIFIER
> help
> Shared Virtual Memory (SVM) provides a facility for devices
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index cb4f62da7b8a..76ae518d55f4 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -159,6 +159,20 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
> EXPORT_SYMBOL_GPL(pci_ats_page_aligned);
>
> #ifdef CONFIG_PCI_PRI
> +void pci_pri_init(struct pci_dev *pdev)
> +{
> + u16 status;
> +
> + pdev->pri_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> +
> + if (!pdev->pri_cap)
> + return;
> +
> + pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_STATUS, &status);
> + if (status & PCI_PRI_STATUS_PASID)
> + pdev->pasid_required = 1;
> +}
> +
> /**
> * pci_enable_pri - Enable PRI capability
> * @ pdev: PCI device structure
> @@ -185,12 +199,8 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
> if (WARN_ON(pdev->pri_enabled))
> return -EBUSY;
>
> - if (!pri) {
> - pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> - if (!pri)
> - return -EINVAL;
> - pdev->pri_cap = pri;
> - }
> + if (!pri)
> + return -EINVAL;
>
> pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
> if (!(status & PCI_PRI_STATUS_STOPPED))
> @@ -290,9 +300,30 @@ int pci_reset_pri(struct pci_dev *pdev)
> return 0;
> }
> EXPORT_SYMBOL_GPL(pci_reset_pri);
> +
> +/**
> + * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
> + * status.
> + * @pdev: PCI device structure
> + *
> + * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
> + */
> +int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> +{
> + if (pdev->is_virtfn)
> + pdev = pci_physfn(pdev);
> +
> + return pdev->pasid_required;
> +}
> +EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> #endif /* CONFIG_PCI_PRI */
>
> #ifdef CONFIG_PCI_PASID
> +void pci_pasid_init(struct pci_dev *pdev)
> +{
> + pdev->pasid_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
> +}
> +
> /**
> * pci_enable_pasid - Enable the PASID capability
> * @pdev: PCI device structure
> @@ -323,12 +354,8 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
> if (!pdev->eetlp_prefix_path)
> return -EINVAL;
>
> - if (!pasid) {
> - pasid = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
> - if (!pasid)
> - return -EINVAL;
> - pdev->pasid_cap = pasid;
> - }
> + if (!pasid)
> + return -EINVAL;
>
> pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
> supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
> @@ -425,39 +452,6 @@ int pci_pasid_features(struct pci_dev *pdev)
> }
> EXPORT_SYMBOL_GPL(pci_pasid_features);
>
> -/**
> - * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
> - * status.
> - * @pdev: PCI device structure
> - *
> - * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
> - *
> - * Even though the PRG response PASID status is read from PRI Status
> - * Register, since this API will mainly be used by PASID users, this
> - * function is defined within #ifdef CONFIG_PCI_PASID instead of
> - * CONFIG_PCI_PRI.
> - */
> -int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> -{
> - u16 status;
> - int pri;
> -
> - if (pdev->is_virtfn)
> - pdev = pci_physfn(pdev);
> -
> - pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
> - if (!pri)
> - return 0;
> -
> - pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
> -
> - if (status & PCI_PRI_STATUS_PASID)
> - return 1;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> -
> #define PASID_NUMBER_SHIFT 8
> #define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
> /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3f6947ee3324..ae84d28ba03a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -456,6 +456,18 @@ static inline void pci_ats_init(struct pci_dev *d) { }
> static inline void pci_restore_ats_state(struct pci_dev *dev) { }
> #endif /* CONFIG_PCI_ATS */
>
> +#ifdef CONFIG_PCI_PRI
> +void pci_pri_init(struct pci_dev *dev);
> +#else
> +static inline void pci_pri_init(struct pci_dev *dev) { }
> +#endif
> +
> +#ifdef CONFIG_PCI_PASID
> +void pci_pasid_init(struct pci_dev *dev);
> +#else
> +static inline void pci_pasid_init(struct pci_dev *dev) { }
> +#endif
> +
> #ifdef CONFIG_PCI_IOV
> int pci_iov_init(struct pci_dev *dev);
> void pci_iov_release(struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3d5271a7a849..df2b77866f3b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2324,6 +2324,12 @@ static void pci_init_capabilities(struct pci_dev *dev)
> /* Address Translation Services */
> pci_ats_init(dev);
>
> + /* Page Request Interface */
> + pci_pri_init(dev);
> +
> + /* Process Address Space ID */
> + pci_pasid_init(dev);
> +
> /* Enable ACS P2P upstream forwarding */
> pci_enable_acs(dev);
>
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index 1ebb88e7c184..a7a2b3d94fcc 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -10,6 +10,7 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
> void pci_disable_pri(struct pci_dev *pdev);
> void pci_restore_pri_state(struct pci_dev *pdev);
> int pci_reset_pri(struct pci_dev *pdev);
> +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>
> #else /* CONFIG_PCI_PRI */
>
> @@ -31,6 +32,10 @@ static inline int pci_reset_pri(struct pci_dev *pdev)
> return -ENODEV;
> }
>
> +static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> +{
> + return 0;
> +}
> #endif /* CONFIG_PCI_PRI */
>
> #ifdef CONFIG_PCI_PASID
> @@ -40,7 +45,6 @@ void pci_disable_pasid(struct pci_dev *pdev);
> void pci_restore_pasid_state(struct pci_dev *pdev);
> int pci_pasid_features(struct pci_dev *pdev);
> int pci_max_pasids(struct pci_dev *pdev);
> -int pci_prg_resp_pasid_required(struct pci_dev *pdev);
>
> #else /* CONFIG_PCI_PASID */
>
> @@ -66,11 +70,6 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
> {
> return -EINVAL;
> }
> -
> -static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> -{
> - return 0;
> -}
> #endif /* CONFIG_PCI_PASID */
>
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6542100bd2dd..64d35e730fab 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -456,6 +456,7 @@ struct pci_dev {
> #ifdef CONFIG_PCI_PRI
> u16 pri_cap; /* PRI Capability offset */
> u32 pri_reqs_alloc; /* Number of PRI requests allocated */
> + unsigned int pasid_required:1; /* PRG Response PASID Required */
> #endif
> #ifdef CONFIG_PCI_PASID
> u16 pasid_cap; /* PASID Capability offset */
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
Powered by blists - more mailing lists