From: Jason Gunthorpe <jgg@nvidia.com>
To: Vidya Sagar <vidyas@nvidia.com>
Cc: corbet@lwn.net, bhelgaas@google.com, galshalom@nvidia.com,
leonro@nvidia.com, treding@nvidia.com, jonathanh@nvidia.com,
mmoshrefjava@nvidia.com, shahafs@nvidia.com, vsethi@nvidia.com,
sdonthineni@nvidia.com, jan@nvidia.com, tdave@nvidia.com,
linux-doc@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, kthota@nvidia.com,
mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V1] PCI: Extend ACS configurability
Date: Wed, 8 May 2024 10:14:27 -0300 [thread overview]
Message-ID: <20240508131427.GH4650@nvidia.com> (raw)
In-Reply-To: <20240508112658.3555882-1-vidyas@nvidia.com>
On Wed, May 08, 2024 at 04:56:58PM +0530, Vidya Sagar wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 41644336e..b4a8207eb 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4456,6 +4456,28 @@
> bridges without forcing it upstream. Note:
> this removes isolation between devices and
> may put more devices in an IOMMU group.
> + config_acs=
> + Format:
> + =<ACS flags>@<pci_dev>[; ...]
> + Specify one or more PCI devices (in the format
> + specified above) optionally prepended with flags
> + and separated by semicolons. The respective
> + capabilities will be enabled, disabled or unchanged
> + based on what is specified in flags.
> + ACS Flags is defined as follows
> + bit-0 : ACS Source Validation
> + bit-1 : ACS Translation Blocking
> + bit-2 : ACS P2P Request Redirect
> + bit-3 : ACS P2P Completion Redirect
> + bit-4 : ACS Upstream Forwarding
> + bit-5 : ACS P2P Egress Control
> + bit-6 : ACS Direct Translated P2P
> + Each bit can be marked as
> + ‘0‘ – force disabled
> + ‘1’ – force enabled
> + ‘x’ – unchanged.
It looks like 'x' doesn't fully work? Or at least it doesn't do what
I'd expect - preserve the FW setting of the bit.
> @@ -1005,6 +1076,7 @@ static void pci_enable_acs(struct pci_dev *dev)
> * preferences.
> */
> pci_disable_acs_redir(dev);
> + pci_config_acs(dev);
Because this sequence starts with:
pci_std_enable_acs(dev);
disable_acs_redir:
pci_disable_acs_redir(dev);
pci_config_acs(dev);
And pci_std_enable_acs() has already mangled the ACS flags:
ctrl |= (cap & PCI_ACS_SV);
ctrl |= (cap & PCI_ACS_RR);
ctrl |= (cap & PCI_ACS_CR);
ctrl |= (cap & PCI_ACS_UF);
if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
So any FW setting on those bits is toast at this point.
It would be nicer if this code was structured a bit more robustly so
that it only wrote the ACS bits once after evaluating all the three
sources of configuration.
But I like the idea, I think this is a nice improvement.
Something sort of like this perhaps:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 94313641bc63fa..64b852ec3d613c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -948,12 +948,20 @@ void pci_request_acs(void)
static const char *disable_acs_redir_param;
static const char *config_acs_param;
-static void __pci_config_acs(struct pci_dev *dev, const char *p,
- u16 mask, u16 flags)
+struct pci_acs {
+ u16 cap;
+ u16 ctrl;
+ u16 fw_ctrl;
+};
+
+static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
+ const char *p, u16 mask, u16 flags)
{
char *delimit;
int ret = 0;
- u16 ctrl, pos;
+
+ if (!p)
+ return;
while (*p) {
if (!mask) {
@@ -1018,98 +1026,37 @@ static void __pci_config_acs(struct pci_dev *dev, const char *p,
if (!pci_dev_specific_disable_acs_redir(dev))
return;
- pos = dev->acs_cap;
- if (!pos) {
- pci_warn(dev, "cannot configure ACS for this hardware as it does not have ACS capabilities\n");
- return;
- }
-
pci_dbg(dev, "ACS mask = 0x%X\n", mask);
pci_dbg(dev, "ACS flags = 0x%X\n", flags);
- pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
- ctrl &= ~mask;
- ctrl |= flags;
- pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ /* If mask is 0 then we copy the bit from the firmware setting. */
+ caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
+ /* FIXME: flags doesn't check for supported? */
+ caps->ctrl |= flags;
- pci_info(dev, "Configured ACS\n");
+ pci_info(dev, "Configured ACS to 0x%x\n", caps->ctrl);
}
-
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
-{
- const char *p;
- u16 mask = 0, flags = 0;
-
- if (!disable_acs_redir_param)
- return;
-
- p = disable_acs_redir_param;
-
- mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC;
- flags = ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
-
- __pci_config_acs(dev, p, mask, flags);
-}
-
-/**
- * pci_config_acs - configure ACS capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the config_acs parameter.
- */
-static void pci_config_acs(struct pci_dev *dev)
-{
- const char *p;
- u16 mask = 0, flags = 0;
-
- if (!config_acs_param)
- return;
-
- p = config_acs_param;
-
- __pci_config_acs(dev, p, mask, flags);
-}
-
/**
* pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
* @dev: the PCI device
*/
-static void pci_std_enable_acs(struct pci_dev *dev)
+static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
{
- int pos;
- u16 cap;
- u16 ctrl;
-
- pos = dev->acs_cap;
- if (!pos)
- return;
-
- pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
- pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
/* Source Validation */
- ctrl |= (cap & PCI_ACS_SV);
+ caps->ctrl |= (caps->cap & PCI_ACS_SV);
/* P2P Request Redirect */
- ctrl |= (cap & PCI_ACS_RR);
+ caps->ctrl |= (caps->cap & PCI_ACS_RR);
/* P2P Completion Redirect */
- ctrl |= (cap & PCI_ACS_CR);
+ caps->ctrl |= (caps->cap & PCI_ACS_CR);
/* Upstream Forwarding */
- ctrl |= (cap & PCI_ACS_UF);
+ caps->ctrl |= (caps->cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices and noats */
if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
- ctrl |= (cap & PCI_ACS_TB);
-
- pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ caps->ctrl |= (caps->cap & PCI_ACS_TB);
}
/**
@@ -1118,24 +1065,33 @@ static void pci_std_enable_acs(struct pci_dev *dev)
*/
static void pci_enable_acs(struct pci_dev *dev)
{
- if (!pci_acs_enable)
- goto disable_acs_redir;
+ struct pci_acs caps;
+ int pos;
- if (!pci_dev_specific_enable_acs(dev))
- goto disable_acs_redir;
+ pos = dev->acs_cap;
+ if (!pos)
+ return;
- pci_std_enable_acs(dev);
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+ caps.fw_ctrl = caps.ctrl;
+
+ /* If an iommu is present we start with kernel default caps */
+ if (pci_acs_enable) {
+ if (pci_dev_specific_enable_acs(dev))
+ pci_std_enable_acs(dev, &caps);
+ }
-disable_acs_redir:
/*
- * Note: pci_disable_acs_redir() must be called even if ACS was not
- * enabled by the kernel because it may have been enabled by
- * platform firmware. So if we are told to disable it, we should
- * always disable it after setting the kernel's default
- * preferences.
+ * Always apply caps from the command line, even if there is no iommu.
+ * Trust that the admin has a reason to change the ACS settings.
*/
- pci_disable_acs_redir(dev);
- pci_config_acs(dev);
+ __pci_config_acs(dev, &caps, disable_acs_redir_param,
+ PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC,
+ ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC));
+ __pci_config_acs(dev, &caps, config_acs_param, 0, 0);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
}
/**
prev parent reply other threads:[~2024-05-08 13:14 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 11:26 [PATCH V1] PCI: Extend ACS configurability Vidya Sagar
2024-05-08 13:14 ` Jason Gunthorpe [this message]
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=20240508131427.GH4650@nvidia.com \
--to=jgg@nvidia.com \
--cc=bhelgaas@google.com \
--cc=corbet@lwn.net \
--cc=galshalom@nvidia.com \
--cc=jan@nvidia.com \
--cc=jonathanh@nvidia.com \
--cc=kthota@nvidia.com \
--cc=leonro@nvidia.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=mmaddireddy@nvidia.com \
--cc=mmoshrefjava@nvidia.com \
--cc=sagar.tv@gmail.com \
--cc=sdonthineni@nvidia.com \
--cc=shahafs@nvidia.com \
--cc=tdave@nvidia.com \
--cc=treding@nvidia.com \
--cc=vidyas@nvidia.com \
--cc=vsethi@nvidia.com \
/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).