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>,
Paul Durrant <paul@xen.org>
Subject: Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled
Date: Wed, 15 May 2024 12:07:50 +0200 [thread overview]
Message-ID: <44af358a-9510-4056-826c-3be99dc25830@suse.com> (raw)
In-Reply-To: <Zjjg2ueqgjmn-MS3@macbook>
On 06.05.2024 15:53, Roger Pau Monné wrote:
> On Mon, May 06, 2024 at 03:20:38PM +0200, Jan Beulich wrote:
>> On 06.05.2024 14:42, Roger Pau Monné wrote:
>>> On Thu, Feb 15, 2024 at 11:15:39AM +0100, Jan Beulich wrote:
>>>> Make the variable a tristate, with (as done elsewhere) a negative value
>>>> meaning "default". Since all use sites need looking at, also rename it
>>>> to match our usual "opt_*" pattern. While touching it, also move it to
>>>> .data.ro_after_init.
>>>
>>> I guess I need to look at further patches, as given the feedback on
>>> the past version I think we agreed we want to set ATS unconditionally
>>> disabled by default, and hence I'm not sure I see the point of the
>>> tri-state if enabling ATS will require an explicit opt-in on the
>>> command line (ats=1).
>>
>> With the present wording in the VT-d spec (which we've now had vague
>> indication that it may not be meant that way) there needs to be
>> tristate behavior:
>> - With "ats=0" ATS won't be used.
>> - With "ats=1" ATS will be used for all ATS-capable devices.
>> - Without either option ATS will be used for devices where firmware
>> mandates its use.
>
> I'm afraid I don't agree to this behavior. Regardless of what the
> firmware requests ATS must only be enabled on user-request (iow: when
> the ats=1 command line option is passed). Otherwise ATS must remain
> disabled for all devices. It's not fine for firmware to trigger the
> enabling of a feature that's not supported on Xen.
Well. On one hand I can see your point. Otoh with the spec still being the
way it is, on systems mandating ATS use for at least one device we'd then
simply need to deem Xen unsupported there altogether. The goal of the
series, though, is to make things work as mandated by the spec on such
systems, which to me implies we need to consider use of ATS supported in
such cases (and only for those specific devices, i.e. still without
considering use of "ats" on the command line supported).
If and when the spec was changed to clarify the flag is a performance hint,
not a functional requirement, then we could do as you suggest. At which
point, as mentioned before, opt_ats may be possible to become a plain
boolean variable.
>>>> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_
>>>> dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, ACPI_IVHD_SYSTEM_MGMT);
>>>>
>>>> if ( use_ats(pdev, iommu, ivrs_dev) )
>>>> - dte->i = ats_enabled;
>>>> + dte->i = true;
>>>
>>> Might be easier to just use:
>>>
>>> dte->i = use_ats(pdev, iommu, ivrs_dev);
>>
>> I'm hesitant here, as in principle we might be overwriting a "true" by
>> "false" then.
>
> Hm, but that would be fine, what's the point in enabling the IOMMU to
> reply to ATS requests if ATS is not enabled on the device?
>
> IOW: overwriting a "true" with a "false" seem like the correct
> behavior if it's based on the output of use_ats().
I don't think so, unless there were flow guarantees excluding the possibility
of taking this path twice without intermediately disabling the device again.
Down from here the enabling of ATS is gated on use_ats(). Hence if, in an
earlier invocation, we enabled ATS (and set dte->i), we wouldn't turn off ATS
below (there's only code to turn it on), yet with what you suggest we'd clear
dte->i.
Thinking about it: Maybe your comment roots in you meaning to leverage here
that use_ats() is not supposed to return different values for the same device,
when invoked multiple times. If so, I'm afraid I'm hesitant to make use of
such a property when I can easily avoid it.
>>>> @@ -43,7 +43,7 @@ static inline int pci_ats_enabled(int se
>>>>
>>>> static inline int pci_ats_device(int seg, int bus, int devfn)
>>>> {
>>>> - if ( !ats_enabled )
>>>> + if ( !opt_ats )
>>>> return 0;
>>>
>>> Can't you remove that check altogether now, since you are adding an
>>> opt_ats check to use_ats()?
>>
>> Two reasons why not: For one this isn't AMD-specific code, and hence
>> shouldn't be tied to the AMD-specific use_ats(). In principle VT-d
>> code should be okay to call here, too. And then
>> amd_iommu_disable_domain_device() doesn't use use_ats(), but does call
>> here.
>
> Oh, that's confusing, I didn't realize use_ats was AMD specific code.
> It should have some kind of prefix to avoid this kind of confusion.
Hmm, the function being static in an AMD-only file, I would have thought that
makes it clear enough that it's AMD-specific.
Jan
next prev parent reply other threads:[~2024-05-15 10:08 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 [this message]
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
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=44af358a-9510-4056-826c-3be99dc25830@suse.com \
--to=jbeulich@suse.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).