[PATCH v3 0/2] x86/pci: reduce PCI accesses

[PATCH v3 0/2] x86/pci: reduce PCI accesses

[Roger Pau Monne]

Hello,

First patch is not really related, but it's some cleanup I've found
while looking at the other fixes.  Patch 2 extends the IOMMU logic to
prevent re-writing the same MSI message.

Thanks, Roger.

Roger Pau Monne (2):
  x86/apic: remove delivery and destination mode fields from drivers
  x86/iommu: avoid MSI address and data writes if IRT index hasn't
    changed

 xen/arch/x86/genapic/bigsmp.c            |  2 --
 xen/arch/x86/genapic/default.c           |  2 --
 xen/arch/x86/genapic/x2apic.c            |  4 ----
 xen/arch/x86/hpet.c                      |  6 +++++-
 xen/arch/x86/hvm/vmx/vmx.c               |  4 +++-
 xen/arch/x86/include/asm/genapic.h       |  5 -----
 xen/arch/x86/io_apic.c                   | 16 ++++++++--------
 xen/arch/x86/msi.c                       | 22 +++++++++-------------
 xen/drivers/passthrough/amd/iommu_intr.c |  4 ++--
 xen/drivers/passthrough/vtd/intremap.c   |  4 +++-
 xen/include/xen/iommu.h                  |  6 ++++++
 11 files changed, 36 insertions(+), 39 deletions(-)

-- 
2.48.1

[PATCH v3 1/2] x86/apic: remove delivery and destination mode fields from drivers

[Roger Pau Monne]

All local APIC drivers use physical destination and fixed delivery modes,
remove the fields from the genapic struct and simplify the logic.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Add comment about dest_mode setting.
---
 xen/arch/x86/genapic/bigsmp.c      |  2 --
 xen/arch/x86/genapic/default.c     |  2 --
 xen/arch/x86/genapic/x2apic.c      |  4 ----
 xen/arch/x86/include/asm/genapic.h |  5 -----
 xen/arch/x86/io_apic.c             | 16 ++++++++--------
 xen/arch/x86/msi.c                 | 11 +++--------
 6 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/genapic/bigsmp.c b/xen/arch/x86/genapic/bigsmp.c
