Re: [PATCH v2 4/5] x86/iommu: pass full IO-APIC RTE for remapping table update

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2023 14:43, Roger Pau Monne wrote:
> So that the remapping entry can be updated atomically when possible.
> 
> Doing such update atomically will avoid Xen having to mask the IO-APIC
> pin prior to performing any interrupt movements (ie: changing the
> destination and vector fields), as the interrupt remapping entry is
> always consistent.
> 
> This also simplifies some of the logic on both VT-d and AMD-Vi
> implementations, as having the full RTE available instead of half of
> it avoids to possibly read and update the missing other half from
> hardware.
> 
> While there remove the explicit zeroing of new_ire fields in
> ioapic_rte_to_remap_entry() and initialize the variable at definition
> so all fields are zeroed.  Note fields could be also initialized with
> final values at definition, but I found that likely too much to be
> done at this time.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Note that certain combination of changes to the RTE are impossible to
> handle atomically. For example changing the vector and/or destination
> fields together with the triggering mode is impossible to be performed
> atomically (as the destination and vector is set in the IRTE, but the
> triggering mode is set in the RTE).  Xen doesn't attempt to perform
> such changes in a single update to the RTE anyway, so it's fine.
> 
> Naming the iommu_update_ire_from_apic() parameter RTE is not really
> correct, as the format of the passed value expands the destination
> field to be 32bits (in order to fit an x2APIC ID).  Passing an
> IO_APIC_route_entry struct is not possible due to the circular
> dependency that would create between io_apic.h and iommu.h.  It might
> be possible to move IO_APIC_route_entry declaration to a different
> header, but I haven't looked into it.
> ---
>  xen/arch/x86/include/asm/iommu.h         |   3 +-
>  xen/arch/x86/io_apic.c                   |   5 +-
>  xen/drivers/passthrough/amd/iommu.h      |   2 +-
>  xen/drivers/passthrough/amd/iommu_intr.c |  98 ++---------------
>  xen/drivers/passthrough/vtd/extern.h     |   2 +-
>  xen/drivers/passthrough/vtd/intremap.c   | 127 +++++++++++------------
>  xen/drivers/passthrough/x86/iommu.c      |   4 +-
>  xen/include/xen/iommu.h                  |   3 +-
>  8 files changed, 80 insertions(+), 164 deletions(-)

Nice diffstat.

> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -84,7 +84,8 @@ struct iommu_init_ops {
>  
>  extern const struct iommu_init_ops *iommu_init_ops;
>  
> -void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, 
> unsigned int value);
> +void iommu_update_ire_from_apic(unsigned int apic, unsigned int pin,
> +                                uint64_t rte);

Much like you have it here, ...

> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -300,7 +300,7 @@ int cf_check amd_iommu_free_intremap_table(
>  unsigned int amd_iommu_intremap_table_order(
>      const void *irt, const struct amd_iommu *iommu);
>  void cf_check amd_iommu_ioapic_update_ire(
> -    unsigned int apic, unsigned int reg, unsigned int value);
> +    unsigned int apic, unsigned int pin, uint64_t raw);

... could the changed parameter also have "rte" in its name? I don't
mind if it's "raw_rte", to avoid colliding with existing variable
names (in at least the VT-d counterpart).

