On 27.07.2023 16:19, Roger Pau Monné wrote:
> 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.
Just this patch is going to be fine (maybe even as v3.1).
Jan
