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

[PATCH v2 0/3] 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 switched dump_msi() to not
rely on the cached MSI message, and finally patch 3 changes the logic to
prevent re-writing the same MSI message.

Patches 1 and 2 are new in this version, and have no changelog.

Thanks, Roger.

Roger Pau Monne (3):
  x86/apic: remove delivery and destination mode fields from drivers
  x86/msi: don't use cached address and data fields in msi_desc for
    dump_msi()
  x86/msi: prevent MSI entry re-writes of the same data

 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/include/asm/msi.h      |  5 +-
 xen/arch/x86/io_apic.c              | 16 +++----
 xen/arch/x86/msi.c                  | 73 ++++++++++++++++++-----------
 xen/drivers/passthrough/vtd/iommu.c | 11 ++---
 8 files changed, 62 insertions(+), 56 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] 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>
---
 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..60b39827c851 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;
             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;
     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;
     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;
 
     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 v2 2/3] x86/msi: don't use cached address and data fields in msi_desc for dump_msi()

[Roger Pau Monne]

Instead compose a dummy MSI message just for the purpose of getting the
delivery attributes, which are the same for all messages.  Note that the
previous usage of the cached MSI message wasn't fetching the hardware MSI
fields either.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msi.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6c11d76015fb..8efe41b1526c 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1454,6 +1454,16 @@ void __init early_msi_init(void)
 static void cf_check dump_msi(unsigned char key)
 {
     unsigned int irq;
+    struct msi_msg msg = {};
+    uint32_t addr, data;
+
+    /*
+     * Compose a dummy msg message for the purpose of getting the delivery
+     * attributes.
+     */
+    msi_compose_msg(FIRST_DYNAMIC_VECTOR, NULL, &msg);
+    addr = msg.address_lo;
+    data = msg.data;
 
     printk("MSI information:\n");
 
@@ -1461,7 +1471,7 @@ static void cf_check dump_msi(unsigned char key)
     {
         struct irq_desc *desc = irq_to_desc(irq);
         const struct msi_desc *entry;
-        u32 addr, data, dest32;
+        uint32_t dest32;
         signed char mask;
         struct msi_attrib attr;
         unsigned long flags;
@@ -1495,8 +1505,6 @@ static void cf_check dump_msi(unsigned char key)
             break;
         }
 
-        data = entry->msg.data;
-        addr = entry->msg.address_lo;
         dest32 = entry->msg.dest32;
         attr = entry->msi_attrib;
         if ( entry->msi_attrib.type )
@@ -1512,8 +1520,7 @@ static void cf_check dump_msi(unsigned char key)
             mask = '?';
         printk(" %-6s%4u vec=%02x%7s%6s%3sassert%5s%7s"
                " dest=%08x mask=%d/%c%c/%c\n",
-               type, irq,
-               (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT,
+               type, irq, desc->arch.vector,
                data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
                data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
                data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
-- 
2.48.1

[PATCH v2 3/3] x86/msi: prevent MSI entry re-writes of the same data

[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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes since v1:
 - Add more comments.
 - Simplify dma_msi_set_affinity().
---
 xen/arch/x86/include/asm/msi.h      |  5 +++-
 xen/arch/x86/msi.c                  | 45 ++++++++++++++++++++---------
 xen/drivers/passthrough/vtd/iommu.c | 11 +++----
 3 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 378b85ee947b..4301c58c7c4d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -124,7 +124,8 @@ struct msi_desc {
     int irq;
     int remap_index;         /* index in interrupt remapping table */
 
-    struct msi_msg msg;      /* Last set MSI message */
+    /* Last set MSI message in remappable format if applicable */
+    struct msi_msg msg;
 };
 
 /*
@@ -236,6 +237,8 @@ struct arch_msix {
 };
 
 void early_msi_init(void);
+
+/* If cpu_mask is NULL msg->dest32 is used as the destination APIC ID. */
 void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask,
                      struct msi_msg *msg);
 void __msi_set_enable(pci_sbdf_t sbdf, int pos, int enable);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 8efe41b1526c..3c9fdc9b7bf5 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -152,11 +152,13 @@ static bool msix_memory_decoded(const struct pci_dev *dev, unsigned int pos)
 }
 
 /*
- * MSI message composition
+ * MSI message composition.
+ * If cpu_mask is NULL msg->dest32 is used as the destination APIC ID.
  */
 void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg *msg)
 {
-    memset(msg, 0, sizeof(*msg));
+    msg->address = 0;
+    msg->data = 0;
 
     if ( vector < FIRST_DYNAMIC_VECTOR )
         return;
@@ -186,8 +188,6 @@ void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg
 
 static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
-    entry->msg = *msg;
-
     if ( iommu_intremap != iommu_intremap_off )
     {
         int rc;
@@ -198,6 +198,20 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
             return rc;
     }
 
+    /*
+     * Avoid updating the MSI entry if the address and data fields haven't
+     * changed.  When using interrupt remapping changing the MSI affinity
+     * shouldn't change the interrupt remapping table index, and hence the MSI
+     * address and data fields should remain the same.
+     */
+    if ( entry->msg.address == msg->address && entry->msg.data == msg->data )
+    {
+        entry->msg.dest32 = msg->dest32;
+        return 0;
+    }
+
+    entry->msg = *msg;
+
     switch ( entry->msi_attrib.type )
     {
     case PCI_CAP_ID_MSI:
@@ -243,21 +257,15 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 void cf_check set_msi_affinity(struct irq_desc *desc, const cpumask_t *mask)
 {
     struct msi_msg msg;
-    unsigned int dest;
     struct msi_desc *msi_desc = desc->msi_desc;
 
-    dest = set_desc_affinity(desc, mask);
-    if ( dest == BAD_APICID || !msi_desc )
+    msg.dest32 = set_desc_affinity(desc, mask);
+    if ( msg.dest32 == BAD_APICID || !msi_desc )
         return;
 
     ASSERT(spin_is_locked(&desc->lock));
 
-    msg = msi_desc->msg;
-    msg.data &= ~MSI_DATA_VECTOR_MASK;
-    msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
-    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
-    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
-    msg.dest32 = dest;
+    msi_compose_msg(desc->arch.vector, NULL, &msg);
 
     write_msi_msg(msi_desc, &msg);
 }
@@ -1402,7 +1410,16 @@ int pci_restore_msi_state(struct pci_dev *pdev)
         }
         type = entry->msi_attrib.type;
 
-        msg = entry->msg;
+        /*
+         * Instead of re-writing the current MSI message, regenerate it and
+         * force a write, as that's simpler than adding support for writing
+         * already translated messages.
+         */
+        msg.dest32 = entry->msg.dest32;
+        /* Pass NULL as the cpu mask to use the value in msg.dest32. */
+        msi_compose_msg(desc->arch.vector, NULL, &msg);
+        /* Zero the cached message to unconditionally trigger a re-write. */
+        entry->msg = (typeof(entry->msg)){};
         write_msi_msg(entry, &msg);
 
         for ( i = 0; ; )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a1927d9f126d..e03e9dfbc595 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1183,22 +1183,19 @@ static void cf_check dma_msi_set_affinity(
     struct irq_desc *desc, const cpumask_t *mask)
 {
     struct msi_msg msg;
-    unsigned int dest;
     unsigned long flags;
     struct vtd_iommu *iommu = desc->action->dev_id;
 
-    dest = set_desc_affinity(desc, mask);
-    if (dest == BAD_APICID){
+    msg.dest32 = set_desc_affinity(desc, mask);
+    if ( msg.dest32 == BAD_APICID )
+    {
         dprintk(XENLOG_ERR VTDPREFIX, "Set iommu interrupt affinity error!\n");
         return;
     }
 
     msi_compose_msg(desc->arch.vector, NULL, &msg);
-    msg.dest32 = dest;
     if (x2apic_enabled)
-        msg.address_hi = dest & 0xFFFFFF00;
-    ASSERT(!(msg.address_lo & MSI_ADDR_DEST_ID_MASK));
-    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
+        msg.address_hi = msg.dest32 & 0xFFFFFF00;
     iommu->msi.msg = msg;
 
     spin_lock_irqsave(&iommu->register_lock, flags);
-- 
2.48.1

Re: [PATCH v2 1/3] x86/apic: remove delivery and destination mode fields from drivers

[Andrew Cooper]

On 06/03/2025 2:57 pm, Roger Pau Monne wrote:
> 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>
> ---
>  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(-)

Definitely going in a good direction.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 1/3] x86/apic: remove delivery and destination mode fields from drivers

[Jan Beulich]

On 06.03.2025 15:57, Roger Pau Monne wrote:
> --- 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,
> --- 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,
> --- 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,

Like we had it everywhere above, ...

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

... here and below these zeros would better gain a comment, or be expressed
as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK).

Jan

Re: [PATCH v2 1/3] x86/apic: remove delivery and destination mode fields from drivers

[Roger Pau Monné]

On Thu, Mar 06, 2025 at 04:33:37PM +0100, Jan Beulich wrote:
> On 06.03.2025 15:57, Roger Pau Monne wrote:
> > --- 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,
> > --- 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,
> > --- 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,
> 
> Like we had it everywhere above, ...
> 
> > --- 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;
> 
> ... here and below these zeros would better gain a comment, or be expressed
> as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK).

I've started adding those comments, but then I got the impression they
where a bit redundant, many of the setting of the fields didn't have a
matching comment.  I was even tempted to just not setting the field at
all, seeing as the structure is zeroed.

Also this is the IO-APIC RTE, so it feels a bit out of place to use
the local APIC defines?

I will add a comment if you are fine with it.

Thanks, Roger.

Re: [PATCH v2 1/3] x86/apic: remove delivery and destination mode fields from drivers

[Jan Beulich]

On 06.03.2025 16:54, Roger Pau Monné wrote:
> On Thu, Mar 06, 2025 at 04:33:37PM +0100, Jan Beulich wrote:
>> On 06.03.2025 15:57, Roger Pau Monne wrote:
>>> --- 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,
>>> --- 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,
>>> --- 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,
>>
>> Like we had it everywhere above, ...
>>
>>> --- 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;
>>
>> ... here and below these zeros would better gain a comment, or be expressed
>> as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK).
> 
> I've started adding those comments, but then I got the impression they
> where a bit redundant, many of the setting of the fields didn't have a
> matching comment.  I was even tempted to just not setting the field at
> all, seeing as the structure is zeroed.
> 
> Also this is the IO-APIC RTE, so it feels a bit out of place to use
> the local APIC defines?

