Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry
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
Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry
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.
Re: [PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry
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
[PATCH RFC 3/6] x86/ioapic: RTE modifications must use ioapic_write_entry
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é
---
xen/arch/x86/include/asm/io_apic.h | 15 ++--
xen/arch/x86/io_apic.c | 37 +++---
2 files changed, 20 insertions(+), 32 deletions(-)
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);
}
--
2.35.1
