On 19.07.2023 17:20, 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
>>> @@ -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().
I would indeed prefer if you did, to avoid the functions going out of
sync.
>>> @@ -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).
I was actually thinking of a 2-bit hint that the caller would pass:
Low half unmodified and high half unmodified.
> This would only be relevant for systems without IOMMU IRTEs, as
> otherwise the IO-APIC RTE gets modified by the IOMMU handlers.
Initially yes. I think there's room there to avoid one half of the
write as well.
Jan
