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.
