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 <jbeul...@suse.com>
>> ---
>> 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
