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

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

On Thu, Jul 27, 2023 at 02:39:06PM +0200, Jan Beulich wrote:
> On 26.07.2023 14:55, Roger Pau Monne wrote:
> > @@ -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?

I think I was trying to mimic the behavior of
amd_iommu_ioapic_update_ire(), which does print the errors as opposed
to doing so in update_intremap_entry_from_ioapic().

I've now removed it, and adjusted the code to match the rest of your
comments.  Will post v4 of 4/4 only as a reply to v3, I don't think
there's a need to resend the rest unless you prefer it that way.

Thanks, Roger.