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
