[PATCH v4 0/3] x86/pci: reduce PCI accesses

[PATCH v4 0/3] x86/pci: reduce PCI accesses

[Roger Pau Monne]

Hello,

Patches 1 and 2 are new bugfixes in this version.  Patch 3 is still
mostly as v3, just with the extra logic to ensure vmx_pi_update_irte()
correctness.

Thanks, Roger.

Roger Pau Monne (3):
  x86/vmx: fix posted interrupts usage of msi_desc->msg field
  x86/hvm: check return code of hvm_pi_update_irte when binding
  x86/iommu: avoid MSI address and data writes if IRT index hasn't
    changed

 xen/arch/x86/hpet.c                      |  6 +++++-
 xen/arch/x86/hvm/vmx/vmx.c               | 20 +++++++++++++++++++-
 xen/arch/x86/include/asm/msi.h           |  2 +-
 xen/arch/x86/msi.c                       | 11 ++++++-----
 xen/drivers/passthrough/amd/iommu_intr.c |  4 ++--
 xen/drivers/passthrough/vtd/intremap.c   |  4 +++-
 xen/drivers/passthrough/x86/hvm.c        | 10 +++++++++-
 xen/include/xen/iommu.h                  |  6 ++++++
 8 files changed, 51 insertions(+), 12 deletions(-)

-- 
2.48.1

[PATCH v4 1/3] x86/vmx: fix posted interrupts usage of msi_desc->msg field

[Roger Pau Monne]

The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
field contain a translated MSI message, instead of the expected
untranslated one.  This breaks dump_msi(), that use the data in
msi_desc->msg to print the interrupt details.

Fix this by introducing a dummy local msi_msg, and use it with
iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
message not changing, so there's no need to propagate the resulting msi_msg
to the hardware, and the contents can be ignored.

Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vmx.c     | 9 ++++++++-
 xen/arch/x86/include/asm/msi.h | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0241303b4bf4..c407513af624 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
     struct irq_desc *desc;
     struct msi_desc *msi_desc;
+    /*
+     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
+     * updates the guest vector, but not the other IRTE fields.  As such the
+     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
+     * if not consumed, zero the contents to avoid possible stack leaks.
+     */
+    struct msi_msg msg = {};
     int rc;
 
     desc = pirq_spin_lock_irq_desc(pirq, NULL);
@@ -415,7 +422,7 @@ 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);
+    return iommu_update_ire_from_msi(msi_desc, &msg);
 
  unlock_out:
     spin_unlock_irq(&desc->lock);
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 378b85ee947b..975d0f26b35d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -124,7 +124,7 @@ struct msi_desc {
     int irq;
     int remap_index;         /* index in interrupt remapping table */
 
-    struct msi_msg msg;      /* Last set MSI message */
+    struct msi_msg msg;      /* Last set MSI message (untranslated) */
 };
 
 /*
-- 
2.48.1

Re: [PATCH v4 1/3] x86/vmx: fix posted interrupts usage of msi_desc->msg field

[Jan Beulich]

On 11.03.2025 13:06, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
>      const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
>      struct irq_desc *desc;
>      struct msi_desc *msi_desc;
> +    /*
> +     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
> +     * updates the guest vector, but not the other IRTE fields.  As such the
> +     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
> +     * if not consumed, zero the contents to avoid possible stack leaks.
> +     */
> +    struct msi_msg msg = {};

