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
