diff mbox series

[v3,09/10] iommu/dart: Exclude MSI doorbell from PCIe device IOVA range

Message ID 20210913182550.264165-10-maz@kernel.org
State Accepted
Commit 946d619fa25f55483206befb035fed6a99fbe7b5
Headers show
Series PCI: Add support for Apple M1 | expand

Commit Message

Marc Zyngier Sept. 13, 2021, 6:25 p.m. UTC
The MSI doorbell on Apple HW can be any address in the low 4GB
range. However, the MSI write is matched by the PCIe block before
hitting the iommu. It must thus be excluded from the IOVA range
that is assigned to any PCIe device.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/iommu/apple-dart.c          | 25 +++++++++++++++++++++++++
 drivers/pci/controller/Kconfig      |  5 +++++
 drivers/pci/controller/pcie-apple.c |  4 +++-
 3 files changed, 33 insertions(+), 1 deletion(-)

Comments

Alyssa Rosenzweig Sept. 13, 2021, 6:55 p.m. UTC | #1
> +config PCIE_APPLE_MSI_DOORBELL_ADDR
> +	hex
> +	default 0xfffff000
> +	depends on PCIE_APPLE
> +
>  config PCIE_APPLE
>  	tristate "Apple PCIe controller"
>  	depends on ARCH_APPLE || COMPILE_TEST
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1ed7b90f8360..76344223245d 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -120,8 +120,10 @@
>   * The doorbell address is set to 0xfffff000, which by convention
>   * matches what MacOS does, and it is possible to use any other
>   * address (in the bottom 4GB, as the base register is only 32bit).
> + * However, it has to be excluded from the the IOVA range, and the
> + * DART driver has to know about it.
>   */
> -#define DOORBELL_ADDR			0xfffff000
> +#define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR

I'm unsure if Kconfig is the right place for this. But if it is, these
hunks should be moved earlier in the series (so the deletion gets
squashed away of the hardcoded-in-the-C.)
Sven Peter Sept. 14, 2021, 1:54 p.m. UTC | #2
On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> The MSI doorbell on Apple HW can be any address in the low 4GB

> range. However, the MSI write is matched by the PCIe block before

> hitting the iommu. It must thus be excluded from the IOVA range

> that is assigned to any PCIe device.

> 

> Signed-off-by: Marc Zyngier <maz@kernel.org>


It's not pretty but I'm not aware of any better solution and this should
work as long as these two are always paired. With the small nit below
addressed:

Reviewed-by: Sven Peter <sven@svenpeter.dev>


> ---

>  drivers/iommu/apple-dart.c          | 25 +++++++++++++++++++++++++

>  drivers/pci/controller/Kconfig      |  5 +++++

>  drivers/pci/controller/pcie-apple.c |  4 +++-

>  3 files changed, 33 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c

> index 559db9259e65..d1456663688e 100644

> --- a/drivers/iommu/apple-dart.c

> +++ b/drivers/iommu/apple-dart.c

> @@ -721,6 +721,29 @@ static int apple_dart_def_domain_type(struct device *dev)

>  	return 0;

>  }

>  

> +#define DOORBELL_ADDR	(CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)

> +

> +static void apple_dart_get_resv_regions(struct device *dev,

> +					struct list_head *head)

> +{

> +#ifdef CONFIG_PCIE_APPLE


I think using IS_ENABLED would be better here in case the pcie driver is built as
a module which would then only define CONFIG_PCIE_APPLE_MODULE AIUI.


Thanks,


Sven
Marc Zyngier Sept. 17, 2021, 10:01 a.m. UTC | #3
On Tue, 14 Sep 2021 14:54:07 +0100,
"Sven Peter" <sven@svenpeter.dev> wrote:
> 

> 

> 

> On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:

> > The MSI doorbell on Apple HW can be any address in the low 4GB

> > range. However, the MSI write is matched by the PCIe block before

> > hitting the iommu. It must thus be excluded from the IOVA range

> > that is assigned to any PCIe device.

> > 

> > Signed-off-by: Marc Zyngier <maz@kernel.org>

> 

> It's not pretty but I'm not aware of any better solution and this should

> work as long as these two are always paired. With the small nit below

> addressed:

> 

> Reviewed-by: Sven Peter <sven@svenpeter.dev>

> 

> > ---

> >  drivers/iommu/apple-dart.c          | 25 +++++++++++++++++++++++++

> >  drivers/pci/controller/Kconfig      |  5 +++++

> >  drivers/pci/controller/pcie-apple.c |  4 +++-

> >  3 files changed, 33 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c

> > index 559db9259e65..d1456663688e 100644

> > --- a/drivers/iommu/apple-dart.c

> > +++ b/drivers/iommu/apple-dart.c

> > @@ -721,6 +721,29 @@ static int apple_dart_def_domain_type(struct device *dev)

> >  	return 0;

> >  }

> >  

> > +#define DOORBELL_ADDR	(CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)

