| Message ID | 20200630044943.3425049-2-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Tighten PCI security, expose dev location in sysfs | expand |
On Mon, Jun 29, 2020 at 09:49:37PM -0700, Rajat Jain wrote: > Currently this is being looked up at a number of places. Read and store it > once at bootup so that it can be used by all later. Write the commit log so it is complete even without the subject. Right now, you have to read the subject to know what "this" refers to. The subject is like the title; the log is like the body of an article. The title isn't *part* of the article, so the article has to make sense all by itself. > +static void pci_enable_acs(struct pci_dev *dev); I don't think we need this forward declaration, do we? > @@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags) > if (!pci_quirk_intel_spt_pch_acs_match(dev)) > return -ENOTTY; > > - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); > + pos = dev->acs_cap; I assume you verified that all these quirks are FINAL quirks, since pci_init_capabilities() is called after HEADER quirks. I'll double-check before applying this. Bjorn
Hi Bjorn, Thanks for taking a look. On Mon, Jul 6, 2020 at 8:58 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jun 29, 2020 at 09:49:37PM -0700, Rajat Jain wrote: > > Currently this is being looked up at a number of places. Read and store it > > once at bootup so that it can be used by all later. > > Write the commit log so it is complete even without the subject. > Right now, you have to read the subject to know what "this" refers to. > > The subject is like the title; the log is like the body of an article. > The title isn't *part* of the article, so the article has to make > sense all by itself. Fixed. > > > +static void pci_enable_acs(struct pci_dev *dev); > > I don't think we need this forward declaration, do we? We need it unless we move its definition further up in the file: drivers/pci/pci.c: In function ‘pci_restore_state’: drivers/pci/pci.c:1551:2: error: implicit declaration of function ‘pci_enable_acs’; did you mean ‘pci_enable_ats’? [-Werror=implicit-function-declaration] 1551 | pci_enable_acs(dev); Do you want me to move it up in the file so that we do not need the forward declaration? > > > @@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags) > > if (!pci_quirk_intel_spt_pch_acs_match(dev)) > > return -ENOTTY; > > > > - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); > > + pos = dev->acs_cap; > > I assume you verified that all these quirks are FINAL quirks, since > pci_init_capabilities() is called after HEADER quirks. I'll > double-check before applying this. None of these quirks are applied via DECLARE_PCI_FIXUP_*(). All these quirks are called (directly or indirectly) from either pci_enable_acs() or pci_acs_enabled(), EXCEPT pci_idt_bus_quirk(). That one is called from pci_bus_read_dev_vendor_id() which should be called only after the parent bridge has been added and setup correctly. So it looks all good to me. Thanks, Rajat > > Bjorn
On Mon, Jul 06, 2020 at 03:16:42PM -0700, Rajat Jain wrote: > On Mon, Jul 6, 2020 at 8:58 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jun 29, 2020 at 09:49:37PM -0700, Rajat Jain wrote: > > > +static void pci_enable_acs(struct pci_dev *dev); > > > > I don't think we need this forward declaration, do we? > > We need it unless we move its definition further up in the file: > > drivers/pci/pci.c: In function ‘pci_restore_state’: > drivers/pci/pci.c:1551:2: error: implicit declaration of function > ‘pci_enable_acs’; did you mean ‘pci_enable_ats’? > [-Werror=implicit-function-declaration] > 1551 | pci_enable_acs(dev); > > Do you want me to move it up in the file so that we do not need the > forward declaration? Yes, please move it. Maybe a preliminary patch that moves it but doesn't change anything else. I think I thought you had renamed the function, in which case you could tell from the patch itself. But I was mistaken! > > > @@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags) > > > if (!pci_quirk_intel_spt_pch_acs_match(dev)) > > > return -ENOTTY; > > > > > > - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); > > > + pos = dev->acs_cap; > > > > I assume you verified that all these quirks are FINAL quirks, since > > pci_init_capabilities() is called after HEADER quirks. I'll > > double-check before applying this. > > None of these quirks are applied via DECLARE_PCI_FIXUP_*(). All these > quirks are called (directly or indirectly) from either > pci_enable_acs() or pci_acs_enabled(), > > EXCEPT > > pci_idt_bus_quirk(). That one is called from > pci_bus_read_dev_vendor_id() which should be called only after the > parent bridge has been added and setup correctly. > > So it looks all good to me. Great, thanks for checking that.
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index e8e444eeb1cd2..f29a48f8fa594 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -253,7 +253,7 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev) int pos; u16 ctrl; - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS); + pos = pdev->acs_cap; if (!pos) return 0; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ce096272f52b1..d2ff987585855 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -51,6 +51,7 @@ EXPORT_SYMBOL(pci_pci_problems); unsigned int pci_pm_d3_delay; +static void pci_enable_acs(struct pci_dev *dev); static void pci_pme_list_scan(struct work_struct *work); static LIST_HEAD(pci_pme_list); @@ -3284,7 +3285,7 @@ static void pci_disable_acs_redir(struct pci_dev *dev) if (!pci_dev_specific_disable_acs_redir(dev)) return; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + pos = dev->acs_cap; if (!pos) { pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n"); return; @@ -3310,7 +3311,7 @@ static void pci_std_enable_acs(struct pci_dev *dev) u16 cap; u16 ctrl; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + pos = dev->acs_cap; if (!pos) return; @@ -3336,7 +3337,7 @@ static void pci_std_enable_acs(struct pci_dev *dev) * pci_enable_acs - enable ACS if hardware support it * @dev: the PCI device */ -void pci_enable_acs(struct pci_dev *dev) +static void pci_enable_acs(struct pci_dev *dev) { if (!pci_acs_enable) goto disable_acs_redir; @@ -3362,7 +3363,7 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) int pos; u16 cap, ctrl; - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS); + pos = pdev->acs_cap; if (!pos) return false; @@ -3487,6 +3488,18 @@ bool pci_acs_path_enabled(struct pci_dev *start, return true; } +/** + * pci_acs_init - Initialize if hardware supports it + * @dev: the PCI device + */ +void pci_acs_init(struct pci_dev *dev) +{ + dev->acs_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + + if (dev->acs_cap) + pci_enable_acs(dev); +} + /** * pci_rebar_find_pos - find position of resize ctrl reg for BAR * @pdev: PCI device diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 6d3f758671064..12fb79fbe29d3 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -532,7 +532,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, return resource_alignment(res); } -void pci_enable_acs(struct pci_dev *dev); +void pci_acs_init(struct pci_dev *dev); #ifdef CONFIG_PCI_QUIRKS int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags); int pci_dev_specific_enable_acs(struct pci_dev *dev); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2f66988cea257..6d87066a5ecc5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2390,7 +2390,7 @@ static void pci_init_capabilities(struct pci_dev *dev) pci_ats_init(dev); /* Address Translation Services */ pci_pri_init(dev); /* Page Request Interface */ pci_pasid_init(dev); /* Process Address Space ID */ - pci_enable_acs(dev); /* Enable ACS P2P upstream forwarding */ + pci_acs_init(dev); /* Access Control Services */ pci_ptm_init(dev); /* Precision Time Measurement */ pci_aer_init(dev); /* Advanced Error Reporting */ pci_dpc_init(dev); /* Downstream Port Containment */ diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 812bfc32ecb82..b341628e47527 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags) if (!pci_quirk_intel_spt_pch_acs_match(dev)) return -ENOTTY; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + pos = dev->acs_cap; if (!pos) return -ENOTTY; @@ -4961,7 +4961,7 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) if (!pci_quirk_intel_spt_pch_acs_match(dev)) return -ENOTTY; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + pos = dev->acs_cap; if (!pos) return -ENOTTY; @@ -4988,7 +4988,7 @@ static int pci_quirk_disable_intel_spt_pch_acs_redir(struct pci_dev *dev) if (!pci_quirk_intel_spt_pch_acs_match(dev)) return -ENOTTY; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); + pos = dev->acs_cap; if (!pos) return -ENOTTY; @@ -5355,7 +5355,7 @@ int pci_idt_bus_quirk(struct pci_bus *bus, int devfn, u32 *l, int timeout) bool found; struct pci_dev *bridge = bus->self; - pos = pci_find_ext_capability(bridge, PCI_EXT_CAP_ID_ACS); + pos = bridge->acs_cap; /* Disable ACS SV before initial config reads */ if (pos) { diff --git a/include/linux/pci.h b/include/linux/pci.h index c79d83304e529..a26be5332bba6 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -486,6 +486,7 @@ struct pci_dev { #ifdef CONFIG_PCI_P2PDMA struct pci_p2pdma *p2pdma; #endif + u16 acs_cap; /* ACS Capability offset */ phys_addr_t rom; /* Physical address if not from BAR */ size_t romlen; /* Length if not from BAR */ char *driver_override; /* Driver name to force a match */
Currently this is being looked up at a number of places. Read and store it once at bootup so that it can be used by all later. Signed-off-by: Rajat Jain <rajatja@google.com> --- v2: Commit log cosmetic changes drivers/pci/p2pdma.c | 2 +- drivers/pci/pci.c | 21 +++++++++++++++++---- drivers/pci/pci.h | 2 +- drivers/pci/probe.c | 2 +- drivers/pci/quirks.c | 8 ++++---- include/linux/pci.h | 1 + 6 files changed, 25 insertions(+), 11 deletions(-)