diff mbox series

[v9,6/6] PCI: Stub out pci_request_acs() when CONFIG_PCI is not set

Message ID 20181214163319.27479-7-okaya@kernel.org (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series None | expand

Commit Message

Sinan Kaya Dec. 14, 2018, 4:33 p.m. UTC
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(+)

Comments

Bjorn Helgaas Dec. 14, 2018, 10:26 p.m. UTC | #1
[+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
>
Robin Murphy Dec. 14, 2018, 11:33 p.m. UTC | #2
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 mbox series

Patch

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);