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

[Jan Beulich <jbeulich@xxxxxxxx>]

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