[PATCH RFC 0/6] x86/ioapic: fix edge triggered interrupt migration

[PATCH RFC 0/6] x86/ioapic: fix edge triggered interrupt migration

[Roger Pau Monne]

Hello,

Following series attempts to solve the issue with IO-APIC edge triggered
interrupts seeing an inconsistent RTE or IRTE when injected while being
migrated.

It's currently RFC because some patches have post commit message notes,
and because I'm not sure if patch 1 is really needed.  I originally had
the idea of suggesting to only backport patch 1 in order to fix the
issue in older releases, while leaving the more complex (and thus
error prone) IOMMU changes in unstable.  Note however that patch 1 is
just a workaround to prevent interrupts seeing inconsistent entries
while being updated, masking the entry just makes the warning go away,
but the interrupt will be lost.

Thanks, Roger.

Roger Pau Monne (6):
  x86/ioapic: set disable hook for masking edge interrupts
  x86/ioapic: add a raw field to RTE struct
  x86/ioapic: RTE modifications must use ioapic_write_entry
  x86/iommu: pass full IO-APIC RTE for remapping table update
  amd/iommu: atomically update remapping entries when possible
  x86/ioapic: mask entry while updating

 xen/arch/x86/include/asm/io_apic.h       |  72 ++++++-------
 xen/arch/x86/include/asm/iommu.h         |   3 +-
 xen/arch/x86/io_apic.c                   |  81 ++++++++++-----
 xen/drivers/passthrough/amd/iommu.h      |   2 +-
 xen/drivers/passthrough/amd/iommu_intr.c | 115 +++++++--------------
 xen/drivers/passthrough/vtd/extern.h     |   2 +-
 xen/drivers/passthrough/vtd/intremap.c   | 125 +++++++++++------------
 xen/drivers/passthrough/x86/iommu.c      |   4 +-
 xen/include/xen/iommu.h                  |   3 +-
 9 files changed, 195 insertions(+), 212 deletions(-)

-- 
2.35.1

[PATCH RFC 1/6] x86/ioapic: set disable hook for masking edge interrupts

[Roger Pau Monne]

Allow disabling (masking) IO-APIC pins set to edge trigger mode.  This
is required in order to safely migrate such interrupts between CPUs,
as the write to update the IO-APIC RTE (or the IRTE) is not done
atomically, so there's a window where there's a mismatch between the
destination CPU and the vector:

(XEN) CPU1: No irq handler for vector b5 (IRQ -11, LAPIC)
(XEN) IRQ10 a=0002[0002,0008] v=bd[b5] t=IO-APIC-edge s=00000030

The main risk with masking edge triggered interrupts is losing them,
but getting them injected while in the process of updating the RTE is
equally harmful as the interrupts gets lost anyway.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/io_apic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index c086f40f63..2e5964640b 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1782,7 +1782,7 @@ static hw_irq_controller ioapic_edge_type = {
     .startup 	= startup_edge_ioapic_irq,
     .shutdown 	= irq_shutdown_none,
     .enable 	= unmask_IO_APIC_irq,
-    .disable 	= irq_disable_none,
+    .disable 	= mask_IO_APIC_irq,
     .ack 		= ack_edge_ioapic_irq,
     .set_affinity 	= set_ioapic_affinity_irq,
 };
-- 
2.35.1

Re: [PATCH RFC 1/6] x86/ioapic: set disable hook for masking edge interrupts

[Jan Beulich]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> Allow disabling (masking) IO-APIC pins set to edge trigger mode.  This
> is required in order to safely migrate such interrupts between CPUs,
> as the write to update the IO-APIC RTE (or the IRTE) is not done
> atomically,

For IRTE on VT-d we use cmpxchg16b if available (i.e. virtually always).

> so there's a window where there's a mismatch between the
> destination CPU and the vector:
> 
> (XEN) CPU1: No irq handler for vector b5 (IRQ -11, LAPIC)
> (XEN) IRQ10 a=0002[0002,0008] v=bd[b5] t=IO-APIC-edge s=00000030

I think this needs some further explanation, as we generally move
edge IRQs only when an un-acked interrupt is in flight (and hence
no further one can arrive).

Jan

> The main risk with masking edge triggered interrupts is losing them,
> but getting them injected while in the process of updating the RTE is
> equally harmful as the interrupts gets lost anyway.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/io_apic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index c086f40f63..2e5964640b 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1782,7 +1782,7 @@ static hw_irq_controller ioapic_edge_type = {
>      .startup 	= startup_edge_ioapic_irq,
>      .shutdown 	= irq_shutdown_none,
>      .enable 	= unmask_IO_APIC_irq,
> -    .disable 	= irq_disable_none,
> +    .disable 	= mask_IO_APIC_irq,
>      .ack 		= ack_edge_ioapic_irq,
>      .set_affinity 	= set_ioapic_affinity_irq,
>  };

Re: [PATCH RFC 1/6] x86/ioapic: set disable hook for masking edge interrupts

[Roger Pau Monné]

On Fri, Jun 03, 2022 at 03:19:34PM +0200, Jan Beulich wrote:
> On 21.04.2022 15:21, Roger Pau Monne wrote:
> > Allow disabling (masking) IO-APIC pins set to edge trigger mode.  This
> > is required in order to safely migrate such interrupts between CPUs,
> > as the write to update the IO-APIC RTE (or the IRTE) is not done
> > atomically,
> 
> For IRTE on VT-d we use cmpxchg16b if available (i.e. virtually always).
> 
> > so there's a window where there's a mismatch between the
> > destination CPU and the vector:
> > 
> > (XEN) CPU1: No irq handler for vector b5 (IRQ -11, LAPIC)
> > (XEN) IRQ10 a=0002[0002,0008] v=bd[b5] t=IO-APIC-edge s=00000030
> 
> I think this needs some further explanation, as we generally move
> edge IRQs only when an un-acked interrupt is in flight (and hence
> no further one can arrive).

A further one can arrive as soon as you modify either the vector or
the destination fields of the IO-APIC RTE, as then the non-EOI'ed
lapic vector is no longer there (because you have moved to a different
destination or vector).

This is the issue with updating the IO-APIC RTE using two separate
writes: even when using interrupt remapping the IRTE cannot be
atomically updated and there's a window where the interrupt is not
masked, but the destination and vector fields are not in sync, because
they reside in different parts of the RTE (destination is in the high
32bits, vector in the low 32bits part of the RTE).

Thanks, Roger.

Re: [PATCH RFC 1/6] x86/ioapic: set disable hook for masking edge interrupts

[Jan Beulich]

