* [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function @ 2019-10-07 23:32 sathyanarayanan.kuppuswamy 2019-10-09 22:30 ` Bjorn Helgaas 0 siblings, 1 reply; 3+ messages in thread From: sathyanarayanan.kuppuswamy @ 2019-10-07 23:32 UTC (permalink / raw) To: bhelgaas Cc: linux-pci, linux-kernel, ashok.raj, keith.busch, sathyanarayanan.kuppuswamy From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Cc: Keith Busch <keith.busch@intel.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 +} + 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function 2019-10-07 23:32 [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function sathyanarayanan.kuppuswamy @ 2019-10-09 22:30 ` Bjorn Helgaas 2019-10-09 22:51 ` Kuppuswamy Sathyanarayanan 0 siblings, 1 reply; 3+ messages in thread From: Bjorn Helgaas @ 2019-10-09 22:30 UTC (permalink / raw) To: sathyanarayanan.kuppuswamy Cc: linux-pci, linux-kernel, ashok.raj, keith.busch On Mon, Oct 07, 2019 at 04:32:42PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com> > Cc: Ashok Raj <ashok.raj@intel.com> > Cc: Keith Busch <keith.busch@intel.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. 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. 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. 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 */ ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function 2019-10-09 22:30 ` Bjorn Helgaas @ 2019-10-09 22:51 ` Kuppuswamy Sathyanarayanan 0 siblings, 0 replies; 3+ messages in thread From: Kuppuswamy Sathyanarayanan @ 2019-10-09 22:51 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, ashok.raj, keith.busch 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@linux.intel.com wrote: >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com> >> Cc: Ashok Raj <ashok.raj@intel.com> >> Cc: Keith Busch <keith.busch@intel.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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-09 22:53 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-07 23:32 [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function sathyanarayanan.kuppuswamy 2019-10-09 22:30 ` Bjorn Helgaas 2019-10-09 22:51 ` Kuppuswamy Sathyanarayanan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).