Re: [PATCH 3/7] VT-d: respect ACPI SATC's ATC_REQUIRED flag

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 13:42, Roger Pau Monné wrote:
> On Mon, Feb 05, 2024 at 02:56:14PM +0100, Jan Beulich wrote:
>> 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.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Is ats_device() using acpi_find_matched_atsr_unit() unconditionally
>> actually correct? Shouldn't that check be skipped for root complex
>> integrated devices?
> 
> Yes, I think so, ATSR only lists root ports supporting ATS, because
> the root complex is assumed to always be ATS capable.
> 
> None of this seems to be working then for PCIe endpoints directly in
> the root complex, as ats_device() will always return 0?

That's my understanding. I've now added a bugfix patch near the front of
the series.

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -225,7 +225,10 @@ 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.
> 
> I'm kind of worried that we add this support while maintaining the
> WARNING below.  If I was an admin I would certainly be worried whether
> my system could lock-up during normal operations, even with the
> devices assigned to dom0 and not a malicious domain.
> 
> I know that enabling ATS is forced on us from DMAR, but still.

I'm with you; see below.

>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -2364,6 +2364,25 @@ 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
>> +         * (e.g. because of being suppressed via command line option).
>> +         */
> 
> I think a safer policy would be to prevent assigning any device that
> has atc_required set unless opt_ats > 1 (ie: the user has explicitly
> opted-in to the usage of ATS).
> 
> While we can't likely avoid ATS being enabled for devices having the
> ATC_REQUIRED flag, we shouldn't allow passthrough to possibly
> untrusted guests without notice.

Switched to that model, including respective wording in the cmdline doc.

Jan