On 03.06.2022 16:53, Roger Pau Monné wrote:
> On Fri, Jun 03, 2022 at 03:19:34PM +0200, Jan Beulich wrote:
>> On 21.04.2022 15:21, Roger Pau Monne wrote:
>>> Allow disabling (masking) IO-APIC pins set to edge trigger mode.  This
>>> is required in order to safely migrate such interrupts between CPUs,
>>> as the write to update the IO-APIC RTE (or the IRTE) is not done
>>> atomically,
>>
>> For IRTE on VT-d we use cmpxchg16b if available (i.e. virtually always).
>>
>>> so there's a window where there's a mismatch between the
>>> destination CPU and the vector:
>>>
>>> (XEN) CPU1: No irq handler for vector b5 (IRQ -11, LAPIC)
>>> (XEN) IRQ10 a=0002[0002,0008] v=bd[b5] t=IO-APIC-edge s=00000030
>>
>> I think this needs some further explanation, as we generally move
>> edge IRQs only when an un-acked interrupt is in flight (and hence
>> no further one can arrive).
> 
> A further one can arrive as soon as you modify either the vector or
> the destination fields of the IO-APIC RTE, as then the non-EOI'ed
> lapic vector is no longer there (because you have moved to a different
> destination or vector).

Right - this is what I'm asking you to spell out in the description.

Jan

> This is the issue with updating the IO-APIC RTE using two separate
> writes: even when using interrupt remapping the IRTE cannot be
> atomically updated and there's a window where the interrupt is not
> masked, but the destination and vector fields are not in sync, because
> they reside in different parts of the RTE (destination is in the high
> 32bits, vector in the low 32bits part of the RTE).
> 
> Thanks, Roger.
> 

[PATCH RFC 2/6] x86/ioapic: add a raw field to RTE struct

[Roger Pau Monne]

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/io_apic.h | 57 ++++++++++++++++--------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index ef0878b09e..a558bb063c 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -89,35 +89,38 @@ enum ioapic_irq_destination_types {
 };
 
 struct IO_APIC_route_entry {
-    unsigned int vector:8;
-    unsigned int delivery_mode:3; /*
-                                   * 000: FIXED
-                                   * 001: lowest prio
-                                   * 111: ExtINT
-                                   */
-    unsigned int dest_mode:1;     /* 0: physical, 1: logical */
-    unsigned int delivery_status:1;
-    unsigned int polarity:1;      /* 0: low, 1: high */
-    unsigned int irr:1;
-    unsigned int trigger:1;       /* 0: edge, 1: level */
-    unsigned int mask:1;          /* 0: enabled, 1: disabled */
-    unsigned int __reserved_2:15;
-
     union {
         struct {
-            unsigned int __reserved_1:24;
-            unsigned int physical_dest:4;
-            unsigned int __reserved_2:4;
-        } physical;
-
-        struct {
-            unsigned int __reserved_1:24;
-            unsigned int logical_dest:8;
-        } logical;
-
-        /* used when Interrupt Remapping with EIM is enabled */
-        unsigned int dest32;
-    } dest;
+            unsigned int vector:8;
+            unsigned int delivery_mode:3; /*
+                                           * 000: FIXED
+                                           * 001: lowest prio
+                                           * 111: ExtINT
+                                           */
+            unsigned int dest_mode:1;     /* 0: physical, 1: logical */
+            unsigned int delivery_status:1;
+            unsigned int polarity:1;      /* 0: low, 1: high */
+            unsigned int irr:1;
+            unsigned int trigger:1;       /* 0: edge, 1: level */
+            unsigned int mask:1;          /* 0: enabled, 1: disabled */
+            unsigned int __reserved_2:15;
+
+            union {
+                struct {
+                    unsigned int __reserved_1:24;
+                    unsigned int physical_dest:4;
+                    unsigned int __reserved_2:4;
+                } physical;
+
+                struct {
+                    unsigned int __reserved_1:24;
+                    unsigned int logical_dest:8;
+                } logical;
+                unsigned int dest32;
+            } dest;
+        };
+        uint64_t raw;
+    };
 };
 
 /*
-- 
2.35.1

Re: [PATCH RFC 2/6] x86/ioapic: add a raw field to RTE struct

[Jan Beulich]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Once seeing the purpose (in a later patch, I suppose) I certainly
don't mind. We do have a couple of literal initializers, though (see
e.g. the top of ioapic_guest_write()). Do those still compile fine
(warning free) even with old gcc?

Jan

> --- a/xen/arch/x86/include/asm/io_apic.h
> +++ b/xen/arch/x86/include/asm/io_apic.h
> @@ -89,35 +89,38 @@ enum ioapic_irq_destination_types {
>  };
>  
>  struct IO_APIC_route_entry {
> -    unsigned int vector:8;
> -    unsigned int delivery_mode:3; /*
> -                                   * 000: FIXED
> -                                   * 001: lowest prio
> -                                   * 111: ExtINT
> -                                   */
> -    unsigned int dest_mode:1;     /* 0: physical, 1: logical */
> -    unsigned int delivery_status:1;
> -    unsigned int polarity:1;      /* 0: low, 1: high */
> -    unsigned int irr:1;
> -    unsigned int trigger:1;       /* 0: edge, 1: level */
> -    unsigned int mask:1;          /* 0: enabled, 1: disabled */
> -    unsigned int __reserved_2:15;
> -
>      union {
>          struct {
> -            unsigned int __reserved_1:24;
> -            unsigned int physical_dest:4;
> -            unsigned int __reserved_2:4;
> -        } physical;
> -
> -        struct {
> -            unsigned int __reserved_1:24;
> -            unsigned int logical_dest:8;
> -        } logical;
> -
> -        /* used when Interrupt Remapping with EIM is enabled */
> -        unsigned int dest32;
> -    } dest;
> +            unsigned int vector:8;
> +            unsigned int delivery_mode:3; /*
> +                                           * 000: FIXED
> +                                           * 001: lowest prio
> +                                           * 111: ExtINT
> +                                           */
> +            unsigned int dest_mode:1;     /* 0: physical, 1: logical */
> +            unsigned int delivery_status:1;
> +            unsigned int polarity:1;      /* 0: low, 1: high */
> +            unsigned int irr:1;
> +            unsigned int trigger:1;       /* 0: edge, 1: level */
> +            unsigned int mask:1;          /* 0: enabled, 1: disabled */
> +            unsigned int __reserved_2:15;
> +
> +            union {
> +                struct {
> +                    unsigned int __reserved_1:24;
> +                    unsigned int physical_dest:4;
> +                    unsigned int __reserved_2:4;
> +                } physical;
> +
> +                struct {
> +                    unsigned int __reserved_1:24;
> +                    unsigned int logical_dest:8;
> +                } logical;
> +                unsigned int dest32;
> +            } dest;
> +        };
> +        uint64_t raw;
> +    };
>  };
>  
>  /*

Re: [PATCH RFC 2/6] x86/ioapic: add a raw field to RTE struct

[Roger Pau Monné]

On Fri, Jun 03, 2022 at 03:24:37PM +0200, Jan Beulich wrote:
> On 21.04.2022 15:21, Roger Pau Monne wrote:
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Once seeing the purpose (in a later patch, I suppose) I certainly
> don't mind. We do have a couple of literal initializers, though (see
> e.g. the top of ioapic_guest_write()). Do those still compile fine
> (warning free) even with old gcc?

Will likely need to test it with the gitlab CI.

Thanks, Roger.

[PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry

[Roger Pau Monne]

Do not allow to write to RTE registers using io_apic_write and instead
require changes to RTE to be performed using ioapic_write_entry.

This is in preparation for passing the full contents of the RTE to the
IOMMU interrupt remapping handlers, so remapping entries for IO-APIC
RTEs can be updated atomically when possible.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/io_apic.h | 15 ++----------
 xen/arch/x86/io_apic.c             | 37 +++++++++++++++---------------
 2 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index a558bb063c..c26261aecb 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -161,22 +161,11 @@ static inline void __io_apic_write(unsigned int apic, unsigned int reg, unsigned
 
 static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned int value)
 {
-    if ( ioapic_reg_remapped(reg) )
-        return iommu_update_ire_from_apic(apic, reg, value);
+    /* RTE writes must use ioapic_write_entry. */
+    BUG_ON(reg >= 0x10);
     __io_apic_write(apic, reg, value);
 }
 
