diff mbox series

[v1,02/13] genirq/msi: Rename iommu_dma_compose_msi_msg() to msi_msg_set_addr()

Message ID 0cfaefcc104e29aeb031f316537249d8d53ef7fa.1739005085.git.nicolinc@nvidia.com (mailing list archive)
State New
Headers show
Series iommu: Add MSI mapping support with nested SMMU | expand

Commit Message

Nicolin Chen Feb. 8, 2025, 9:02 a.m. UTC 
From: Jason Gunthorpe <jgg@nvidia.com>

The new function is used to take in a u64 MSI address and store it in the
msi_msg. If the iommu has provided an alternative address then that is
replaced instead.

All callers have a tidy u64 already so this also consolidates the repeated
low/high code into a small helper.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/msi.h               | 18 ++++++++----------
 drivers/irqchip/irq-gic-v2m.c     |  5 +----
 drivers/irqchip/irq-gic-v3-its.c  | 13 +++----------
 drivers/irqchip/irq-gic-v3-mbi.c  | 12 ++++--------
 drivers/irqchip/irq-ls-scfg-msi.c |  5 ++---
 5 files changed, 18 insertions(+), 35 deletions(-)

Comments

Thomas Gleixner Feb. 13, 2025, 12:11 p.m. UTC | #1 
On Sat, Feb 08 2025 at 01:02, Nicolin Chen wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
>
> The new function is used to take in a u64 MSI address and store it in the

Which new function? The subject claims this is a rename. That's
confusing at best.

> msi_msg. If the iommu has provided an alternative address then that is
> replaced instead.
>
> All callers have a tidy u64 already so this also consolidates the repeated
> low/high code into a small helper.

Now I'm really confused. Something like:

    genirq/msi: Refactor iommu_dma_compose_msi_msg()

    The call sites of iommu_dma_compose_msi_msg() have the following
    pattern:

      1) Set the MSI message address to the non-translated address

      2) Invoke iommu_dma_compose_msi_msg() to apply IOVA translation if
         available.

    Rework and rename iommu_dma_compose_msi_msg() to handles both
    address set up variants in one consolidated helper.

Or something to that effect.

> -/**
> - * iommu_dma_compose_msi_msg() - Apply translation to an MSI message
> - * @desc: MSI descriptor prepared by iommu_dma_prepare_msi()
> - * @msg: MSI message containing target physical address
> - */

Please keep and rework the documentation comment.

> -static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,
> -					     struct msi_msg *msg)
> +static inline void msi_msg_set_addr(struct msi_desc *desc, struct msi_msg *msg,
> +				    u64 msi_addr)

No line break required. You have 100 characters available.

>  {
>  #ifdef CONFIG_IRQ_MSI_IOMMU
>  	if (desc->iommu_msi_page_shift) {
> @@ -314,11 +309,14 @@ static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,
>  			       << desc->iommu_msi_page_shift;
>  
>  		msg->address_hi = upper_32_bits(msi_iova);
> -		msg->address_lo = lower_32_bits(msi_iova) |
> -				  (msg->address_lo &
> -				   ((1 << desc->iommu_msi_page_shift) - 1));
> +		msg->address_lo =
> +			lower_32_bits(msi_iova) |
> +			(msi_addr & ((1 << desc->iommu_msi_page_shift) - 1));

Please avoid unrelated random formatting changes. Especially in this
case this is even more non-sensical as you introduced the original
formatting in the previous patch. So fix it up there and make it:

		msg->address_lo = lower_32_bits(msi_iova) |
				  (msg->address_lo & ((1 << desc->iommu_msi_page_shift) - 1));

which is way more readable than this displaced variant above.

> +		return;
>  
> -	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
> +	msg->data = its_get_event_id(d);
> +	msi_msg_set_addr(irq_data_get_msi_desc(d), msg,
> +			 its_dev->its->get_msi_base(its_dev));

No line break required here and at the other places.

Thanks,

        tglx
Jason Gunthorpe Feb. 13, 2025, 2:37 p.m. UTC | #2 
On Thu, Feb 13, 2025 at 01:11:37PM +0100, Thomas Gleixner wrote:
> On Sat, Feb 08 2025 at 01:02, Nicolin Chen wrote:
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> >
> > The new function is used to take in a u64 MSI address and store it in the

Assuming Nicolin moves the hunk as I suggested for patch 1 lets say:

genirq/msi: Refactor iommu_dma_compose_msi_msg()

The two step process to translate the MSI address involves two
functions, iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg().

Previously iommu_dma_compose_msi_msg() needed to be in the iommu layer
as it had to dereference the opaque cookie pointer. The previous patch
changed the cookie pointer into an integer so there is no longer any
need for the iommu layer to be involved.

Further, the call sites of iommu_dma_compose_msi_msg() all follow the
same pattern of setting the MSI message address_hi/lo to
non-translated and then immediately calling
iommu_dma_compose_msi_msg().

Refactor iommu_dma_compose_msi_msg() into msi_msg_set_addr() which
directly accepts the u64 version of the address and simplifies all the
callers.

Move the implementation to linux/msi.h since it has nothing to do with
iommu.

Aside from refactoring, this logically prepares for the next patch
which allows multiple implementation options for
iommu_dma_prepare_msi(). It does not make sense to have the
iommu_dma_compose_msi_msg() in dma-iommu.c when it no longer provides
the only iommu_dma_prepare_msi() implementation.

Ok?

> > -static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,
> > -					     struct msi_msg *msg)
> > +static inline void msi_msg_set_addr(struct msi_desc *desc, struct msi_msg *msg,
> > +				    u64 msi_addr)
> 
> No line break required. You have 100 characters available.

Sure, I make alot of patches for places that only accept 80 :\ It is
hard to keep track of everyones preferences. Thank you for having
patience, it will get fixed - I think following a 100 char limit will
resolve all of these notes.

Thanks,
Jason
diff mbox series

Patch

diff --git a/include/linux/msi.h b/include/linux/msi.h
index 74c6a823f157..2514116fa5dd 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -300,13 +300,8 @@  static inline void msi_desc_set_iommu_msi_iova(struct msi_desc *desc,
 #endif
 }
 
-/**
- * iommu_dma_compose_msi_msg() - Apply translation to an MSI message
- * @desc: MSI descriptor prepared by iommu_dma_prepare_msi()
- * @msg: MSI message containing target physical address
- */
-static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,
-					     struct msi_msg *msg)
+static inline void msi_msg_set_addr(struct msi_desc *desc, struct msi_msg *msg,
+				    u64 msi_addr)
 {
 #ifdef CONFIG_IRQ_MSI_IOMMU
 	if (desc->iommu_msi_page_shift) {
@@ -314,11 +309,14 @@  static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,
 			       << desc->iommu_msi_page_shift;
 
 		msg->address_hi = upper_32_bits(msi_iova);
-		msg->address_lo = lower_32_bits(msi_iova) |
-				  (msg->address_lo &
-				   ((1 << desc->iommu_msi_page_shift) - 1));
+		msg->address_lo =
+			lower_32_bits(msi_iova) |
+			(msi_addr & ((1 << desc->iommu_msi_page_shift) - 1));
+		return;
 	}
 #endif
+	msg->address_hi = upper_32_bits(msi_addr);
+	msg->address_lo = lower_32_bits(msi_addr);
 }
 
 int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid,
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index be35c5349986..57e0470e0d13 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -87,9 +87,6 @@  static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	struct v2m_data *v2m = irq_data_get_irq_chip_data(data);
 	phys_addr_t addr = gicv2m_get_msi_addr(v2m, data->hwirq);
 
