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
> > @@ -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).

I've wondered about this, and I'm not sure how often can one of the
two writes be suppressed here, as we modify both the low (for the
vector) and the high part of the RTE (for the destination).  It's
unlikely that the same vector could be used on both destinations, and
IMO such case doesn't warrant the introduction of the extra logic
required in order to suppress one of the writes.

Am I overlooking something?

Thanks, Roger.