-/*
- * Re-write a value: to be used for read-modify-write
- * cycles where the read already set up the index register.
- */
-static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
-{
-    if ( ioapic_reg_remapped(reg) )
-        return iommu_update_ire_from_apic(apic, reg, value);
-    *(IO_APIC_BASE(apic) + 4) = value;
-}
-
 /* 1 if "noapic" boot option passed */
 extern bool skip_ioapic_setup;
 extern bool ioapic_ack_new;
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 2e5964640b..3a5e3b7872 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -269,15 +269,15 @@ void __ioapic_write_entry(
 {
     union entry_union eu = { .entry = e };
 
-    if ( raw )
+    if ( raw || !iommu_intremap )
     {
         __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
         __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
     }
     else
     {
-        io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
-        io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
+        iommu_update_ire_from_apic(apic, 0x11 + 2 * pin, eu.w2);
+        iommu_update_ire_from_apic(apic, 0x10 + 2 * pin, eu.w1);
     }
 }
 
@@ -433,16 +433,17 @@ static void modify_IO_APIC_irq(unsigned int irq, unsigned int enable,
                                unsigned int disable)
 {
     struct irq_pin_list *entry = irq_2_pin + irq;
-    unsigned int pin, reg;
 
     for (;;) {
-        pin = entry->pin;
+        unsigned int pin = entry->pin;
+        struct IO_APIC_route_entry rte;
+
         if (pin == -1)
             break;
-        reg = io_apic_read(entry->apic, 0x10 + pin*2);
-        reg &= ~disable;
-        reg |= enable;
-        io_apic_modify(entry->apic, 0x10 + pin*2, reg);
+        rte = __ioapic_read_entry(entry->apic, pin, false);
+        rte.raw &= ~(uint64_t)disable;
+        rte.raw |= enable;
+        __ioapic_write_entry(entry->apic, pin, false, rte);
         if (!entry->next)
             break;
         entry = irq_2_pin + entry->next;
@@ -584,16 +585,16 @@ set_ioapic_affinity_irq(struct irq_desc *desc, const cpumask_t *mask)
             dest = SET_APIC_LOGICAL_ID(dest);
         entry = irq_2_pin + irq;
         for (;;) {
-            unsigned int data;
+            struct IO_APIC_route_entry rte;
+
             pin = entry->pin;
             if (pin == -1)
                 break;
 
-            io_apic_write(entry->apic, 0x10 + 1 + pin*2, dest);
-            data = io_apic_read(entry->apic, 0x10 + pin*2);
-            data &= ~IO_APIC_REDIR_VECTOR_MASK;
-            data |= MASK_INSR(desc->arch.vector, IO_APIC_REDIR_VECTOR_MASK);
-            io_apic_modify(entry->apic, 0x10 + pin*2, data);
+            rte = __ioapic_read_entry(entry->apic, pin, false);
+            rte.dest.dest32 = dest;
+            rte.vector = desc->arch.vector;
+            __ioapic_write_entry(entry->apic, pin, false, rte);
 
             if (!entry->next)
                 break;
@@ -2129,10 +2130,8 @@ void ioapic_resume(void)
             reg_00.bits.ID = mp_ioapics[apic].mpc_apicid;
             __io_apic_write(apic, 0, reg_00.raw);
         }
-        for (i = 0; i < nr_ioapic_entries[apic]; i++, entry++) {
-            __io_apic_write(apic, 0x11+2*i, *(((int *)entry)+1));
-            __io_apic_write(apic, 0x10+2*i, *(((int *)entry)+0));
-        }
+        for (i = 0; i < nr_ioapic_entries[apic]; i++, entry++)
+            __ioapic_write_entry(apic, i, true, *entry);
     }
     spin_unlock_irqrestore(&ioapic_lock, flags);
 }
-- 
2.35.1

Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry

[Jan Beulich]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> Do not allow to write to RTE registers using io_apic_write and instead
> require changes to RTE to be performed using ioapic_write_entry.

Hmm, this doubles the number of MMIO access in affected code fragments.

> --- a/xen/arch/x86/include/asm/io_apic.h
> +++ b/xen/arch/x86/include/asm/io_apic.h
> @@ -161,22 +161,11 @@ static inline void __io_apic_write(unsigned int apic, unsigned int reg, unsigned
>  
>  static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned int value)
>  {
> -    if ( ioapic_reg_remapped(reg) )
> -        return iommu_update_ire_from_apic(apic, reg, value);
> +    /* RTE writes must use ioapic_write_entry. */
> +    BUG_ON(reg >= 0x10);
>      __io_apic_write(apic, reg, value);
>  }
>  
> -/*
> - * Re-write a value: to be used for read-modify-write
> - * cycles where the read already set up the index register.
> - */
> -static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
> -{
> -    if ( ioapic_reg_remapped(reg) )
> -        return iommu_update_ire_from_apic(apic, reg, value);
> -    *(IO_APIC_BASE(apic) + 4) = value;
> -}

While the last caller goes away, I don't think this strictly needs to
be dropped (but could just gain a BUG_ON() like you do a few lines up)?

Jan

Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry

[Roger Pau Monné]

On Fri, Jun 03, 2022 at 03:34:33PM +0200, Jan Beulich wrote:
> On 21.04.2022 15:21, Roger Pau Monne wrote:
> > Do not allow to write to RTE registers using io_apic_write and instead
> > require changes to RTE to be performed using ioapic_write_entry.
> 
> Hmm, this doubles the number of MMIO access in affected code fragments.

But it does allow to simplify the IOMMU side quite a lot by no longer
having to update the IRTE using two different calls.  I'm quite sure
it saves quite some accesses to the IOMMU RTE in the following
patches.

> > --- a/xen/arch/x86/include/asm/io_apic.h
> > +++ b/xen/arch/x86/include/asm/io_apic.h
> > @@ -161,22 +161,11 @@ static inline void __io_apic_write(unsigned int apic, unsigned int reg, unsigned
> >  
> >  static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned int value)
> >  {
> > -    if ( ioapic_reg_remapped(reg) )
> > -        return iommu_update_ire_from_apic(apic, reg, value);
> > +    /* RTE writes must use ioapic_write_entry. */
> > +    BUG_ON(reg >= 0x10);
> >      __io_apic_write(apic, reg, value);
> >  }
> >  
> > -/*
> > - * Re-write a value: to be used for read-modify-write
> > - * cycles where the read already set up the index register.
> > - */
> > -static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
> > -{
> > -    if ( ioapic_reg_remapped(reg) )
> > -        return iommu_update_ire_from_apic(apic, reg, value);
> > -    *(IO_APIC_BASE(apic) + 4) = value;
> > -}
> 
> While the last caller goes away, I don't think this strictly needs to
> be dropped (but could just gain a BUG_ON() like you do a few lines up)?

Hm, could do, but it won't be suitable to be used to modify RTEs
anymore, and given that was it's only usage I didn't see much value
for leaving it around.

Thanks, Roger.

Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry

[Jan Beulich]

On 03.06.2022 17:01, Roger Pau Monné wrote:
> On Fri, Jun 03, 2022 at 03:34:33PM +0200, Jan Beulich wrote:
>> On 21.04.2022 15:21, Roger Pau Monne wrote:
>>> Do not allow to write to RTE registers using io_apic_write and instead
>>> require changes to RTE to be performed using ioapic_write_entry.
>>
>> Hmm, this doubles the number of MMIO access in affected code fragments.
> 
> But it does allow to simplify the IOMMU side quite a lot by no longer
> having to update the IRTE using two different calls.  I'm quite sure
> it saves quite some accesses to the IOMMU RTE in the following
> patches.

Right. You may want to mention upsides and downsides (and the ultimate
balance) in the description.

>>> --- a/xen/arch/x86/include/asm/io_apic.h
>>> +++ b/xen/arch/x86/include/asm/io_apic.h
>>> @@ -161,22 +161,11 @@ static inline void __io_apic_write(unsigned int apic, unsigned int reg, unsigned
>>>  
>>>  static inline void io_apic_write(unsigned int apic, unsigned int reg, unsigned int value)
>>>  {
>>> -    if ( ioapic_reg_remapped(reg) )
>>> -        return iommu_update_ire_from_apic(apic, reg, value);
>>> +    /* RTE writes must use ioapic_write_entry. */
>>> +    BUG_ON(reg >= 0x10);
>>>      __io_apic_write(apic, reg, value);
>>>  }
>>>  
>>> -/*
>>> - * Re-write a value: to be used for read-modify-write
>>> - * cycles where the read already set up the index register.
>>> - */
>>> -static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
>>> -{
>>> -    if ( ioapic_reg_remapped(reg) )
>>> -        return iommu_update_ire_from_apic(apic, reg, value);
>>> -    *(IO_APIC_BASE(apic) + 4) = value;
>>> -}
>>
>> While the last caller goes away, I don't think this strictly needs to
>> be dropped (but could just gain a BUG_ON() like you do a few lines up)?
> 
> Hm, could do, but it won't be suitable to be used to modify RTEs
> anymore, and given that was it's only usage I didn't see much value
> for leaving it around.

I could see room for use of it elsewhere, e.g. setup_ioapic_ids_from_mpc(),
io_apic_get_unique_id() (albeit read and write may be a little far apart in
both of them) or ioapic_resume(). Otoh one may argue its benefit is
marginal, so with some extra justification I could also see the function go
away at this occasion.

Jan

[PATCH RFC 4/6] x86/iommu: pass full IO-APIC RTE for remapping table update

[Roger Pau Monne]

So that the remapping entry can be updated atomically when possible.

Doing such update atomically will avoid Xen having to mask the IO-APIC
pin prior to performing any interrupt movements (ie: changing the
destination and vector fields), as the interrupt remapping entry is
always consistent.

This also simplifies some of the logic on both VT-d and AMD-Vi
implementations, as having the full RTE available instead of half of
it avoids to possibly read and update the missing other half from
hardware.

While there remove the explicit zeroing of new_ire fields in
ioapic_rte_to_remap_entry and initialize the variable at definition so
all fields are zeroed.  Note fields could be also initialized with
final values at definition, but I found that likely too much to be
done at this time.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
There's some functional differences between VT-d and AMD-Vi that we
might also want to address: the logic on failure from setting up the
remap entry on VT-d unmasks the previous RTE, while on AMD-Vi the pin
is left masked.

Note that certain combination of changes to the RTE are impossible to
handle atomically. For example changing the vector and/or destination
fields together with the triggering mode is impossible to be performed
atomically (as the destination and vector is set in the IRTE, but the
triggering mode is set in the RTE).  Xen doesn't attempt to perform
such changes in a single update to the RTE anyway, so it's fine.

Since the IOMMU code now also uses __ioapic_write_entry to update the
RTE it might be helpful to introduce an explicit raw version of the
function, as IOMMU always wants to write the provided raw value to the
RTE.
---
 xen/arch/x86/include/asm/iommu.h         |   3 +-
 xen/arch/x86/io_apic.c                   |   5 +-
 xen/drivers/passthrough/amd/iommu.h      |   2 +-
 xen/drivers/passthrough/amd/iommu_intr.c |  84 ++-------------
 xen/drivers/passthrough/vtd/extern.h     |   2 +-
 xen/drivers/passthrough/vtd/intremap.c   | 125 +++++++++++------------
 xen/drivers/passthrough/x86/iommu.c      |   4 +-
 xen/include/xen/iommu.h                  |   3 +-
 8 files changed, 80 insertions(+), 148 deletions(-)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index 9ccf4f8bdd..1d62b42e17 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -97,7 +97,8 @@ struct iommu_init_ops {
 
 extern const struct iommu_init_ops *iommu_init_ops;
 
-void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
+void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg,
+                                uint64_t rte);
 unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
 int iommu_setup_hpet_msi(struct msi_desc *);
 
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 3a5e3b7872..f61e56f3d1 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -275,10 +275,7 @@ void __ioapic_write_entry(
         __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
     }
     else
-    {
-        iommu_update_ire_from_apic(apic, 0x11 + 2 * pin, eu.w2);
-        iommu_update_ire_from_apic(apic, 0x10 + 2 * pin, eu.w1);
-    }
+        iommu_update_ire_from_apic(apic, pin, e.raw);
 }
 
 static void ioapic_write_entry(
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 64e4bbf33c..35df504406 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -298,7 +298,7 @@ int cf_check amd_iommu_free_intremap_table(
 unsigned int amd_iommu_intremap_table_order(
     const void *irt, const struct amd_iommu *iommu);
 void cf_check amd_iommu_ioapic_update_ire(
-    unsigned int apic, unsigned int reg, unsigned int value);
+    unsigned int apic, unsigned int pin, uint64_t raw);
 unsigned int cf_check amd_iommu_read_ioapic_from_ire(
     unsigned int apic, unsigned int reg);
 int cf_check amd_iommu_msi_msg_update_ire(
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index cebf9ceca7..feed1d1447 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -247,11 +247,6 @@ static void update_intremap_entry(const struct amd_iommu *iommu,
     }
 }
 
-static inline int get_rte_index(const struct IO_APIC_route_entry *rte)
-{
-    return rte->vector | (rte->delivery_mode << 8);
-}
-
 static inline void set_rte_index(struct IO_APIC_route_entry *rte, int offset)
 {
     rte->vector = (u8)offset;
@@ -267,7 +262,6 @@ static int update_intremap_entry_from_ioapic(
     int bdf,
     struct amd_iommu *iommu,
     struct IO_APIC_route_entry *rte,
-    bool_t lo_update,
     u16 *index)
 {
     unsigned long flags;
@@ -315,31 +309,6 @@ static int update_intremap_entry_from_ioapic(
         spin_lock(lock);
     }
 
-    if ( fresh )
-        /* nothing */;
-    else if ( !lo_update )
-    {
-        /*
-         * Low half of incoming RTE is already in remapped format,
-         * so need to recover vector and delivery mode from IRTE.
-         */
-        ASSERT(get_rte_index(rte) == offset);
-        if ( iommu->ctrl.ga_en )
-            vector = entry.ptr128->full.vector;
-        else
-            vector = entry.ptr32->flds.vector;
-        /* The IntType fields match for both formats. */
-        delivery_mode = entry.ptr32->flds.int_type;
-    }
-    else if ( x2apic_enabled )
-    {
-        /*
-         * High half of incoming RTE was read from the I/O APIC and hence may
-         * not hold the full destination, so need to recover full destination
-         * from IRTE.
-         */
-        dest = get_full_dest(entry.ptr128);
-    }
     update_intremap_entry(iommu, entry, vector, delivery_mode, dest_mode, dest);
 
     spin_unlock_irqrestore(lock, flags);
@@ -350,23 +319,15 @@ static int update_intremap_entry_from_ioapic(
 }
 
 void cf_check amd_iommu_ioapic_update_ire(
-    unsigned int apic, unsigned int reg, unsigned int value)
+    unsigned int apic, unsigned int pin, uint64_t raw)
 {
-    struct IO_APIC_route_entry old_rte = { 0 };
-    struct IO_APIC_route_entry new_rte = { 0 };
-    unsigned int rte_lo = (reg & 1) ? reg - 1 : reg;
-    unsigned int pin = (reg - 0x10) / 2;
+    struct IO_APIC_route_entry new_rte = { .raw = raw };
+    struct IO_APIC_route_entry old_rte = { };
     int seg, bdf, rc;
     bool saved_mask, fresh = false;
     struct amd_iommu *iommu;
     unsigned int idx;
 
-    if ( !iommu_intremap )
-    {
-        __io_apic_write(apic, reg, value);
-        return;
-    }
-
     idx = ioapic_id_to_index(IO_APIC_ID(apic));
     if ( idx == MAX_IO_APICS )
         return;
@@ -379,26 +340,14 @@ void cf_check amd_iommu_ioapic_update_ire(
     {
         AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
                        seg, bdf);
-        __io_apic_write(apic, reg, value);
+        __ioapic_write_entry(apic, pin, true, new_rte);
         return;
     }
 
     /* save io-apic rte lower 32 bits */
-    *((u32 *)&old_rte) =  __io_apic_read(apic, rte_lo);
+    old_rte = __ioapic_read_entry(apic, pin, true);
     saved_mask = old_rte.mask;
 
-    if ( reg == rte_lo )
-    {
-        *((u32 *)&new_rte) = value;
-        /* read upper 32 bits from io-apic rte */
-        *(((u32 *)&new_rte) + 1) = __io_apic_read(apic, reg + 1);
-    }
-    else
-    {
-        *((u32 *)&new_rte) = *((u32 *)&old_rte);
-        *(((u32 *)&new_rte) + 1) = value;
-    }
-
     if ( ioapic_sbdf[idx].pin_2_idx[pin] >= INTREMAP_MAX_ENTRIES )
     {
         ASSERT(saved_mask);
@@ -410,7 +359,7 @@ void cf_check amd_iommu_ioapic_update_ire(
          */
         if ( new_rte.mask && !x2apic_enabled )
         {
-            __io_apic_write(apic, reg, value);
+            __ioapic_write_entry(apic, pin, true, new_rte);
             return;
         }
 
@@ -421,16 +370,14 @@ void cf_check amd_iommu_ioapic_update_ire(
     if ( !saved_mask )
     {
         old_rte.mask = 1;
-        __io_apic_write(apic, rte_lo, *((u32 *)&old_rte));
+        __ioapic_write_entry(apic, pin, true, old_rte);
     }
 
     /* Update interrupt remapping entry */
     rc = update_intremap_entry_from_ioapic(
-             bdf, iommu, &new_rte, reg == rte_lo,
+             bdf, iommu, &new_rte,
              &ioapic_sbdf[idx].pin_2_idx[pin]);
 
-    __io_apic_write(apic, reg, ((u32 *)&new_rte)[reg != rte_lo]);
-
     if ( rc )
     {
         /* Keep the entry masked. */
@@ -439,20 +386,7 @@ void cf_check amd_iommu_ioapic_update_ire(
         return;
     }
 
-    /* For lower bits access, return directly to avoid double writes */
-    if ( reg == rte_lo )
-        return;
-
-    /*
-     * Unmask the interrupt after we have updated the intremap table. Also
-     * write the low half if a fresh entry was allocated for a high half
-     * update in x2APIC mode.
-     */
-    if ( !saved_mask || (x2apic_enabled && fresh) )
-    {
-        old_rte.mask = saved_mask;
-        __io_apic_write(apic, rte_lo, *((u32 *)&old_rte));
-    }
+    __ioapic_write_entry(apic, pin, true, new_rte);
 }
 
 unsigned int cf_check amd_iommu_read_ioapic_from_ire(
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index 39602d1f88..032a7c3b42 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -92,7 +92,7 @@ int cf_check intel_iommu_get_reserved_device_memory(
 unsigned int cf_check io_apic_read_remap_rte(
     unsigned int apic, unsigned int reg);
 void cf_check io_apic_write_remap_rte(
-    unsigned int apic, unsigned int reg, unsigned int value);
+    unsigned int apic, unsigned int ioapic_pin, uint64_t raw);
 
 struct msi_desc;
 struct msi_msg;
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index e6ba89591b..52058efe86 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -328,12 +328,11 @@ static int remap_entry_to_ioapic_rte(
 
 static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
     int apic, unsigned int ioapic_pin, struct IO_xAPIC_route_entry *old_rte,
-    unsigned int rte_upper, unsigned int value)
+    struct IO_xAPIC_route_entry new_rte)
 {
     struct iremap_entry *iremap_entry = NULL, *iremap_entries;
-    struct iremap_entry new_ire;
+    struct iremap_entry new_ire = { };
     struct IO_APIC_route_remap_entry *remap_rte;
-    struct IO_xAPIC_route_entry new_rte;
     int index;
     unsigned long flags;
     bool init = false;
@@ -364,48 +363,37 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu *iommu,
 
     new_ire = *iremap_entry;
 
-    if ( rte_upper )
-    {
-        if ( x2apic_enabled )
-            new_ire.remap.dst = value;
-        else
-            new_ire.remap.dst = (value >> 24) << 8;
-    }
+    if ( x2apic_enabled )
+        new_ire.remap.dst = new_rte.dest.dest32;
     else
-    {
-        *(((u32 *)&new_rte) + 0) = value;
-        new_ire.remap.fpd = 0;
-        new_ire.remap.dm = new_rte.dest_mode;
-        new_ire.remap.tm = new_rte.trigger;
-        new_ire.remap.dlm = new_rte.delivery_mode;
-        /* Hardware require RH = 1 for LPR delivery mode */
-        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
-        new_ire.remap.avail = 0;
-        new_ire.remap.res_1 = 0;
-        new_ire.remap.vector = new_rte.vector;
-        new_ire.remap.res_2 = 0;
-
-        set_ioapic_source_id(IO_APIC_ID(apic), &new_ire);
-        new_ire.remap.res_3 = 0;
-        new_ire.remap.res_4 = 0;
-        new_ire.remap.p = 1;     /* finally, set present bit */
-
-        /* now construct new ioapic rte entry */
-        remap_rte->vector = new_rte.vector;
-        remap_rte->delivery_mode = 0;    /* has to be 0 for remap format */
-        remap_rte->index_15 = (index >> 15) & 0x1;
-        remap_rte->index_0_14 = index & 0x7fff;
-
-        remap_rte->delivery_status = new_rte.delivery_status;
-        remap_rte->polarity = new_rte.polarity;
-        remap_rte->irr = new_rte.irr;
-        remap_rte->trigger = new_rte.trigger;
-        remap_rte->mask = new_rte.mask;
-        remap_rte->reserved = 0;
-        remap_rte->format = 1;    /* indicate remap format */
-    }
-
-    update_irte(iommu, iremap_entry, &new_ire, !init);
+        new_ire.remap.dst = (new_rte.dest.dest32 >> 24) << 8;
+
+    new_ire.remap.dm = new_rte.dest_mode;
+    new_ire.remap.tm = new_rte.trigger;
+    new_ire.remap.dlm = new_rte.delivery_mode;
+    /* Hardware require RH = 1 for LPR delivery mode */
+    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
+    new_ire.remap.vector = new_rte.vector;
+
+    set_ioapic_source_id(IO_APIC_ID(apic), &new_ire);
+    new_ire.remap.p = 1;     /* finally, set present bit */
+
+    /* now construct new ioapic rte entry */
+    remap_rte->vector = new_rte.vector;
+    remap_rte->delivery_mode = 0;    /* has to be 0 for remap format */
+    remap_rte->index_15 = (index >> 15) & 0x1;
+    remap_rte->index_0_14 = index & 0x7fff;
+
+    remap_rte->delivery_status = new_rte.delivery_status;
+    remap_rte->polarity = new_rte.polarity;
+    remap_rte->irr = new_rte.irr;
+    remap_rte->trigger = new_rte.trigger;
+    remap_rte->mask = new_rte.mask;
+    remap_rte->reserved = 0;
+    remap_rte->format = 1;    /* indicate remap format */
+
+    /* If cmpxchg16b is not available the caller must mask the IO-APIC pin. */
+    update_irte(iommu, iremap_entry, &new_ire, !init && cpu_has_cx16);
     iommu_sync_cache(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);
 
@@ -439,33 +427,44 @@ unsigned int cf_check io_apic_read_remap_rte(
 }
 
 void cf_check io_apic_write_remap_rte(
-    unsigned int apic, unsigned int reg, unsigned int value)
+    unsigned int apic, unsigned int ioapic_pin, uint64_t raw)
 {
-    unsigned int ioapic_pin = (reg - 0x10) / 2;
-    struct IO_xAPIC_route_entry old_rte = { 0 };
+    struct IO_APIC_route_entry rte = { .raw = raw };
+    struct IO_xAPIC_route_entry old_rte = { };
     struct IO_APIC_route_remap_entry *remap_rte;
-    unsigned int rte_upper = (reg & 1) ? 1 : 0;
     struct vtd_iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic));
-    int saved_mask;
-
-    old_rte = __ioapic_read_entry(apic, ioapic_pin, true);
+    bool masked = true;
+    int rc;
 
     remap_rte = (struct IO_APIC_route_remap_entry *) &old_rte;
 
-    /* mask the interrupt while we change the intremap table */
-    saved_mask = remap_rte->mask;
-    remap_rte->mask = 1;
-    __io_apic_write(apic, reg & ~1, *(u32 *)&old_rte);
-    remap_rte->mask = saved_mask;
-
-    if ( ioapic_rte_to_remap_entry(iommu, apic, ioapic_pin,
-                                   &old_rte, rte_upper, value) )
+    if ( !cpu_has_cx16 )
     {
-        __io_apic_write(apic, reg, value);
+       /*
+        * Cannot atomically update the IRTE entry: mask the IO-APIC pin to
+        * avoid interrupts seeing an inconsistent IRTE entry.
+        */
+        old_rte = __ioapic_read_entry(apic, ioapic_pin, true);
+        if ( !old_rte.mask )
+        {
+            masked = false;
+            old_rte.mask = 1;
+            __ioapic_write_entry(apic, ioapic_pin, true, old_rte);
+        }
+    }
 
-        /* Recover the original value of 'mask' bit */
-        if ( rte_upper )
-            __io_apic_write(apic, reg & ~1, *(u32 *)&old_rte);
+    rc = ioapic_rte_to_remap_entry(iommu, apic, ioapic_pin, &old_rte, rte);
+    if ( rc )
+    {
+        dprintk(XENLOG_ERR,
+                "failed to update IRTE for IO-APIC#%u pin %u: %d\n",
+                apic, ioapic_pin, rc);
+        if ( !masked )
+        {
+            /* Recover the original value of 'mask' bit */
+            old_rte.mask = 0;
+            __ioapic_write_entry(apic, ioapic_pin, true, old_rte);
+        }
     }
     else
         __ioapic_write_entry(apic, ioapic_pin, true, old_rte);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index d5bf4d3241..e0ad38a09d 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -133,9 +133,9 @@ int iommu_enable_x2apic(void)
 }
 
 void iommu_update_ire_from_apic(
-    unsigned int apic, unsigned int reg, unsigned int value)
+    unsigned int apic, unsigned int idx, uint64_t rte)
 {
-    iommu_vcall(&iommu_ops, update_ire_from_apic, apic, reg, value);
+    iommu_vcall(&iommu_ops, update_ire_from_apic, apic, idx, rte);
 }
 
 unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg)
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3a83981464..b619cd872b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -262,7 +262,8 @@ struct iommu_ops {
     int (*enable_x2apic)(void);
     void (*disable_x2apic)(void);
 
-    void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
+    void (*update_ire_from_apic)(unsigned int apic, unsigned int idx,
+                                 uint64_t rte);
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
 
     int (*setup_hpet_msi)(struct msi_desc *);
-- 
2.35.1

[PATCH RFC 5/6] amd/iommu: atomically update remapping entries when possible

[Roger Pau Monne]

Doing so matches existing VT-d behavior, and does prevent having to
disable the remapping entry or mask the IO-APIC pin prior to being
updated, as the remap entry content is always consistent.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/amd/iommu_intr.c | 31 +++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index feed1d1447..b24e703c75 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -39,6 +39,7 @@ union irte32 {
 };
 
 union irte128 {
+    __uint128_t raw128;
     uint64_t raw[2];
     struct {
         bool remap_en:1;
@@ -222,6 +223,21 @@ static void update_intremap_entry(const struct amd_iommu *iommu,
             },
         };
 
+        if ( cpu_has_cx16 )
+        {
+            union irte128 old_irte = *entry.ptr128;
+            __uint128_t ret = cmpxchg16b(entry.ptr128, &old_irte, &irte);
+
+            /*
+             * In the above, we use cmpxchg16 to atomically update the 128-bit
+             * IRTE, and the hardware cannot update the IRTE behind us, so
+             * the return value of cmpxchg16 should be the same as old_ire.
+             * This ASSERT validate it.
+             */
+            ASSERT(ret == old_irte.raw128);
+            return;
+        }
+
         ASSERT(!entry.ptr128->full.remap_en);
         entry.ptr128->raw[1] = irte.raw[1];
         /*
@@ -299,7 +315,8 @@ static int update_intremap_entry_from_ioapic(
     entry = get_intremap_entry(iommu, req_id, offset);
 
     /* The RemapEn fields match for all formats. */
-    while ( iommu->enabled && entry.ptr32->flds.remap_en )
+    while ( iommu->enabled && entry.ptr32->flds.remap_en &&
+            !cpu_has_cx16 && iommu->ctrl.ga_en )
     {
         entry.ptr32->flds.remap_en = false;
         spin_unlock(lock);
@@ -366,8 +383,11 @@ void cf_check amd_iommu_ioapic_update_ire(
         fresh = true;
     }
 
-    /* mask the interrupt while we change the intremap table */
-    if ( !saved_mask )
+    /*
+     * Mask the interrupt while we change the intremap table if it can't be
+     * done atomically.
+     */
+    if ( !saved_mask && !cpu_has_cx16 && iommu->ctrl.ga_en )
     {
         old_rte.mask = 1;
         __ioapic_write_entry(apic, pin, true, old_rte);
@@ -383,6 +403,11 @@ void cf_check amd_iommu_ioapic_update_ire(
         /* Keep the entry masked. */
         printk(XENLOG_ERR "Remapping IO-APIC %#x pin %u failed (%d)\n",
                IO_APIC_ID(apic), pin, rc);
+        if ( !saved_mask && (cpu_has_cx16 || !iommu->ctrl.ga_en) )
+        {
+            old_rte.mask = 1;
+            __ioapic_write_entry(apic, pin, true, old_rte);
+        }
         return;
     }
 
-- 
2.35.1

Re: [PATCH RFC 5/6] amd/iommu: atomically update remapping entries when possible

[Jan Beulich]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> @@ -366,8 +383,11 @@ void cf_check amd_iommu_ioapic_update_ire(
>          fresh = true;
>      }
>  
> -    /* mask the interrupt while we change the intremap table */
> -    if ( !saved_mask )
> +    /*
> +     * Mask the interrupt while we change the intremap table if it can't be
> +     * done atomically.
> +     */
> +    if ( !saved_mask && !cpu_has_cx16 && iommu->ctrl.ga_en )
>      {
>          old_rte.mask = 1;
>          __ioapic_write_entry(apic, pin, true, old_rte);
> @@ -383,6 +403,11 @@ void cf_check amd_iommu_ioapic_update_ire(
>          /* Keep the entry masked. */
>          printk(XENLOG_ERR "Remapping IO-APIC %#x pin %u failed (%d)\n",
>                 IO_APIC_ID(apic), pin, rc);
> +        if ( !saved_mask && (cpu_has_cx16 || !iommu->ctrl.ga_en) )
> +        {
> +            old_rte.mask = 1;
> +            __ioapic_write_entry(apic, pin, true, old_rte);
> +        }
>          return;
>      }

Iirc you have a question about this (wrt differing VT-d behavior)
elsewhere. I'm not convinced of this masking. I could see it as a
measure to prevent damage if an update was done partially, but I
don't see such being possible here. Hence to me it would look more
logical if the entry was simply left as is, leaving it to the
caller to correctly deal with the failure. But of course that
would first require telling the caller about the failure ...

Jan

[PATCH RFC 6/6] x86/ioapic: mask entry while updating

[Roger Pau Monne]

When writing an IO-APIC RTE entry make sure incoming interrupts never
see a partially updated entry, by masking the pin while doing the
update when necessary.  Add some logic to attempt to limit the number
of writes.

With the masking now handled by __ioapic_write_entry itself when
necessary, we can drop the setting of the disable hook for IO-APIC
edge triggered interrupts.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Note this includes a revert of the first patch in the series.
---
 xen/arch/x86/io_apic.c | 45 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index f61e56f3d1..1860af7353 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -267,12 +267,47 @@ void __ioapic_write_entry(
     unsigned int apic, unsigned int pin, bool raw,
     struct IO_APIC_route_entry e)
 {
-    union entry_union eu = { .entry = e };
-
     if ( raw || !iommu_intremap )
     {
-        __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
-        __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
+        union entry_union eu = { .entry = e };
+        union entry_union curr = {
+            .entry = __ioapic_read_entry(apic, pin, true),
+        };
+        bool masked = true;
+
+        if ( curr.entry.mask )
+        {
+            /*
+             * If pin is currently masked we can update the high part first
+             * without worrying about the RTE being in an inconsistent state.
+             */
+            if ( curr.w2 != eu.w2 )
+                __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
+            if ( curr.w1 != eu.w1 )
+                __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
+            return;
+        }
+
+        if ( curr.w1 != eu.w1 && curr.w2 != eu.w2 && !eu.entry.mask )
+        {
+            /*
+             * If updating both halves mask the entry while updating so
+             * interrupts are not injected with an inconsistent RTE.
+             */
+            eu.entry.mask = 1;
+            masked = false;
+        }
+
+        if ( curr.w1 != eu.w1 )
+            __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
+        if ( curr.w2 != eu.w2 )
+            __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
+
+        if ( !masked )
+        {
+            eu.entry.mask = 0;
+            __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
+        }
     }
     else
         iommu_update_ire_from_apic(apic, pin, e.raw);
@@ -1780,7 +1815,7 @@ static hw_irq_controller ioapic_edge_type = {
     .startup 	= startup_edge_ioapic_irq,
     .shutdown 	= irq_shutdown_none,
     .enable 	= unmask_IO_APIC_irq,
-    .disable 	= mask_IO_APIC_irq,
+    .disable 	= irq_disable_none,
     .ack 		= ack_edge_ioapic_irq,
     .set_affinity 	= set_ioapic_affinity_irq,
 };
-- 
2.35.1

Re: [PATCH RFC 6/6] x86/ioapic: mask entry while updating

[Jan Beulich]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -267,12 +267,47 @@ void __ioapic_write_entry(
>      unsigned int apic, unsigned int pin, bool raw,
>      struct IO_APIC_route_entry e)
>  {
> -    union entry_union eu = { .entry = e };
> -
>      if ( raw || !iommu_intremap )
>      {
> -        __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
> -        __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +        union entry_union eu = { .entry = e };
> +        union entry_union curr = {
> +            .entry = __ioapic_read_entry(apic, pin, true),
> +        };
> +        bool masked = true;
> +
> +        if ( curr.entry.mask )
> +        {
> +            /*
> +             * If pin is currently masked we can update the high part first
> +             * without worrying about the RTE being in an inconsistent state.
> +             */
> +            if ( curr.w2 != eu.w2 )
> +                __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
> +            if ( curr.w1 != eu.w1 )
> +                __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +            return;
> +        }
> +
> +        if ( curr.w1 != eu.w1 && curr.w2 != eu.w2 && !eu.entry.mask )
> +        {
> +            /*
> +             * If updating both halves mask the entry while updating so
> +             * interrupts are not injected with an inconsistent RTE.
> +             */
> +            eu.entry.mask = 1;
> +            masked = false;
> +        }
> +
> +        if ( curr.w1 != eu.w1 )
> +            __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +        if ( curr.w2 != eu.w2 )
> +            __io_apic_write(apic, 0x11 + 2 * pin, eu.w2);
> +
> +        if ( !masked )
> +        {
> +            eu.entry.mask = 0;
> +            __io_apic_write(apic, 0x10 + 2 * pin, eu.w1);
> +        }

For the write avoidance don't you want to hide differences in the
r/o delivery_status field, e.g. by setting curr's to eu's after
having read curr?

Jan

Re: [PATCH RFC 0/6] x86/ioapic: fix edge triggered interrupt migration

[Roger Pau Monné]

On Thu, Apr 21, 2022 at 03:21:08PM +0200, Roger Pau Monne wrote:
> Hello,
> 
> Following series attempts to solve the issue with IO-APIC edge triggered
> interrupts seeing an inconsistent RTE or IRTE when injected while being
> migrated.
> 
> It's currently RFC because some patches have post commit message notes,
> and because I'm not sure if patch 1 is really needed.  I originally had
> the idea of suggesting to only backport patch 1 in order to fix the
> issue in older releases, while leaving the more complex (and thus
> error prone) IOMMU changes in unstable.  Note however that patch 1 is
> just a workaround to prevent interrupts seeing inconsistent entries
> while being updated, masking the entry just makes the warning go away,
> but the interrupt will be lost.

Ping?

Thanks, Roger.

Re: [PATCH RFC 0/6] x86/ioapic: fix edge triggered interrupt migration

[Jan Beulich]

On 20.05.2022 14:36, Roger Pau Monné wrote:
> On Thu, Apr 21, 2022 at 03:21:08PM +0200, Roger Pau Monne wrote:
>> Hello,
>>
>> Following series attempts to solve the issue with IO-APIC edge triggered
>> interrupts seeing an inconsistent RTE or IRTE when injected while being
>> migrated.
>>
>> It's currently RFC because some patches have post commit message notes,
>> and because I'm not sure if patch 1 is really needed.  I originally had
>> the idea of suggesting to only backport patch 1 in order to fix the
>> issue in older releases, while leaving the more complex (and thus
>> error prone) IOMMU changes in unstable.  Note however that patch 1 is
>> just a workaround to prevent interrupts seeing inconsistent entries
>> while being updated, masking the entry just makes the warning go away,
>> but the interrupt will be lost.
> 
> Ping?

Sorry, the usual thing with RFCs: They take lower priority than other
work items. This series is certainly the first of the several pending
RFC series which I mean to get to, but it's hard to predict when this
would be.

Jan

Re: [PATCH RFC 0/6] x86/ioapic: fix edge triggered interrupt migration

[Roger Pau Monné]

On Fri, May 20, 2022 at 02:46:39PM +0200, Jan Beulich wrote:
> On 20.05.2022 14:36, Roger Pau Monné wrote:
> > On Thu, Apr 21, 2022 at 03:21:08PM +0200, Roger Pau Monne wrote:
> >> Hello,
> >>
> >> Following series attempts to solve the issue with IO-APIC edge triggered
> >> interrupts seeing an inconsistent RTE or IRTE when injected while being
> >> migrated.
> >>
> >> It's currently RFC because some patches have post commit message notes,
> >> and because I'm not sure if patch 1 is really needed.  I originally had
> >> the idea of suggesting to only backport patch 1 in order to fix the
> >> issue in older releases, while leaving the more complex (and thus
> >> error prone) IOMMU changes in unstable.  Note however that patch 1 is
> >> just a workaround to prevent interrupts seeing inconsistent entries
> >> while being updated, masking the entry just makes the warning go away,
> >> but the interrupt will be lost.
> > 
> > Ping?
> 
> Sorry, the usual thing with RFCs: They take lower priority than other
> work items. This series is certainly the first of the several pending
> RFC series which I mean to get to, but it's hard to predict when this
> would be.

No problem, I don't think it's a super urgent issue: we have always
handled interrupts this way and so far got no noticeable issues (apart
from the log messages reported on the console).

There's a non-trivial amount of IOMMU code changes, so maybe the IOMMU
maintainers could take a stab at those as a start?

Thanks, Roger.