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