> > +

> > +static void apple_dart_get_resv_regions(struct device *dev,

> > +					struct list_head *head)

> > +{

> > +#ifdef CONFIG_PCIE_APPLE

> 

> I think using IS_ENABLED would be better here in case the pcie

> driver is built as a module which would then only define

> CONFIG_PCIE_APPLE_MODULE AIUI.


You're right, this isn't great. However, IS_ENABLED() still results in
the evaluation of the following code by the compiler, even if it would
be dropped by the optimiser.

I resorted to the following construct:

<quote>
#ifndef CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
/* Keep things compiling when CONFIG_PCI_APPLE isn't selected */
#define CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR	0
#endif
#define DOORBELL_ADDR	(CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)

static void apple_dart_get_resv_regions(struct device *dev,
					struct list_head *head)
{
	if (IS_ENABLED(CONFIG_PCIE_APPLE) && dev_is_pci(dev)) {
</quote>

which is ugly, but works. The alternative is, of course, to add a new
include file for one single value, which I find even worse.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
Marc Zyngier Sept. 17, 2021, 10:05 a.m. UTC | #4
On Mon, 13 Sep 2021 19:55:53 +0100,
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> 

> > +config PCIE_APPLE_MSI_DOORBELL_ADDR

> > +	hex

> > +	default 0xfffff000

> > +	depends on PCIE_APPLE

> > +

> >  config PCIE_APPLE

> >  	tristate "Apple PCIe controller"

> >  	depends on ARCH_APPLE || COMPILE_TEST

> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c

> > index 1ed7b90f8360..76344223245d 100644

> > --- a/drivers/pci/controller/pcie-apple.c

> > +++ b/drivers/pci/controller/pcie-apple.c

> > @@ -120,8 +120,10 @@

> >   * The doorbell address is set to 0xfffff000, which by convention

> >   * matches what MacOS does, and it is possible to use any other

> >   * address (in the bottom 4GB, as the base register is only 32bit).

> > + * However, it has to be excluded from the the IOVA range, and the

> > + * DART driver has to know about it.

> >   */

> > -#define DOORBELL_ADDR			0xfffff000

> > +#define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR

> 

> I'm unsure if Kconfig is the right place for this. But if it is, these

> hunks should be moved earlier in the series (so the deletion gets

> squashed away of the hardcoded-in-the-C.)


I'd rather not doing that. There is a progression in the series, and
moving the value over to Kconfig is part of that progression.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
diff mbox series

Patch

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 559db9259e65..d1456663688e 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -721,6 +721,29 @@  static int apple_dart_def_domain_type(struct device *dev)
 	return 0;
 }
 
+#define DOORBELL_ADDR	(CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)
+
+static void apple_dart_get_resv_regions(struct device *dev,
+					struct list_head *head)
+{
+#ifdef CONFIG_PCIE_APPLE
+	if (dev_is_pci(dev)) {
+		struct iommu_resv_region *region;
+		int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+
+		region = iommu_alloc_resv_region(DOORBELL_ADDR,
+						 PAGE_SIZE, prot,
+						 IOMMU_RESV_MSI);
+		if (!region)
+			return;
+
+		list_add_tail(&region->list, head);
+	}
+#endif
+
+	iommu_dma_get_resv_regions(dev, head);
+}
+
 static const struct iommu_ops apple_dart_iommu_ops = {
 	.domain_alloc = apple_dart_domain_alloc,
 	.domain_free = apple_dart_domain_free,
@@ -737,6 +760,8 @@  static const struct iommu_ops apple_dart_iommu_ops = {
 	.device_group = apple_dart_device_group,
 	.of_xlate = apple_dart_of_xlate,
 	.def_domain_type = apple_dart_def_domain_type,
+	.get_resv_regions = apple_dart_get_resv_regions,
+	.put_resv_regions = generic_iommu_put_resv_regions,
 	.pgsize_bitmap = -1UL, /* Restricted during dart probe */
 };
 
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 814833a8120d..b6e7410da254 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -312,6 +312,11 @@  config PCIE_HISI_ERR
 	  Say Y here if you want error handling support
 	  for the PCIe controller's errors on HiSilicon HIP SoCs
 
+config PCIE_APPLE_MSI_DOORBELL_ADDR
+	hex
+	default 0xfffff000
+	depends on PCIE_APPLE
+
 config PCIE_APPLE
 	tristate "Apple PCIe controller"
 	depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 1ed7b90f8360..76344223245d 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -120,8 +120,10 @@ 
  * The doorbell address is set to 0xfffff000, which by convention
  * matches what MacOS does, and it is possible to use any other
  * address (in the bottom 4GB, as the base register is only 32bit).
+ * However, it has to be excluded from the the IOVA range, and the
+ * DART driver has to know about it.
  */
-#define DOORBELL_ADDR			0xfffff000
+#define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
 
 struct apple_pcie {
 	struct mutex		lock;