Re: [PATCH v2 2/5] x86/ioapic: RTE modifications must use ioapic_write_entry

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 18, 2023 at 05:40:18PM +0200, Jan Beulich wrote:
> On 18.07.2023 14:43, Roger Pau Monne wrote:
> > --- 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);
> >      }
> >  }
> 
> I think __ioapic_read_entry() wants updating similarly, so that both
> remain consistent.

My plan was to deal with __ioapic_read_entry() separately, as I would
also like to convert iommu_read_apic_from_ire() to get passed a pin
instead of a register, but I could make your requested adjustment here
for consistency with __ioapic_write_entry().

> > @@ -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);
> 
> While this isn't going to happen overly often, ...
> 
> > @@ -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);
> 
> ... this makes me wonder whether there shouldn't be an
> __ioapic_modify_entry() capable of suppressing one of the two
> writes (but still being handed the full RTE).

We would then need the original RTE to be passed to
__ioapic_modify_entry() in order for it to decide whether one of the
accesses can be eliminated (as I don't think we want to read the RTE
to check for differences, as we would then perform even more
accesses).

This would only be relevant for systems without IOMMU IRTEs, as
otherwise the IO-APIC RTE gets modified by the IOMMU handlers.

Thanks, Roger.