Re: [PATCH RFC 5/6] amd/iommu: atomically update remapping entries when possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2022 15:21, Roger Pau Monne wrote:
> @@ -366,8 +383,11 @@ void cf_check amd_iommu_ioapic_update_ire(
>          fresh = true;
>      }
>  
> -    /* mask the interrupt while we change the intremap table */
> -    if ( !saved_mask )
> +    /*
> +     * Mask the interrupt while we change the intremap table if it can't be
> +     * done atomically.
> +     */
> +    if ( !saved_mask && !cpu_has_cx16 && iommu->ctrl.ga_en )
>      {
>          old_rte.mask = 1;
>          __ioapic_write_entry(apic, pin, true, old_rte);
> @@ -383,6 +403,11 @@ void cf_check amd_iommu_ioapic_update_ire(
>          /* Keep the entry masked. */
>          printk(XENLOG_ERR "Remapping IO-APIC %#x pin %u failed (%d)\n",
>                 IO_APIC_ID(apic), pin, rc);
> +        if ( !saved_mask && (cpu_has_cx16 || !iommu->ctrl.ga_en) )
> +        {
> +            old_rte.mask = 1;
> +            __ioapic_write_entry(apic, pin, true, old_rte);
> +        }
>          return;
>      }

Iirc you have a question about this (wrt differing VT-d behavior)
elsewhere. I'm not convinced of this masking. I could see it as a
measure to prevent damage if an update was done partially, but I
don't see such being possible here. Hence to me it would look more
logical if the entry was simply left as is, leaving it to the
caller to correctly deal with the failure. But of course that
would first require telling the caller about the failure ...

Jan