From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Kevin Tian <kevin.tian@intel.com>, Paul Durrant <paul@xen.org>
Subject: Re: [PATCH v2 06/12] VT-d: respect ACPI SATC's ATC_REQUIRED flag
Date: Wed, 15 May 2024 12:42:40 +0200 [thread overview]
Message-ID: <e9281523-a807-4889-80f0-a13804188af9@suse.com> (raw)
In-Reply-To: <ZjjdZRPluS0YIazc@macbook>
On 06.05.2024 15:38, Roger Pau Monné wrote:
> On Thu, Feb 15, 2024 at 11:16:11AM +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),
>
> I think we should somehow be able to signal dom0 that this device
> might not operate as expected, otherwise dom0 might use it and the
> device could silently malfunction due to ATS not being enabled.
Whatever signaling we invented, no Dom0 would be required to respect it,
and for (perhaps quite) some time no Dom0 kernel would even exist to query
that property.
> Otherwise we should just hide the device from dom0.
This would feel wrong to me, almost like a regression from what we had
before.
> I assume setting the IOMMU context entry to passthrough mode would
> also be fine for such devices that require ATS?
I'm afraid I'm lacking the connection of the question to what is being
done here. Can you perhaps provide some more context? To provide some
context from my side: Using pass-through mode would be excluded when Dom0
is PVH. Hence why I'm not getting why we would want to even just consider
doing so.
Yet, looking at the spec, in pass-through mode translation requests are
treated as UR. So maybe your question was towards there needing to be
handling (whichever way) for the case where pass-through mode was
requested for PV Dom0? The only half-way sensible thing to do in that case
that I can think of right now would be to ignore that command line option,
just like we do when Dom0 is PVH. Yet that would equally apply to use of
"ats" on the command line, i.e. would likely first require yet another
separate patch. Except that in the "ats" case it may be reasonable to
instead panic(), for there being conflicting requests on the command line
(and it being unclear which one would be better to ignore).
>> --- 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) )
>
> Do you need the opt_ats check here?
>
> I don't think it's possible for pci_ats_enabled() to return true if
> opt_ats is <= 0, and hence the opt_ats < 0 check can be dropped from
> the conditional?
In the present tristate mode of opt_ats a device can have ATS enabled when
opt_ats is -1 (i.e. no command line override): For devices with ATC_REQUIRED
set.
>> @@ -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 )
>
> Shouldn't this be opt_ats <= 0?
No, again not as long as this variable is a tristate one.
>> --- 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;
>
> FWIW, I find this change confusing, hence my request earlier that
> opt_ats must be set to 0 or 1 by the point it gets used.
Right, but as said in particular on the subthread of patch 5, for now it has
to remain a full tristate. Whereas if the spec was changed, I expect the
variable could be switched to bool, and hence no overriding from -1 to 0/1
would be needed anymore at all.
Jan
next prev parent reply other threads:[~2024-05-15 10:43 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 10:11 [PATCH v2 00/12] VT-d: SATC handling; ATS: tidying Jan Beulich
2024-02-15 10:13 ` [PATCH v2 01/12] VT-d: correct ATS checking for root complex integrated devices Jan Beulich
2024-05-03 14:01 ` Roger Pau Monné
2024-02-15 10:14 ` [PATCH v2 02/12] VT-d: tidy error handling of RMRR parsing Jan Beulich
2024-05-06 9:12 ` Roger Pau Monné
2024-05-06 9:21 ` Jan Beulich
2024-05-06 9:26 ` Roger Pau Monné
2024-02-15 10:14 ` [PATCH v2 03/12] VT-d: parse ACPI "SoC Integrated Address Translation Cache Reporting Structure"s Jan Beulich
2024-05-06 10:29 ` Roger Pau Monné
2024-05-06 11:01 ` Jan Beulich
2024-05-06 11:09 ` Roger Pau Monné
2024-02-15 10:15 ` [PATCH v2 04/12] AMD/IOMMU: add helper to check whether ATS is to be used for a device Jan Beulich
2024-05-06 11:27 ` Roger Pau Monné
2024-02-15 10:15 ` [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled Jan Beulich
2024-02-15 10:21 ` Jan Beulich
2024-05-06 12:42 ` Roger Pau Monné
2024-05-06 13:20 ` Jan Beulich
2024-05-06 13:53 ` Roger Pau Monné
2024-05-15 10:07 ` Jan Beulich
2024-05-20 10:29 ` Roger Pau Monné
2024-05-21 6:21 ` Jan Beulich
2024-05-21 10:03 ` Roger Pau Monné
2024-05-21 10:23 ` Jan Beulich
2024-02-15 10:16 ` [PATCH v2 06/12] VT-d: respect ACPI SATC's ATC_REQUIRED flag Jan Beulich
2024-05-06 13:38 ` Roger Pau Monné
2024-05-15 10:42 ` Jan Beulich [this message]
2024-05-20 11:36 ` Roger Pau Monné
2024-05-21 6:25 ` Jan Beulich
2024-02-15 10:16 ` [PATCH v2 07/12] VT-d: replace find_ats_dev_drhd() Jan Beulich
2024-02-15 10:17 ` [PATCH v2 08/12] VT-d: move ats_device() to the sole file it's used from Jan Beulich
2024-02-15 10:18 ` [PATCH v2 09/12] VT-d: move dev_invalidate_iotlb() " Jan Beulich
2024-02-15 10:18 ` [PATCH v2 10/12] VT-d: move {,un}map_vtd_domain_page() Jan Beulich
2024-02-15 10:18 ` [PATCH v2 11/12] VT-d: drop flush_dev_iotlb parameter from IOTLB flush hook Jan Beulich
2024-05-06 14:06 ` Roger Pau Monné
2024-02-15 10:19 ` [PATCH v2 12/12] PCI/ATS: tidy {en,dis}able_ats_device() a little Jan Beulich
2024-05-06 14:10 ` Roger Pau Monné
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e9281523-a807-4889-80f0-a13804188af9@suse.com \
--to=jbeulich@suse.com \
--cc=kevin.tian@intel.com \
--cc=paul@xen.org \
--cc=roger.pau@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).