Re: [PATCH v2 06/12] VT-d: respect ACPI SATC's ATC_REQUIRED flag

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2025 15:30, Teddy Astie wrote:
> Le 23/10/2025 à 15:14, Jan Beulich a écrit :
>> When the flag is set, permit Dom0 to control the device (no worse than
>> what we had before and in line with other "best effort" behavior we use
>> when it comes to Dom0), but suppress passing through to DomU-s unless
>> ATS can actually be enabled for such devices (and was explicitly enabled
>> on the command line).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Re-base over new earlier patches.
>>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -225,7 +225,11 @@ exceptions (watchdog NMIs and unexpected
>>   > Default: `false`
>>   
>>   Permits Xen to set up and use PCI Address Translation Services.  This is a
>> -performance optimisation for PCI Passthrough.
>> +performance optimisation for PCI Passthrough.  Note that firmware may 
>> indicate
>> +that certain devices need to have ATS enabled for proper operation. For such
>> +devices ATS will be enabled by default, unless the option is used in its
>> +negative form.  Such devices will still not be eligible for passing through 
>> to
>> +guests, unless the option is used in its positive form.
>>   
>>   **WARNING: Xen cannot currently safely use ATS because of its synchronous 
>> wait
>>   loops for Queued Invalidation completions.**
> 
> Do we want to address the warning before attempting to unconditionnaly 
> enable ATS in these scenarios ? A unstable hypervisor is likely worse 
> than a non-functionning device to me.

Addressing this requires, afaict, lots of changes. Such devices also can still
only be used by Dom0 unless ATS is explicitly enabled from the command line.
Whether a non-functioning device is worse than a (only possibly) "unstable"
hypervisor is also hard to tell. Dom0 may fail to boot if the "right" device is
affected. ("Possibly" because the synchronous wait loops of course are of
concern only if they end up taking long.)

> Or at least log a warning that ATS is enabled due to a device requiring it.

This would need to be a per-device message, which may not scale very well.

>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -2364,6 +2364,26 @@ static int cf_check intel_iommu_add_devi
>>       if ( ret )
>>           dprintk(XENLOG_ERR VTDPREFIX, "%pd: context mapping failed\n",
>>                   pdev->domain);
>> +    else if ( !pdev->broken )
>> +    {
>> +        const struct acpi_drhd_unit *drhd = 
>> acpi_find_matched_drhd_unit(pdev);
>> +        const struct acpi_satc_unit *satc = 
>> acpi_find_matched_satc_unit(pdev);
>> +
>> +        /*
>> +         * Prevent the device from getting assigned to an unprivileged 
>> domain
>> +         * when firmware indicates ATS is required, but ATS could not be 
>> enabled
>> +         * or was not explicitly enabled via command line option.
>> +         */
>> +        if ( satc && satc->atc_required &&
>> +             (!drhd || ats_device(pdev, drhd) <= 0 ||
>> +              !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) ||
>> +              opt_ats < 0) )
>> +        {
>> +            printk(XENLOG_WARNING "ATS: %pp is not eligible for 
>> pass-through\n",
>> +                   &pdev->sbdf);
>> +            pdev->broken = true;
>> +        }
>> +    }
> 
> I don't feel pdev->broken is the right way for signaling ineligibility 
> for passthrough due to policy (ATS required).
> Especially if we eventually consider in the future allowing on a 
> per-domain basis the ability to use ATS (starting with Dom0).

Well, pdev->broken is what we have available. Anything better can come later,
imo. For now the goal has been to at least get in line with the spec. That said,
while - afaik - not written down anywhere, back at the time I got indications
that the "required" in ATC_REQUIRED may not be as strict an indication as the
word may suggest.

Jan