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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 19, 2023 at 12:37:47PM +0200, Jan Beulich wrote:
> On 18.07.2023 14:43, Roger Pau Monne wrote:
> > @@ -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).

I've instead opted to remove remap_rte from io_apic_write_remap_rte(),
as it was unused.  I've also added a comment to clarify the usage of
old_rte when ioapic_rte_to_remap_entry() returns success.

Thanks, Roger.