index b2e721845275..ddb3a0b5d727 100644
--- a/xen/arch/x86/genapic/bigsmp.c
+++ b/xen/arch/x86/genapic/bigsmp.c
@@ -46,8 +46,6 @@ static int __init cf_check probe_bigsmp(void)
 
 const struct genapic __initconst_cf_clobber apic_bigsmp = {
 	APIC_INIT("bigsmp", probe_bigsmp),
-	.int_delivery_mode = dest_Fixed,
-	.int_dest_mode = 0, /* physical delivery */
 	.init_apic_ldr = init_apic_ldr_phys,
 	.vector_allocation_cpumask = vector_allocation_cpumask_phys,
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
diff --git a/xen/arch/x86/genapic/default.c b/xen/arch/x86/genapic/default.c
index 59c79afdb8fa..16e1875f6378 100644
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -16,8 +16,6 @@
 /* should be called last. */
 const struct genapic __initconst_cf_clobber apic_default = {
 	APIC_INIT("default", NULL),
-	.int_delivery_mode = dest_Fixed,
-	.int_dest_mode = 0, /* physical delivery */
 	.init_apic_ldr = init_apic_ldr_flat,
 	.vector_allocation_cpumask = vector_allocation_cpumask_phys,
 	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index c277f4f79b0a..74a6d808ac30 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -140,8 +140,6 @@ static void cf_check send_IPI_mask_x2apic_cluster(
 
 static const struct genapic __initconst_cf_clobber apic_x2apic_phys = {
     APIC_INIT("x2apic_phys", NULL),
-    .int_delivery_mode = dest_Fixed,
-    .int_dest_mode = 0 /* physical delivery */,
     .init_apic_ldr = init_apic_ldr_phys,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
@@ -163,8 +161,6 @@ static const struct genapic __initconst_cf_clobber apic_x2apic_mixed = {
      * The following fields are exclusively used by external interrupts and
      * hence are set to use Physical destination mode handlers.
      */
-    .int_delivery_mode = dest_Fixed,
-    .int_dest_mode = 0 /* physical delivery */,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
 
diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
index cf36d48f3b07..04d3f1de7a1f 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -23,9 +23,6 @@ struct genapic {
 	const char *name;
 	int (*probe)(void);
 
-	/* Interrupt delivery parameters ('physical' vs. 'logical flat'). */
-	int int_delivery_mode;
-	int int_dest_mode;
 	void (*init_apic_ldr)(void);
 	const cpumask_t *(*vector_allocation_cpumask)(int cpu);
 	unsigned int (*cpu_mask_to_apicid)(const cpumask_t *cpumask);
@@ -37,8 +34,6 @@ struct genapic {
 	.name = aname, \
 	.probe = aprobe
 
-#define INT_DELIVERY_MODE (genapic.int_delivery_mode)
-#define INT_DEST_MODE (genapic.int_dest_mode)
 #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map)
 #define init_apic_ldr() alternative_vcall(genapic.init_apic_ldr)
 #define cpu_mask_to_apicid(mask) ({ \
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 776dd57720a2..c6cf94481129 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1080,8 +1080,8 @@ static void __init setup_IO_APIC_irqs(void)
              */
             memset(&entry,0,sizeof(entry));
 
-            entry.delivery_mode = INT_DELIVERY_MODE;
-            entry.dest_mode = INT_DEST_MODE;
+            entry.delivery_mode = dest_Fixed;
+            entry.dest_mode = 0; /* physical delivery */
             entry.mask = 0;                /* enable IRQ */
 
             idx = find_irq_entry(apic,pin,mp_INT);
@@ -1150,10 +1150,10 @@ static void __init setup_ExtINT_IRQ0_pin(unsigned int apic, unsigned int pin, in
      * We use logical delivery to get the timer IRQ
      * to the first CPU.
      */
-    entry.dest_mode = INT_DEST_MODE;
+    entry.dest_mode = 0; /* physical delivery */
     entry.mask = 0;					/* unmask IRQ now */
     SET_DEST(entry, logical, cpu_mask_to_apicid(TARGET_CPUS));
-    entry.delivery_mode = INT_DELIVERY_MODE;
+    entry.delivery_mode = dest_Fixed;
     entry.polarity = 0;
     entry.trigger = 0;
     entry.vector = vector;
@@ -2338,8 +2338,8 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
 
     memset(&entry,0,sizeof(entry));
 
-    entry.delivery_mode = INT_DELIVERY_MODE;
-    entry.dest_mode = INT_DEST_MODE;
+    entry.delivery_mode = dest_Fixed;
+    entry.dest_mode = 0; /* physical delivery */
     entry.trigger = edge_level;
     entry.polarity = active_high_low;
     entry.mask  = 1;
@@ -2473,8 +2473,8 @@ int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
      * The guest does not know physical APIC arrangement (flat vs. cluster).
      * Apply genapic conventions for this platform.
      */
-    rte.delivery_mode = INT_DELIVERY_MODE;
-    rte.dest_mode     = INT_DEST_MODE;
+    rte.delivery_mode = dest_Fixed;
+    rte.dest_mode     = 0; /* physical delivery */
 
     irq = apic_pin_2_gsi_irq(apic, pin);
     if ( irq < 0 )
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index bf5b71822ea9..6c11d76015fb 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -174,18 +174,13 @@ void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg
 
     msg->address_hi = MSI_ADDR_BASE_HI;
     msg->address_lo = MSI_ADDR_BASE_LO |
-                      (INT_DEST_MODE ? MSI_ADDR_DESTMODE_LOGIC
-                                     : MSI_ADDR_DESTMODE_PHYS) |
-                      ((INT_DELIVERY_MODE != dest_LowestPrio)
-                       ? MSI_ADDR_REDIRECTION_CPU
-                       : MSI_ADDR_REDIRECTION_LOWPRI) |
+                      MSI_ADDR_DESTMODE_PHYS |
+                      MSI_ADDR_REDIRECTION_CPU |
                       MSI_ADDR_DEST_ID(msg->dest32);
 
     msg->data = MSI_DATA_TRIGGER_EDGE |
                 MSI_DATA_LEVEL_ASSERT |
-                ((INT_DELIVERY_MODE != dest_LowestPrio)
-                 ? MSI_DATA_DELIVERY_FIXED
-                 : MSI_DATA_DELIVERY_LOWPRI) |
+                MSI_DATA_DELIVERY_FIXED |
                 MSI_DATA_VECTOR(vector);
 }
 
-- 
2.48.1

[PATCH v3 2/2] x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

[Roger Pau Monne]

Attempt to reduce the MSI entry writes, and the associated checking whether
memory decoding and MSI-X is enabled for the PCI device, when the MSI data
hasn't changed.

When using Interrupt Remapping the MSI entry will contain an index into
the remapping table, and it's in such remapping table where the MSI vector
and destination CPU is stored.  As such, when using interrupt remapping,
changes to the interrupt affinity shouldn't result in changes to the MSI
entry, and the MSI entry update can be avoided.

Signal from the IOMMU update_ire_from_msi hook whether the MSI data or
address fields have changed, and thus need writing to the device registers.
Such signaling is done by returning 1 from the function.  Otherwise
returning 0 means no update of the MSI fields, and thus no write
required.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes since v2:
 - New approach.

Changes since v1:
 - Add more comments.
 - Simplify dma_msi_set_affinity().
---
 xen/arch/x86/hpet.c                      |  6 +++++-
 xen/arch/x86/hvm/vmx/vmx.c               |  4 +++-
 xen/arch/x86/msi.c                       | 11 ++++++-----
 xen/drivers/passthrough/amd/iommu_intr.c |  4 ++--
 xen/drivers/passthrough/vtd/intremap.c   |  4 +++-
 xen/include/xen/iommu.h                  |  6 ++++++
 6 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 51ff7f12f5c0..1bca8c8b670d 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -283,8 +283,12 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
     {
         int rc = iommu_update_ire_from_msi(&ch->msi, msg);
 
-        if ( rc )
+        if ( rc < 0 )
             return rc;
+        /*
+         * Always propagate writes, to avoid having to pass a flag for handling
+         * a forceful write in the resume from suspension case.
+         */
     }
 
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0241303b4bf4..764d2ff9517a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -415,7 +415,9 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
 
     ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
 
-    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+    rc = iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+
+    return rc < 0 ? rc : 0;
 
  unlock_out:
     spin_unlock_irq(&desc->lock);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6c11d76015fb..163ccf874720 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -184,7 +184,8 @@ void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg
                 MSI_DATA_VECTOR(vector);
 }
 
-static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
+static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg,
+                         bool force)
 {
     entry->msg = *msg;
 
@@ -194,7 +195,7 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 
         ASSERT(msg != &entry->msg);
         rc = iommu_update_ire_from_msi(entry, msg);
-        if ( rc )
+        if ( rc < 0 || (rc == 0 && !force) )
             return rc;
     }
 
@@ -259,7 +260,7 @@ void cf_check set_msi_affinity(struct irq_desc *desc, const cpumask_t *mask)
     msg.address_lo |= MSI_ADDR_DEST_ID(dest);
     msg.dest32 = dest;
 
-    write_msi_msg(msi_desc, &msg);
+    write_msi_msg(msi_desc, &msg, false);
 }
 
 void __msi_set_enable(pci_sbdf_t sbdf, int pos, int enable)
@@ -522,7 +523,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
     desc->msi_desc = msidesc;
     desc->handler = handler;
     msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
-    ret = write_msi_msg(msidesc, &msg);
+    ret = write_msi_msg(msidesc, &msg, false);
     if ( unlikely(ret) )
     {
         desc->handler = &no_irq_type;
@@ -1403,7 +1404,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
         type = entry->msi_attrib.type;
 
         msg = entry->msg;
-        write_msi_msg(entry, &msg);
+        write_msi_msg(entry, &msg, true);
 
         for ( i = 0; ; )
         {
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index c0273059cb1d..07b21c6043ef 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -492,7 +492,7 @@ static int update_intremap_entry_from_msi_msg(
                get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
     }
 
-    return 0;
+    return !fresh ? 0 : 1;
 }
 
 static struct amd_iommu *_find_iommu_for_device(int seg, int bdf)
@@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
     rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
                                             &msi_desc->remap_index,
                                             msg, &data);
-    if ( !rc )
+    if ( rc > 0 )
     {
         for ( i = 1; i < nr; ++i )
             msi_desc[i].remap_index = msi_desc->remap_index + i;
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index 1aeaeb5ec595..a9d96fcdbac8 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -506,6 +506,7 @@ static int msi_msg_to_remap_entry(
     unsigned int index, i, nr = 1;
     unsigned long flags;
     const struct pi_desc *pi_desc = msi_desc->pi_desc;
+    bool alloc = false;
 
     if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
         nr = msi_desc->msi.nvec;
@@ -529,6 +530,7 @@ static int msi_msg_to_remap_entry(
         index = alloc_remap_entry(iommu, nr);
         for ( i = 0; i < nr; ++i )
             msi_desc[i].remap_index = index + i;
+        alloc = true;
     }
     else
         index = msi_desc->remap_index;
@@ -601,7 +603,7 @@ static int msi_msg_to_remap_entry(
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&iommu->intremap.lock, flags);
 
-    return 0;
+    return alloc ? 1 : 0;
 }
 
 int cf_check msi_msg_write_remap_rte(
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 77a514019cc6..984f0735d4a9 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -435,6 +435,12 @@ extern struct page_list_head iommu_pt_cleanup_list;
 bool arch_iommu_use_permitted(const struct domain *d);
 
 #ifdef CONFIG_X86
+/*
+ * Return values:
+ *  - < 0 on error.
+ *  - 0 on success and no need to write msi_msg to the hardware.
+ *  - 1 on success and msi_msg must be propagated to the hardware.
+ */
 static inline int iommu_update_ire_from_msi(
     struct msi_desc *msi_desc, struct msi_msg *msg)
 {
-- 
2.48.1

Re: [PATCH v3 2/2] x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

[Jan Beulich]

On 10.03.2025 10:55, Roger Pau Monne wrote:
> Attempt to reduce the MSI entry writes, and the associated checking whether
> memory decoding and MSI-X is enabled for the PCI device, when the MSI data
> hasn't changed.
> 
> When using Interrupt Remapping the MSI entry will contain an index into
> the remapping table, and it's in such remapping table where the MSI vector
> and destination CPU is stored.  As such, when using interrupt remapping,
> changes to the interrupt affinity shouldn't result in changes to the MSI
> entry, and the MSI entry update can be avoided.
> 
> Signal from the IOMMU update_ire_from_msi hook whether the MSI data or
> address fields have changed, and thus need writing to the device registers.
> Such signaling is done by returning 1 from the function.  Otherwise
> returning 0 means no update of the MSI fields, and thus no write
> required.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two purely cosmetic suggestions and an only loosely related question below.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -415,7 +415,9 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
>  
>      ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
>  
> -    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> +    rc = iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> +
> +    return rc < 0 ? rc : 0;

Only tangential here, but: Why does this function have a return type of
non-void, when neither caller cares?

> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -492,7 +492,7 @@ static int update_intremap_entry_from_msi_msg(
>                 get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
>      }
>  
> -    return 0;
> +    return !fresh ? 0 : 1;
>  }

Simply

    return fresh;

?

> @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
>      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
>                                              &msi_desc->remap_index,
>                                              msg, &data);
> -    if ( !rc )
> +    if ( rc > 0 )
>      {
>          for ( i = 1; i < nr; ++i )
>              msi_desc[i].remap_index = msi_desc->remap_index + i;
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -506,6 +506,7 @@ static int msi_msg_to_remap_entry(
>      unsigned int index, i, nr = 1;
>      unsigned long flags;
>      const struct pi_desc *pi_desc = msi_desc->pi_desc;
> +    bool alloc = false;
>  
>      if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
>          nr = msi_desc->msi.nvec;
> @@ -529,6 +530,7 @@ static int msi_msg_to_remap_entry(
>          index = alloc_remap_entry(iommu, nr);
>          for ( i = 0; i < nr; ++i )
>              msi_desc[i].remap_index = index + i;
> +        alloc = true;
>      }
>      else
>          index = msi_desc->remap_index;
> @@ -601,7 +603,7 @@ static int msi_msg_to_remap_entry(
>      unmap_vtd_domain_page(iremap_entries);
>      spin_unlock_irqrestore(&iommu->intremap.lock, flags);
>  
> -    return 0;
> +    return alloc ? 1 : 0;
>  }

Like above, simply

    return alloc;

?

Jan

Re: [PATCH v3 2/2] x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

[Roger Pau Monné]

On Mon, Mar 10, 2025 at 11:51:09AM +0100, Jan Beulich wrote:
> On 10.03.2025 10:55, Roger Pau Monne wrote:
> > Attempt to reduce the MSI entry writes, and the associated checking whether
> > memory decoding and MSI-X is enabled for the PCI device, when the MSI data
> > hasn't changed.
> > 
> > When using Interrupt Remapping the MSI entry will contain an index into
> > the remapping table, and it's in such remapping table where the MSI vector
> > and destination CPU is stored.  As such, when using interrupt remapping,
> > changes to the interrupt affinity shouldn't result in changes to the MSI
> > entry, and the MSI entry update can be avoided.
> > 
> > Signal from the IOMMU update_ire_from_msi hook whether the MSI data or
> > address fields have changed, and thus need writing to the device registers.
> > Such signaling is done by returning 1 from the function.  Otherwise
> > returning 0 means no update of the MSI fields, and thus no write
> > required.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two purely cosmetic suggestions and an only loosely related question below.
> 
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -415,7 +415,9 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
> >  
> >      ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
> >  
> > -    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> > +    rc = iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> > +
> > +    return rc < 0 ? rc : 0;
> 
> Only tangential here, but: Why does this function have a return type of
> non-void, when neither caller cares?

I'm afraid there's more wrong in vmx_pi_update_irte() that I've just
spotted afterwards.

vmx_pi_update_irte() passes to iommu_update_ire_from_msi() the
msi_desc->msg field, but that field is supposed to always contain the
non-translated MSI data, as you correctly pointed out in v1 it's
consumed by dump_msi().  vmx_pi_update_irte() using msi_desc->msg to
store the translated MSI effectively breaks dump_msi().

Also vmx_pi_update_irte() relies on the IRT index never changing, as
otherwise it's missing any logic to update the MSI registers.

I will fix that in a pre-patch.

> 
> > --- a/xen/drivers/passthrough/amd/iommu_intr.c
> > +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> > @@ -492,7 +492,7 @@ static int update_intremap_entry_from_msi_msg(
> >                 get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
> >      }
> >  
> > -    return 0;
> > +    return !fresh ? 0 : 1;
> >  }
> 
> Simply
> 
>     return fresh;
> 
> ?
> 
> > @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
> >      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
> >                                              &msi_desc->remap_index,
> >                                              msg, &data);
> > -    if ( !rc )
> > +    if ( rc > 0 )
> >      {
> >          for ( i = 1; i < nr; ++i )
> >              msi_desc[i].remap_index = msi_desc->remap_index + i;
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -506,6 +506,7 @@ static int msi_msg_to_remap_entry(
> >      unsigned int index, i, nr = 1;
> >      unsigned long flags;
> >      const struct pi_desc *pi_desc = msi_desc->pi_desc;
> > +    bool alloc = false;
> >  
> >      if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> >          nr = msi_desc->msi.nvec;
> > @@ -529,6 +530,7 @@ static int msi_msg_to_remap_entry(
> >          index = alloc_remap_entry(iommu, nr);
> >          for ( i = 0; i < nr; ++i )
> >              msi_desc[i].remap_index = index + i;
> > +        alloc = true;
> >      }
> >      else
> >          index = msi_desc->remap_index;
> > @@ -601,7 +603,7 @@ static int msi_msg_to_remap_entry(
> >      unmap_vtd_domain_page(iremap_entries);
> >      spin_unlock_irqrestore(&iommu->intremap.lock, flags);
> >  
> > -    return 0;
> > +    return alloc ? 1 : 0;
> >  }
> 
> Like above, simply
> 
>     return alloc;
> 
> ?

I wasn't sure whether this was overloading the boolean type and
possibly breaking some MISRA rule.  I can adjust.

Thanks, Roger.

Re: [PATCH v3 2/2] x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

[Jan Beulich]

On 10.03.2025 16:42, Roger Pau Monné wrote:
> On Mon, Mar 10, 2025 at 11:51:09AM +0100, Jan Beulich wrote:
>> On 10.03.2025 10:55, Roger Pau Monne wrote:
>>> Attempt to reduce the MSI entry writes, and the associated checking whether
>>> memory decoding and MSI-X is enabled for the PCI device, when the MSI data
>>> hasn't changed.
>>>
>>> When using Interrupt Remapping the MSI entry will contain an index into
>>> the remapping table, and it's in such remapping table where the MSI vector
>>> and destination CPU is stored.  As such, when using interrupt remapping,
>>> changes to the interrupt affinity shouldn't result in changes to the MSI
>>> entry, and the MSI entry update can be avoided.
>>>
>>> Signal from the IOMMU update_ire_from_msi hook whether the MSI data or
>>> address fields have changed, and thus need writing to the device registers.
>>> Such signaling is done by returning 1 from the function.  Otherwise
>>> returning 0 means no update of the MSI fields, and thus no write
>>> required.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with two purely cosmetic suggestions and an only loosely related question below.
>>
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -415,7 +415,9 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
>>>  
>>>      ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
>>>  
>>> -    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
>>> +    rc = iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
>>> +
>>> +    return rc < 0 ? rc : 0;
>>
>> Only tangential here, but: Why does this function have a return type of
>> non-void, when neither caller cares?
> 
> I'm afraid there's more wrong in vmx_pi_update_irte() that I've just
> spotted afterwards.
> 
> vmx_pi_update_irte() passes to iommu_update_ire_from_msi() the
> msi_desc->msg field, but that field is supposed to always contain the
> non-translated MSI data, as you correctly pointed out in v1 it's
> consumed by dump_msi().  vmx_pi_update_irte() using msi_desc->msg to
> store the translated MSI effectively breaks dump_msi().

Oh, indeed - it violates what write_msi_msg() specifically checks by
an assertion.

> Also vmx_pi_update_irte() relies on the IRT index never changing, as
> otherwise it's missing any logic to update the MSI registers.

Isn't this a valid assumption here? msi_msg_to_remap_entry() will only
ever allocate a new index if none was previously allocated.

>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>> @@ -492,7 +492,7 @@ static int update_intremap_entry_from_msi_msg(
>>>                 get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
>>>      }
>>>  
>>> -    return 0;
>>> +    return !fresh ? 0 : 1;
>>>  }
>>
>> Simply
>>
>>     return fresh;
>>
>> ?
>>
>>> @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
>>>      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
>>>                                              &msi_desc->remap_index,
>>>                                              msg, &data);
>>> -    if ( !rc )
>>> +    if ( rc > 0 )
>>>      {
>>>          for ( i = 1; i < nr; ++i )
>>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>>> --- a/xen/drivers/passthrough/vtd/intremap.c
>>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>>> @@ -506,6 +506,7 @@ static int msi_msg_to_remap_entry(
>>>      unsigned int index, i, nr = 1;
>>>      unsigned long flags;
>>>      const struct pi_desc *pi_desc = msi_desc->pi_desc;
>>> +    bool alloc = false;
>>>  
>>>      if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
>>>          nr = msi_desc->msi.nvec;
>>> @@ -529,6 +530,7 @@ static int msi_msg_to_remap_entry(
>>>          index = alloc_remap_entry(iommu, nr);
>>>          for ( i = 0; i < nr; ++i )
>>>              msi_desc[i].remap_index = index + i;
>>> +        alloc = true;
>>>      }
>>>      else
>>>          index = msi_desc->remap_index;
>>> @@ -601,7 +603,7 @@ static int msi_msg_to_remap_entry(
>>>      unmap_vtd_domain_page(iremap_entries);
>>>      spin_unlock_irqrestore(&iommu->intremap.lock, flags);
>>>  
>>> -    return 0;
>>> +    return alloc ? 1 : 0;
>>>  }
>>
>> Like above, simply
>>
>>     return alloc;
>>
>> ?
> 
> I wasn't sure whether this was overloading the boolean type and
> possibly breaking some MISRA rule.  I can adjust.

What we are to do with Misra's essential type system is entirely unclear at
this point, aiui.

Jan

Re: [PATCH v3 2/2] x86/iommu: avoid MSI address and data writes if IRT index hasn't changed

[Roger Pau Monné]

On Tue, Mar 11, 2025 at 08:35:35AM +0100, Jan Beulich wrote:
> On 10.03.2025 16:42, Roger Pau Monné wrote:
> > On Mon, Mar 10, 2025 at 11:51:09AM +0100, Jan Beulich wrote:
> >> On 10.03.2025 10:55, Roger Pau Monne wrote:
> >>> Attempt to reduce the MSI entry writes, and the associated checking whether
> >>> memory decoding and MSI-X is enabled for the PCI device, when the MSI data
> >>> hasn't changed.
> >>>
> >>> When using Interrupt Remapping the MSI entry will contain an index into
> >>> the remapping table, and it's in such remapping table where the MSI vector
> >>> and destination CPU is stored.  As such, when using interrupt remapping,
> >>> changes to the interrupt affinity shouldn't result in changes to the MSI
> >>> entry, and the MSI entry update can be avoided.
> >>>
> >>> Signal from the IOMMU update_ire_from_msi hook whether the MSI data or
> >>> address fields have changed, and thus need writing to the device registers.
> >>> Such signaling is done by returning 1 from the function.  Otherwise
> >>> returning 0 means no update of the MSI fields, and thus no write
> >>> required.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> with two purely cosmetic suggestions and an only loosely related question below.
> >>
> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >>> @@ -415,7 +415,9 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
> >>>  
> >>>      ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
> >>>  
> >>> -    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> >>> +    rc = iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
> >>> +
> >>> +    return rc < 0 ? rc : 0;
> >>
> >> Only tangential here, but: Why does this function have a return type of
> >> non-void, when neither caller cares?
> > 
> > I'm afraid there's more wrong in vmx_pi_update_irte() that I've just
> > spotted afterwards.
> > 
> > vmx_pi_update_irte() passes to iommu_update_ire_from_msi() the
> > msi_desc->msg field, but that field is supposed to always contain the
> > non-translated MSI data, as you correctly pointed out in v1 it's
> > consumed by dump_msi().  vmx_pi_update_irte() using msi_desc->msg to
> > store the translated MSI effectively breaks dump_msi().
> 
> Oh, indeed - it violates what write_msi_msg() specifically checks by
> an assertion.

Indeed.  I have a pre-patch to fix this.

> > Also vmx_pi_update_irte() relies on the IRT index never changing, as
> > otherwise it's missing any logic to update the MSI registers.
> 
> Isn't this a valid assumption here? msi_msg_to_remap_entry() will only
> ever allocate a new index if none was previously allocated.

Yes, but I think it needs to be accounted for, I've now switched this
to:

    rc = iommu_update_ire_from_msi(msi_desc, &msg);
    if ( rc > 0 )
    {
        /*
         * Callers of vmx_pi_update_irte() won't propagate the updated MSI
         * fields to the hardware, must assert there are no changes.
         */
        ASSERT_UNREACHABLE();
        rc = -EILSEQ;
    }

    return rc;

Which I think better reflects the expectations of the function.

> >>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> >>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> >>> @@ -492,7 +492,7 @@ static int update_intremap_entry_from_msi_msg(
> >>>                 get_ivrs_mappings(iommu->seg)[alias_id].intremap_table);
> >>>      }
> >>>  
> >>> -    return 0;
> >>> +    return !fresh ? 0 : 1;
> >>>  }
> >>
> >> Simply
> >>
> >>     return fresh;
> >>
> >> ?
> >>
> >>> @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
> >>>      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
> >>>                                              &msi_desc->remap_index,
> >>>                                              msg, &data);
> >>> -    if ( !rc )
> >>> +    if ( rc > 0 )
> >>>      {
> >>>          for ( i = 1; i < nr; ++i )
> >>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
> >>> --- a/xen/drivers/passthrough/vtd/intremap.c
> >>> +++ b/xen/drivers/passthrough/vtd/intremap.c
> >>> @@ -506,6 +506,7 @@ static int msi_msg_to_remap_entry(
> >>>      unsigned int index, i, nr = 1;
> >>>      unsigned long flags;
> >>>      const struct pi_desc *pi_desc = msi_desc->pi_desc;
> >>> +    bool alloc = false;
> >>>  
> >>>      if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
> >>>          nr = msi_desc->msi.nvec;
> >>> @@ -529,6 +530,7 @@ static int msi_msg_to_remap_entry(
> >>>          index = alloc_remap_entry(iommu, nr);
> >>>          for ( i = 0; i < nr; ++i )
> >>>              msi_desc[i].remap_index = index + i;
> >>> +        alloc = true;
> >>>      }
> >>>      else
> >>>          index = msi_desc->remap_index;
> >>> @@ -601,7 +603,7 @@ static int msi_msg_to_remap_entry(
> >>>      unmap_vtd_domain_page(iremap_entries);
> >>>      spin_unlock_irqrestore(&iommu->intremap.lock, flags);
> >>>  
> >>> -    return 0;
> >>> +    return alloc ? 1 : 0;
> >>>  }
> >>
> >> Like above, simply
> >>
> >>     return alloc;
> >>
> >> ?
> > 
> > I wasn't sure whether this was overloading the boolean type and
> > possibly breaking some MISRA rule.  I can adjust.
> 
> What we are to do with Misra's essential type system is entirely unclear at
> this point, aiui.

Thanks, given the findings above I will post v4 with the extra
adjustments.

Roger.