-	msg->address_hi = upper_32_bits(addr);
-	msg->address_lo = lower_32_bits(addr);
-
 	if (v2m->flags & GICV2M_GRAVITON_ADDRESS_ONLY)
 		msg->data = 0;
 	else
@@ -97,7 +94,7 @@  static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	if (v2m->flags & GICV2M_NEEDS_SPI_OFFSET)
 		msg->data -= v2m->spi_offset;
 
-	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+	msi_msg_set_addr(irq_data_get_msi_desc(data), msg, addr);
 }
 
 static struct irq_chip gicv2m_irq_chip = {
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 8c3ec5734f1e..ce0bf70b9eaf 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1809,17 +1809,10 @@  static u64 its_irq_get_msi_base(struct its_device *its_dev)
 static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
 {
 	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
-	struct its_node *its;
-	u64 addr;
-
-	its = its_dev->its;
-	addr = its->get_msi_base(its_dev);
-
-	msg->address_lo		= lower_32_bits(addr);
-	msg->address_hi		= upper_32_bits(addr);
-	msg->data		= its_get_event_id(d);
 
-	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);
+	msg->data = its_get_event_id(d);
+	msi_msg_set_addr(irq_data_get_msi_desc(d), msg,
+			 its_dev->its->get_msi_base(its_dev));
 }
 
 static int its_irq_set_irqchip_state(struct irq_data *d,
diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c
index 3fe870f8ee17..a6510128611e 100644
--- a/drivers/irqchip/irq-gic-v3-mbi.c
+++ b/drivers/irqchip/irq-gic-v3-mbi.c
@@ -147,22 +147,18 @@  static const struct irq_domain_ops mbi_domain_ops = {
 
 static void mbi_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
-	msg[0].address_hi = upper_32_bits(mbi_phys_base + GICD_SETSPI_NSR);
-	msg[0].address_lo = lower_32_bits(mbi_phys_base + GICD_SETSPI_NSR);
 	msg[0].data = data->parent_data->hwirq;
-
-	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+	msi_msg_set_addr(irq_data_get_msi_desc(data), &msg[0],
+			 mbi_phys_base + GICD_SETSPI_NSR);
 }
 
 static void mbi_compose_mbi_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	mbi_compose_msi_msg(data, msg);
 
-	msg[1].address_hi = upper_32_bits(mbi_phys_base + GICD_CLRSPI_NSR);
-	msg[1].address_lo = lower_32_bits(mbi_phys_base + GICD_CLRSPI_NSR);
 	msg[1].data = data->parent_data->hwirq;
-
-	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), &msg[1]);
+	msi_msg_set_addr(irq_data_get_msi_desc(data), &msg[1],
+			 mbi_phys_base + GICD_CLRSPI_NSR);
 }
 
 static bool mbi_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c
index c0e1aafe468c..3cb80796cc7c 100644
--- a/drivers/irqchip/irq-ls-scfg-msi.c
+++ b/drivers/irqchip/irq-ls-scfg-msi.c
@@ -87,8 +87,6 @@  static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
 {
 	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);
 
-	msg->address_hi = upper_32_bits(msi_data->msiir_addr);
-	msg->address_lo = lower_32_bits(msi_data->msiir_addr);
 	msg->data = data->hwirq;
 
 	if (msi_affinity_flag) {
@@ -98,7 +96,8 @@  static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
 		msg->data |= cpumask_first(mask);
 	}
 
-	iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);
+	msi_msg_set_addr(irq_data_get_msi_desc(data), msg,
+			 msi_data->msiir_addr);
 }
 
 static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,