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
