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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2023 15:30, Roger Pau Monné wrote:
> 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?

Oh, no, that was me seeing the io_apic_modify() without paying attention
to the earlier io_apic_write() (both in the code you replace).

Jan