| Message ID | 20220421132114.35118-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ioapic: fix edge triggered interrupt migration | expand |
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. > --- 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)? Jan
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. > > --- 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. Thanks, Roger.
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
diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h index a558bb063c..c26261aecb 100644 --- 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; -} - /* 1 if "noapic" boot option passed */ extern bool skip_ioapic_setup; extern bool ioapic_ack_new; diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 2e5964640b..3a5e3b7872 100644 --- 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); } } @@ -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); if (!entry->next) break; entry = irq_2_pin + entry->next; @@ -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); if (!entry->next) break; @@ -2129,10 +2130,8 @@ void ioapic_resume(void) reg_00.bits.ID = mp_ioapics[apic].mpc_apicid; __io_apic_write(apic, 0, reg_00.raw); } - for (i = 0; i < nr_ioapic_entries[apic]; i++, entry++) { - __io_apic_write(apic, 0x11+2*i, *(((int *)entry)+1)); - __io_apic_write(apic, 0x10+2*i, *(((int *)entry)+0)); - } + for (i = 0; i < nr_ioapic_entries[apic]; i++, entry++) + __ioapic_write_entry(apic, i, true, *entry); } spin_unlock_irqrestore(&ioapic_lock, flags); }
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. This is in preparation for passing the full contents of the RTE to the IOMMU interrupt remapping handlers, so remapping entries for IO-APIC RTEs can be updated atomically when possible. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/io_apic.h | 15 ++---------- xen/arch/x86/io_apic.c | 37 +++++++++++++++--------------- 2 files changed, 20 insertions(+), 32 deletions(-)