Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2022 17:01, Roger Pau Monné wrote:
> On Fri, Jun 03, 2022 at 03:34:33PM +0200, Jan Beulich wrote:
>> On 21.04.2022 15:21, Roger Pau Monne wrote:
>>> Do not allow to write to RTE registers using io_apic_write and instead
>>> require changes to RTE to be performed using ioapic_write_entry.
>>
>> Hmm, this doubles the number of MMIO access in affected code fragments.
> 
> But it does allow to simplify the IOMMU side quite a lot by no longer
> having to update the IRTE using two different calls.  I'm quite sure
> it saves quite some accesses to the IOMMU RTE in the following
> patches.

Right. You may want to mention upsides and downsides (and the ultimate
balance) in the description.

>>> --- a/xen/arch/x86/include/asm/io_apic.h
>>> +++ b/xen/arch/x86/include/asm/io_apic.h
>>> @@ -161,22 +161,11 @@ static inline void __io_apic_write(unsigned int apic, 
>>> unsigned int reg, unsigned
>>>  
>>>  static inline void io_apic_write(unsigned int apic, unsigned int reg, 
>>> unsigned int value)
>>>  {
>>> -    if ( ioapic_reg_remapped(reg) )
>>> -        return iommu_update_ire_from_apic(apic, reg, value);
>>> +    /* RTE writes must use ioapic_write_entry. */
>>> +    BUG_ON(reg >= 0x10);
>>>      __io_apic_write(apic, reg, value);
>>>  }
>>>  
>>> -/*
>>> - * Re-write a value: to be used for read-modify-write
>>> - * cycles where the read already set up the index register.
>>> - */
>>> -static inline void io_apic_modify(unsigned int apic, unsigned int reg, 
>>> unsigned int value)
>>> -{
>>> -    if ( ioapic_reg_remapped(reg) )
>>> -        return iommu_update_ire_from_apic(apic, reg, value);
>>> -    *(IO_APIC_BASE(apic) + 4) = value;
>>> -}
>>
>> While the last caller goes away, I don't think this strictly needs to
>> be dropped (but could just gain a BUG_ON() like you do a few lines up)?
> 
> Hm, could do, but it won't be suitable to be used to modify RTEs
> anymore, and given that was it's only usage I didn't see much value
> for leaving it around.

I could see room for use of it elsewhere, e.g. setup_ioapic_ids_from_mpc(),
io_apic_get_unique_id() (albeit read and write may be a little far apart in
both of them) or ioapic_resume(). Otoh one may argue its benefit is
marginal, so with some extra justification I could also see the function go
away at this occasion.

Jan