| Message ID | 20181214163319.27479-7-okaya@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
[+cc Lorenzo, Robin, Logan] On Fri, Dec 14, 2018 at 04:33:19PM +0000, Sinan Kaya wrote: > ACPI IORT table code relies on pci_request_acs() to be present. Define > a stub function when CONFI_PCI is not set. This doesn't seem like the simplest approach to me, but I probably don't understand what's going on in IORT. It looks like *all* of iort_enable_acs() (the caller of pci_request_acs()) is PCI-specific; at least, the whole thing is wrapped in a test for ACPI_IORT_NODE_PCI_ROOT_COMPLEX. So the whole function could be wrapped in #ifdef CONFIG_PCI. Here's the caller of iort_enable_acs(): iort_init_platform_devices acs_enabled = false for (i = 0; i < iort->node_count; i++) { if (!acs_enabled) acs_enabled = iort_enable_acs(iort_node); It seems like the acs_enabled state could be encapsulated inside iort_enable_acs(). Today pci_request_acs() is a system-wide thing, but I don't know why that's the case. Isn't it conceivable that different PCI hierarchies could have different ACS policies, e.g., because of P2P DMA or something? Bottom line, pci_request_acs() is being called from what looks like PCI-specific code in IORT, and it would make more sense to me to prune out that code in IORT than to make a stub pci_request_acs(). > Signed-off-by: Sinan Kaya <okaya@kernel.org> > --- > include/linux/pci.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 51a5a5217667..f0f2f55ea93c 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2101,7 +2101,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) > return NULL; > } > > +#ifdef CONFIG_PCI > void pci_request_acs(void); > +#else > +static inline void pci_request_acs(void) {} > +#endif > bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags); > bool pci_acs_path_enabled(struct pci_dev *start, > struct pci_dev *end, u16 acs_flags); > -- > 2.19.0 >
On 2018-12-14 10:26 pm, Bjorn Helgaas wrote: > [+cc Lorenzo, Robin, Logan] > > On Fri, Dec 14, 2018 at 04:33:19PM +0000, Sinan Kaya wrote: >> ACPI IORT table code relies on pci_request_acs() to be present. Define >> a stub function when CONFI_PCI is not set. > > This doesn't seem like the simplest approach to me, but I probably > don't understand what's going on in IORT. > > It looks like *all* of iort_enable_acs() (the caller of > pci_request_acs()) is PCI-specific; at least, the whole thing is > wrapped in a test for ACPI_IORT_NODE_PCI_ROOT_COMPLEX. So the whole > function could be wrapped in #ifdef CONFIG_PCI. > > Here's the caller of iort_enable_acs(): > > iort_init_platform_devices > acs_enabled = false > for (i = 0; i < iort->node_count; i++) { > if (!acs_enabled) > acs_enabled = iort_enable_acs(iort_node); > > It seems like the acs_enabled state could be encapsulated inside > iort_enable_acs(). It could, but with the tiny disadvantage of having to allocate static storage for it to maintain the "don't bother checking if we want ACS if we've already requested it once" logic for multiple root complexes. > Today pci_request_acs() is a system-wide thing, but I don't know why > that's the case. Isn't it conceivable that different PCI hierarchies > could have different ACS policies, e.g., because of P2P DMA or > something? I can certainly imagine systems using entirely separate PCI domains for, say, expansion slots vs. on-board/on-chip devices, where the former may be expected to be assigned to VMs or userspace drivers and the latter only ever controlled by the host kernel. Not forcing ACS overhead upon the "non-virtualisable" (or "trusted" vs. "untrusted") domain could perhaps be beneficial in some cases. > Bottom line, pci_request_acs() is being called from what looks like > PCI-specific code in IORT, and it would make more sense to me to prune > out that code in IORT than to make a stub pci_request_acs(). Agreed - without CONFIG_PCI the whole of iort_enable_acs() may as well be stubbed out, because it won't achieve anything anyway. The implication is that if we have a root complex whose inbound DMA is controlled by an SMMU, we should enable ACS to make VFIO useful (or in case we want to enforce strict DMA isolation in general), but if Linux is never going to touch any PCI devices there's no point even looking at the RC node(s). TBH I'm pretty dubious about having IORT at all without PCI, but I suppose there could potentially be embedded SoCs which have an ITS and platform MSIs, or use an SMMU for their video/display subsystem, and for some reason demand ACPI-based firmware. Robin. >> Signed-off-by: Sinan Kaya <okaya@kernel.org> >> --- >> include/linux/pci.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 51a5a5217667..f0f2f55ea93c 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -2101,7 +2101,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) >> return NULL; >> } >> >> +#ifdef CONFIG_PCI >> void pci_request_acs(void); >> +#else >> +static inline void pci_request_acs(void) {} >> +#endif >> bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags); >> bool pci_acs_path_enabled(struct pci_dev *start, >> struct pci_dev *end, u16 acs_flags); >> -- >> 2.19.0 >>
diff --git a/include/linux/pci.h b/include/linux/pci.h index 51a5a5217667..f0f2f55ea93c 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2101,7 +2101,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev) return NULL; } +#ifdef CONFIG_PCI void pci_request_acs(void); +#else +static inline void pci_request_acs(void) {} +#endif bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags); bool pci_acs_path_enabled(struct pci_dev *start, struct pci_dev *end, u16 acs_flags);
ACPI IORT table code relies on pci_request_acs() to be present. Define a stub function when CONFI_PCI is not set. Signed-off-by: Sinan Kaya <okaya@kernel.org> --- include/linux/pci.h | 4 ++++ 1 file changed, 4 insertions(+)