What the comment says is true only when pi_desc != NULL. As can be seen in
context above, it can very well be NULL here, though (which isn't to say
that I'm convinced the NULL case is handled correctly here). I'd view it as
more safe anyway if you set msg from msi_desc->msg.

Jan

Re: [PATCH v4 1/3] x86/vmx: fix posted interrupts usage of msi_desc->msg field

[Roger Pau Monné]

On Tue, Mar 11, 2025 at 02:10:04PM +0100, Jan Beulich wrote:
> On 11.03.2025 13:06, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
> >      const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
> >      struct irq_desc *desc;
> >      struct msi_desc *msi_desc;
> > +    /*
> > +     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
> > +     * updates the guest vector, but not the other IRTE fields.  As such the
> > +     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
> > +     * if not consumed, zero the contents to avoid possible stack leaks.
> > +     */
> > +    struct msi_msg msg = {};
> 
> What the comment says is true only when pi_desc != NULL. As can be seen in
> context above, it can very well be NULL here, though (which isn't to say
> that I'm convinced the NULL case is handled correctly here). I'd view it as
> more safe anyway if you set msg from msi_desc->msg.

Indeed that's likely better.  I'm also unsure the teardown is correct
(or needed), but I didn't want to deal with that right now.

Thanks, Roger.

[PATCH v5 1/3] x86/vmx: fix posted interrupts usage of msi_desc->msg field

[Roger Pau Monne]

The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
field contain a translated MSI message, instead of the expected
untranslated one.  This breaks dump_msi(), that use the data in
msi_desc->msg to print the interrupt details.

Fix this by introducing a dummy local msi_msg, and use it with
iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
message not changing, so there's no need to propagate the resulting msi_msg
to the hardware, and the contents can be ignored.

Additionally add a comment to clarify that msi_desc->msg must always
contain the untranslated MSI message.

Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v4:
 - Initialize the local msi_msg with the contents of msi_desc->msg.

Changes since v3:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vmx.c     | 4 +++-
 xen/arch/x86/include/asm/msi.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0241303b4bf4..23b7ecd77f85 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -396,6 +396,7 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
     struct irq_desc *desc;
     struct msi_desc *msi_desc;
+    struct msi_msg msg;
     int rc;
 
     desc = pirq_spin_lock_irq_desc(pirq, NULL);
@@ -410,12 +411,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     }
     msi_desc->pi_desc = pi_desc;
     msi_desc->gvec = gvec;
+    msg = msi_desc->msg;
 
     spin_unlock_irq(&desc->lock);
 
     ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
 
-    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+    return iommu_update_ire_from_msi(msi_desc, &msg);
 
  unlock_out:
     spin_unlock_irq(&desc->lock);
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 378b85ee947b..975d0f26b35d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -124,7 +124,7 @@ struct msi_desc {
     int irq;
     int remap_index;         /* index in interrupt remapping table */
 
-    struct msi_msg msg;      /* Last set MSI message */
+    struct msi_msg msg;      /* Last set MSI message (untranslated) */
 };
 
 /*
-- 
2.48.1

Re: [PATCH v5 1/3] x86/vmx: fix posted interrupts usage of msi_desc->msg field

[Jan Beulich]

On 11.03.2025 16:27, Roger Pau Monne wrote:
> The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
> field contain a translated MSI message, instead of the expected
> untranslated one.  This breaks dump_msi(), that use the data in
> msi_desc->msg to print the interrupt details.
> 
> Fix this by introducing a dummy local msi_msg, and use it with
> iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
> message not changing, so there's no need to propagate the resulting msi_msg
> to the hardware, and the contents can be ignored.
> 
> Additionally add a comment to clarify that msi_desc->msg must always
> contain the untranslated MSI message.
> 
> Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v4 2/3] x86/hvm: check return code of hvm_pi_update_irte when binding

[Roger Pau Monne]

Consume the return code from hvm_pi_update_irte(), and propagate the error
back to the caller if hvm_pi_update_irte() fails.

Fixes: 35a1caf8b6b5 ('pass-through: update IRTE according to guest interrupt config changes')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - New in this version.
---
 xen/drivers/passthrough/x86/hvm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index f5faff7a499a..47de6953fdf8 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -381,7 +381,15 @@ int pt_irq_create_bind(
 
         /* Use interrupt posting if it is supported. */
         if ( iommu_intpost )
-            hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
+        {
+            rc = hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec);
+
+            if ( rc )
+            {
+                pt_irq_destroy_bind(d, pt_irq_bind);
+                return rc;
+            }
+        }
 
         if ( pt_irq_bind->u.msi.gflags & XEN_DOMCTL_VMSI_X86_UNMASKED )
         {
-- 
2.48.1

Re: [PATCH v4 2/3] x86/hvm: check return code of hvm_pi_update_irte when binding

[Jan Beulich]

On 11.03.2025 13:06, Roger Pau Monne wrote:
> Consume the return code from hvm_pi_update_irte(), and propagate the error
> back to the caller if hvm_pi_update_irte() fails.
> 
> Fixes: 35a1caf8b6b5 ('pass-through: update IRTE according to guest interrupt config changes')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v4 3/3] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes since v3:
 - Assert MSI fields are never updated in vmx_pi_update_irte().
 - Directly return booleans from msi_msg_to_remap_entry() and
   update_intremap_entry_from_msi_msg().

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               | 13 ++++++++++++-
 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, 34 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 c407513af624..7535a45b31fc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -422,7 +422,18 @@ 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, &msg);
+    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;
 
  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..9abdc38053d7 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;
 }
 
 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..b3b53518e294 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;
 }
 
 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