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

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

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.

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