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

[Jan Beulich <jbeulich@xxxxxxxx>]

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