> @@ -350,14 +319,11 @@ static int update_intremap_entry_from_ioapic(
>  }
>  
>  void cf_check amd_iommu_ioapic_update_ire(
> -    unsigned int apic, unsigned int reg, unsigned int value)
> +    unsigned int apic, unsigned int pin, uint64_t raw)
>  {
>      struct IO_APIC_route_entry old_rte = { };

Looks like the initializer here now isn't needed anymore?

> @@ -364,48 +363,37 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu 
> *iommu,
>  
>      new_ire = *iremap_entry;
>  
> -    if ( rte_upper )
> -    {
> -        if ( x2apic_enabled )
> -            new_ire.remap.dst = value;
> -        else
> -            new_ire.remap.dst = (value >> 24) << 8;
> -    }
> +    if ( x2apic_enabled )
> +        new_ire.remap.dst = new_rte.dest.dest32;
>      else
> -    {
> -        *(((u32 *)&new_rte) + 0) = value;
> -        new_ire.remap.fpd = 0;
> -        new_ire.remap.dm = new_rte.dest_mode;
> -        new_ire.remap.tm = new_rte.trigger;
> -        new_ire.remap.dlm = new_rte.delivery_mode;
> -        /* Hardware require RH = 1 for LPR delivery mode */
> -        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> -        new_ire.remap.avail = 0;
> -        new_ire.remap.res_1 = 0;
> -        new_ire.remap.vector = new_rte.vector;
> -        new_ire.remap.res_2 = 0;
> -
> -        set_ioapic_source_id(IO_APIC_ID(apic), &new_ire);
> -        new_ire.remap.res_3 = 0;
> -        new_ire.remap.res_4 = 0;
> -        new_ire.remap.p = 1;     /* finally, set present bit */
> -
> -        /* now construct new ioapic rte entry */
> -        remap_rte->vector = new_rte.vector;
> -        remap_rte->delivery_mode = 0;    /* has to be 0 for remap format */
> -        remap_rte->index_15 = (index >> 15) & 0x1;
> -        remap_rte->index_0_14 = index & 0x7fff;
> -
> -        remap_rte->delivery_status = new_rte.delivery_status;
> -        remap_rte->polarity = new_rte.polarity;
> -        remap_rte->irr = new_rte.irr;
> -        remap_rte->trigger = new_rte.trigger;
> -        remap_rte->mask = new_rte.mask;
> -        remap_rte->reserved = 0;
> -        remap_rte->format = 1;    /* indicate remap format */
> -    }
> -
> -    update_irte(iommu, iremap_entry, &new_ire, !init);
> +        new_ire.remap.dst = (new_rte.dest.dest32 >> 24) << 8;
> +
> +    new_ire.remap.dm = new_rte.dest_mode;
> +    new_ire.remap.tm = new_rte.trigger;
> +    new_ire.remap.dlm = new_rte.delivery_mode;
> +    /* Hardware require RH = 1 for LPR delivery mode */
> +    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> +    new_ire.remap.vector = new_rte.vector;
> +
> +    set_ioapic_source_id(IO_APIC_ID(apic), &new_ire);
> +    new_ire.remap.p = 1;     /* finally, set present bit */
> +
> +    /* now construct new ioapic rte entry */

Nit: Would you mind correcting comment style here as you touch (unindent)
the line?

> +    remap_rte->vector = new_rte.vector;
> +    remap_rte->delivery_mode = 0;    /* has to be 0 for remap format */
> +    remap_rte->index_15 = (index >> 15) & 0x1;
> +    remap_rte->index_0_14 = index & 0x7fff;
> +
> +    remap_rte->delivery_status = new_rte.delivery_status;
> +    remap_rte->polarity = new_rte.polarity;
> +    remap_rte->irr = new_rte.irr;
> +    remap_rte->trigger = new_rte.trigger;
> +    remap_rte->mask = new_rte.mask;
> +    remap_rte->reserved = 0;
> +    remap_rte->format = 1;    /* indicate remap format */
> +
> +    /* If cmpxchg16b is not available the caller must mask the IO-APIC pin. 
> */
> +    update_irte(iommu, iremap_entry, &new_ire, !init && cpu_has_cx16);

The comment suggests to me that you mean to strengthen the logic, yet
the code change weakens it (the called function BUG()s when "atomic"
is true). Wouldn't this better be "!init && !<old RTE was/is masked>"
anyway?

> @@ -439,36 +427,47 @@ unsigned int cf_check io_apic_read_remap_rte(
>  }
>  
>  void cf_check io_apic_write_remap_rte(
> -    unsigned int apic, unsigned int reg, unsigned int value)
> +    unsigned int apic, unsigned int pin, uint64_t raw)
>  {
> -    unsigned int pin = (reg - 0x10) / 2;
> +    struct IO_xAPIC_route_entry rte = { .raw = raw };
>      struct IO_xAPIC_route_entry old_rte = { };
>      struct IO_APIC_route_remap_entry *remap_rte;
> -    unsigned int rte_upper = (reg & 1) ? 1 : 0;
>      struct vtd_iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic));
> -    int saved_mask;
> -
> -    old_rte = __ioapic_read_entry(apic, pin, true);
> +    bool masked = true;
> +    int rc;
>  
>      remap_rte = (struct IO_APIC_route_remap_entry *) &old_rte;
>  
> -    /* mask the interrupt while we change the intremap table */
> -    saved_mask = remap_rte->mask;
> -    remap_rte->mask = 1;
> -    __io_apic_write(apic, reg & ~1, *(u32 *)&old_rte);
> -    remap_rte->mask = saved_mask;
> -
> -    if ( ioapic_rte_to_remap_entry(iommu, apic, pin,
> -                                   &old_rte, rte_upper, value) )
> +    if ( !cpu_has_cx16 )
>      {
> -        __io_apic_write(apic, reg, value);
> +       /*
> +        * Cannot atomically update the IRTE entry: mask the IO-APIC pin to
> +        * avoid interrupts seeing an inconsistent IRTE entry.
> +        */
> +        old_rte = __ioapic_read_entry(apic, pin, true);
> +        if ( !old_rte.mask )
> +        {
> +            masked = false;
> +            old_rte.mask = 1;
> +            __ioapic_write_entry(apic, pin, true, old_rte);
> +        }
> +    }
>  
> -        /* Recover the original value of 'mask' bit */
> -        if ( rte_upper )
> -            __io_apic_write(apic, reg & ~1, *(u32 *)&old_rte);
> +    rc = ioapic_rte_to_remap_entry(iommu, apic, pin, &old_rte, rte);

I realize it has been like this before, but passing &old_rte here is
odd. We already have its properly typed alias: remap_rte. All the
called function does is do the same type cast again. Question is
whether ...

> +    if ( rc )
> +    {
> +        if ( !masked )
> +        {
> +            /* Recover the original value of 'mask' bit */
> +            old_rte.mask = 0;
> +            __ioapic_write_entry(apic, pin, true, old_rte);
> +        }
> +        dprintk(XENLOG_ERR VTDPREFIX,
> +                "failed to update IRTE for IO-APIC#%u pin %u: %d\n",
> +                apic, pin, rc);
> +        return;
>      }
> -    else
> -        __ioapic_write_entry(apic, pin, true, old_rte);
> +    __ioapic_write_entry(apic, pin, true, old_rte);

... the further uses of old_rte then won't end up yet more confusing
than they already are (first and foremost again because of "old" not
being applicable here).

Jan