Maybe. There is a certain level of correlation, but yes, it may end up
being confusing.

> I will add a comment if you are fine with it.

I am.

Jan

Re: [PATCH v2 2/3] x86/msi: don't use cached address and data fields in msi_desc for dump_msi()

[Jan Beulich]

On 06.03.2025 15:57, Roger Pau Monne wrote:
> Instead compose a dummy MSI message just for the purpose of getting the
> delivery attributes, which are the same for all messages.  Note that the
> previous usage of the cached MSI message wasn't fetching the hardware MSI
> fields either.

This feels not future proof. There's no guarantee special IRQs (HPET, IOMMU)
would necessarily use msi_compose_msg() (or any open-coded subset thereof).

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1454,6 +1454,16 @@ void __init early_msi_init(void)
>  static void cf_check dump_msi(unsigned char key)
>  {
>      unsigned int irq;
> +    struct msi_msg msg = {};
> +    uint32_t addr, data;
> +
> +    /*
> +     * Compose a dummy msg message for the purpose of getting the delivery
> +     * attributes.
> +     */
> +    msi_compose_msg(FIRST_DYNAMIC_VECTOR, NULL, &msg);
> +    addr = msg.address_lo;
> +    data = msg.data;
>  
>      printk("MSI information:\n");
>  
> @@ -1461,7 +1471,7 @@ static void cf_check dump_msi(unsigned char key)
>      {
>          struct irq_desc *desc = irq_to_desc(irq);
>          const struct msi_desc *entry;
> -        u32 addr, data, dest32;
> +        uint32_t dest32;
>          signed char mask;
>          struct msi_attrib attr;
>          unsigned long flags;
> @@ -1495,8 +1505,6 @@ static void cf_check dump_msi(unsigned char key)
>              break;
>          }
>  
> -        data = entry->msg.data;
> -        addr = entry->msg.address_lo;
>          dest32 = entry->msg.dest32;
>          attr = entry->msi_attrib;
>          if ( entry->msi_attrib.type )
> @@ -1512,8 +1520,7 @@ static void cf_check dump_msi(unsigned char key)
>              mask = '?';
>          printk(" %-6s%4u vec=%02x%7s%6s%3sassert%5s%7s"
>                 " dest=%08x mask=%d/%c%c/%c\n",
> -               type, irq,
> -               (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT,
> +               type, irq, desc->arch.vector,

We've already dropped desc's lock, so shouldn't be de-referencing desc anymore.

>                 data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
>                 data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
>                 data & MSI_DATA_LEVEL_ASSERT ? "" : "de",

To add to the comment at the top, plus taking patch 1 into account: If we
uniformly used the output of the dummy msi_compose_msg() invocation, why would
we even bother to log information conditionally upon what is in data/addr?

Jan

Re: [PATCH v2 2/3] x86/msi: don't use cached address and data fields in msi_desc for dump_msi()

[Roger Pau Monné]

On Thu, Mar 06, 2025 at 05:45:27PM +0100, Jan Beulich wrote:
> On 06.03.2025 15:57, Roger Pau Monne wrote:
> > Instead compose a dummy MSI message just for the purpose of getting the
> > delivery attributes, which are the same for all messages.  Note that the
> > previous usage of the cached MSI message wasn't fetching the hardware MSI
> > fields either.
> 
> This feels not future proof. There's no guarantee special IRQs (HPET, IOMMU)
> would necessarily use msi_compose_msg() (or any open-coded subset thereof).

Hm, even if not using msi_compose_msg() I don't see how any device
would use a different MSI settings from physical delivery and fixed
destination.  I think it's unlikely for a device to use anything
different from the current values set by msi_compose_msg().

Otherwise I can see about returning whether the entry needs to be
updated from iommu_update_ire_from_msi() (if the offset into the IRT
for the entry has changed).  However that requires adding code to both
AMD and Intel IOMMU implementations, and will need at least a way to
signal that the MSI fields must forcefully be written on resume.

> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -1454,6 +1454,16 @@ void __init early_msi_init(void)
> >  static void cf_check dump_msi(unsigned char key)
> >  {
> >      unsigned int irq;
> > +    struct msi_msg msg = {};
> > +    uint32_t addr, data;
> > +
> > +    /*
> > +     * Compose a dummy msg message for the purpose of getting the delivery
> > +     * attributes.
> > +     */
> > +    msi_compose_msg(FIRST_DYNAMIC_VECTOR, NULL, &msg);
> > +    addr = msg.address_lo;
> > +    data = msg.data;
> >  
> >      printk("MSI information:\n");
> >  
> > @@ -1461,7 +1471,7 @@ static void cf_check dump_msi(unsigned char key)
> >      {
> >          struct irq_desc *desc = irq_to_desc(irq);
> >          const struct msi_desc *entry;
> > -        u32 addr, data, dest32;
> > +        uint32_t dest32;
> >          signed char mask;
> >          struct msi_attrib attr;
> >          unsigned long flags;
> > @@ -1495,8 +1505,6 @@ static void cf_check dump_msi(unsigned char key)
> >              break;
> >          }
> >  
> > -        data = entry->msg.data;
> > -        addr = entry->msg.address_lo;
> >          dest32 = entry->msg.dest32;
> >          attr = entry->msi_attrib;
> >          if ( entry->msi_attrib.type )
> > @@ -1512,8 +1520,7 @@ static void cf_check dump_msi(unsigned char key)
> >              mask = '?';
> >          printk(" %-6s%4u vec=%02x%7s%6s%3sassert%5s%7s"
> >                 " dest=%08x mask=%d/%c%c/%c\n",
> > -               type, irq,
> > -               (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT,
> > +               type, irq, desc->arch.vector,
> 
> We've already dropped desc's lock, so shouldn't be de-referencing desc anymore.

Right, I need to cache it before dropping the lock.

> >                 data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> >                 data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> >                 data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> 
> To add to the comment at the top, plus taking patch 1 into account: If we
> uniformly used the output of the dummy msi_compose_msg() invocation, why would
> we even bother to log information conditionally upon what is in data/addr?

We could change what's set by msi_compose_msg(), and then the
information here would be incorrect (if hardcoded).

Thanks, Roger.

Re: [PATCH v2 2/3] x86/msi: don't use cached address and data fields in msi_desc for dump_msi()

[Jan Beulich]

On 06.03.2025 18:56, Roger Pau Monné wrote:
> On Thu, Mar 06, 2025 at 05:45:27PM +0100, Jan Beulich wrote:
>> On 06.03.2025 15:57, Roger Pau Monne wrote:
>>> Instead compose a dummy MSI message just for the purpose of getting the
>>> delivery attributes, which are the same for all messages.  Note that the
>>> previous usage of the cached MSI message wasn't fetching the hardware MSI
>>> fields either.
>>
>> This feels not future proof. There's no guarantee special IRQs (HPET, IOMMU)
>> would necessarily use msi_compose_msg() (or any open-coded subset thereof).
> 
> Hm, even if not using msi_compose_msg() I don't see how any device
> would use a different MSI settings from physical delivery and fixed
> destination.  I think it's unlikely for a device to use anything
> different from the current values set by msi_compose_msg().

I'm not entirely sure about this. If e.g. broadcast was possible to send via
MSI, I could see use cases. Potentially also for e.g. NMI.

> Otherwise I can see about returning whether the entry needs to be
> updated from iommu_update_ire_from_msi() (if the offset into the IRT
> for the entry has changed).  However that requires adding code to both
> AMD and Intel IOMMU implementations, and will need at least a way to
> signal that the MSI fields must forcefully be written on resume.

While that may indeed be more intrusive, it feels like it may be the less
risky approach (as to overlooking yet another case where we rely on the
untranslated message to be stored). Overall I can only say that I'm
uncertain which of the two approaches would end up being better.

>>> @@ -1512,8 +1520,7 @@ static void cf_check dump_msi(unsigned char key)
>>>              mask = '?';
>>>          printk(" %-6s%4u vec=%02x%7s%6s%3sassert%5s%7s"
>>>                 " dest=%08x mask=%d/%c%c/%c\n",
>>> -               type, irq,
>>> -               (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT,
>>> +               type, irq, desc->arch.vector,
>>
>> We've already dropped desc's lock, so shouldn't be de-referencing desc anymore.
> 
> Right, I need to cache it before dropping the lock.
> 
>>>                 data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
>>>                 data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
>>>                 data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
>>
>> To add to the comment at the top, plus taking patch 1 into account: If we
>> uniformly used the output of the dummy msi_compose_msg() invocation, why would
>> we even bother to log information conditionally upon what is in data/addr?
> 
> We could change what's set by msi_compose_msg(), and then the
> information here would be incorrect (if hardcoded).

Hmm, yes, that could happen, but would hopefully be caught in review (much
like it happened this time round).

Jan