|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 06/12] VT-d: respect ACPI SATC's ATC_REQUIRED flag
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.
Or at least log a warning that ATS is enabled due to a device requiring it.
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -253,6 +253,24 @@ struct acpi_atsr_unit *acpi_find_matched
> return all_ports;
> }
>
> +const struct acpi_satc_unit *acpi_find_matched_satc_unit(
> + const struct pci_dev *pdev)
> +{
> + const struct acpi_satc_unit *satc;
> +
> + list_for_each_entry ( satc, &acpi_satc_units, list )
> + {
> + if ( satc->segment != pdev->seg )
> + continue;
> +
> + for ( unsigned int i = 0; i < satc->scope.devices_cnt; ++i )
> + if ( satc->scope.devices[i] == pdev->sbdf.bdf )
> + return satc;
> + }
> +
> + return NULL;
> +}
> +
> struct acpi_rhsa_unit *drhd_to_rhsa(const struct acpi_drhd_unit *drhd)
> {
> struct acpi_rhsa_unit *rhsa;
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -112,6 +112,8 @@ struct acpi_satc_unit {
>
> struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const struct pci_dev *);
> struct acpi_atsr_unit *acpi_find_matched_atsr_unit(const struct pci_dev *);
> +const struct acpi_satc_unit *acpi_find_matched_satc_unit(
> + const struct pci_dev *pdev);
>
> #define DMAR_TYPE 1
> #define RMRR_TYPE 2
> --- 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).
>
> return ret;
> }
> @@ -2375,12 +2395,26 @@ static int cf_check intel_iommu_enable_d
>
> pci_vtd_quirk(pdev);
>
> - if ( ret <= 0 )
> - return ret;
> + if ( ret <= 0 ||
> + (ret = enable_ats_device(pdev, &drhd->iommu->ats_devices)) < 0 ||
> + opt_ats < 0 )
> + {
> + const struct acpi_satc_unit *satc =
> acpi_find_matched_satc_unit(pdev);
> +
> + /*
> + * Besides in error cases also prevent the device from getting
> assigned
> + * to an unprivileged domain when firmware indicates ATS is required,
> + * but ATS use was not explicitly enabled via command line option.
> + */
> + if ( satc && satc->atc_required && !pdev->broken )
> + {
> + printk(XENLOG_WARNING "ATS: %pp is not eligible for
> pass-through\n",
> + &pdev->sbdf);
> + pdev->broken = true;
> + }
> + }
>
> - ret = enable_ats_device(pdev, &drhd->iommu->ats_devices);
> -
> - return ret >= 0 ? 0 : ret;
> + return ret <= 0 ? ret : 0;
> }
>
> static int cf_check intel_iommu_remove_device(u8 devfn, struct pci_dev
> *pdev)
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -45,8 +45,9 @@ int ats_device(const struct pci_dev *pde
> {
> struct acpi_drhd_unit *ats_drhd;
> unsigned int pos, expfl = 0;
> + const struct acpi_satc_unit *satc;
>
> - if ( opt_ats <= 0 || !iommu_qinval )
> + if ( !opt_ats || !iommu_qinval )
> return 0;
>
> if ( !ecap_queued_inval(drhd->iommu->ecap) ||
> @@ -61,6 +62,10 @@ int ats_device(const struct pci_dev *pde
> !acpi_find_matched_atsr_unit(pdev) )
> return 0;
>
> + satc = acpi_find_matched_satc_unit(pdev);
> + if ( opt_ats < 0 && (!satc || !satc->atc_required) )
> + return 0;
> +
> ats_drhd = find_ats_dev_drhd(drhd->iommu);
> pos = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_ATS);
>
>
>
>
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |