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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2023 14:55, Roger Pau Monne wrote:
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -92,7 +92,7 @@ int cf_check intel_iommu_get_reserved_device_memory(
>  unsigned int cf_check io_apic_read_remap_rte(
>      unsigned int apic, unsigned int reg);
>  void cf_check io_apic_write_remap_rte(
> -    unsigned int apic, unsigned int reg, unsigned int value);
> +    unsigned int apic, unsigned int ioapic_pin, uint64_t rte);

Forgot to rename the middle parameter to "pin"?

> @@ -364,48 +363,40 @@ 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;

I realize this was this way before, but I wonder if we couldn't use
GET_xAPIC_ID() here to reduce the open-coding at least a bit.

> @@ -439,36 +430,45 @@ 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 rte)
>  {
> -    unsigned int pin = (reg - 0x10) / 2;
> +    struct IO_xAPIC_route_entry new_rte = { .raw = rte };
>      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, new_rte);
> +    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);

Afaics you don't alter the error behavior of ioapic_rte_to_remap_entry(),
and in the sole (pre-existing) case of an error a debug log message is
already being issued. Why the addition?

Jan