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

[Jan Beulich <jbeulich@xxxxxxxx>]

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