This patch enables P2P upstream forwarding in ACS capable PCIe switches. This solves two potential problems in virtualization environment where a PCIe device is assigned to a guest domain using a HW iommu such as VT-d:
1) Unintentional failure caused by guest physical address programmed into the device's DMA that happens to match the memory address range of other downstream ports in the same PCIe switch. This causes the PCI transaction to go to the matching downstream port instead of go to the root complex to get translated by VT-d as it should be.
2) Malicious guest software intentionally attacks another downstream PCIe device by programming the DMA address into the assigned device that matches memory address range of the downstream PCIe port.
We are in process of implementing device filtering software in KVM/XEN management software to allow device assignment of PCIe devices behind a PCIe switch only if it has ACS capability and with the P2P upstream forwarding bits enabled. This patch is intended to work for both KVM and Xen environments.
Signed-off-by: Allen Kay [email protected]
---
drivers/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 3 +++
include/linux/pci_regs.h | 14 ++++++++++++++
4 files changed, 55 insertions(+)
=================================================================
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7b70312..e70abde 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1513,6 +1513,49 @@ void pci_enable_ari(struct pci_dev *dev)
}
/**
+ * pci_acs_enable - enable ACS if hardware support it
+ * @dev: the PCI device
+ */
+#define ACS_ENABLED (PCI_EXP_ACS_V|PCI_EXP_ACS_R|PCI_EXP_ACS_C|PCI_EXP_ACS_U)
+void pci_acs_init(struct pci_dev *dev)
+{
+ int pos;
+ u16 cap;
+ u16 ctrl;
+
+ if (!dev->is_pcie)
+ return;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos)
+ return;
+
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
+ if ((cap & ACS_ENABLED) != ACS_ENABLED)
+ dev_info(&dev->dev, "ACS P2P upstream not supported\n");
+
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+
+ /* Source Validation */
+ if (cap & PCI_EXP_ACS_V)
+ ctrl |= PCI_EXP_ACS_V;
+
+ /* P2P Request Redirect */
+ if (cap & PCI_EXP_ACS_R)
+ ctrl |= PCI_EXP_ACS_R;
+
+ /* P2P Completion Redirect */
+ if (cap & PCI_EXP_ACS_C)
+ ctrl |= PCI_EXP_ACS_C;
+
+ /* Upstream Forwarding */
+ if (cap & PCI_EXP_ACS_U)
+ ctrl |= PCI_EXP_ACS_U;
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+}
+
+/**
* pci_swizzle_interrupt_pin - swizzle INTx for device behind bridge
* @dev: the PCI device
* @pin: the INTx pin (1=INTA, 2=INTB, 3=INTD, 4=INTD)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5ff4d25..1d8976d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -202,6 +202,7 @@ static inline int pci_ari_enabled(struct pci_bus *bus)
{
return bus->self && bus->self->ari_enabled;
}
+extern void pci_acs_init(struct pci_dev *dev);
#ifdef CONFIG_PCI_QUIRKS
extern int pci_is_reassigndev(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 40e75f6..4a5ec9e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -991,6 +991,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
/* Single Root I/O Virtualization */
pci_iov_init(dev);
+
+ /* Access Control Service */
+ pci_acs_init(dev);
}
void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index fcaee42..90014ad 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -492,6 +492,14 @@
#define PCI_EXP_LNKCTL2 48 /* Link Control 2 */
#define PCI_EXP_SLTCTL2 56 /* Slot Control 2 */
+#define PCI_EXP_ACS_V 0x01 /* Source Validation */
+#define PCI_EXP_ACS_B 0x02 /* Translation Blocking */
+#define PCI_EXP_ACS_R 0x04 /* P2P Request Redirect */
+#define PCI_EXP_ACS_C 0x08 /* P2P Completion Redirect */
+#define PCI_EXP_ACS_U 0x10 /* Upstream Forwarding */
+#define PCI_EXP_ACS_E 0x20 /* P2P Egress Control */
+#define PCI_EXP_ACS_T 0x40 /* Direct Translated P2P */
+
/* Extended Capabilities (PCI-X 2.0 and Express) */
#define PCI_EXT_CAP_ID(header) (header & 0x0000ffff)
#define PCI_EXT_CAP_VER(header) ((header >> 16) & 0xf)
@@ -501,6 +509,7 @@
#define PCI_EXT_CAP_ID_VC 2
#define PCI_EXT_CAP_ID_DSN 3
#define PCI_EXT_CAP_ID_PWR 4
+#define PCI_EXT_CAP_ID_ACS 13
#define PCI_EXT_CAP_ID_ARI 14
#define PCI_EXT_CAP_ID_ATS 15
#define PCI_EXT_CAP_ID_SRIOV 16
@@ -661,4 +670,9 @@
#define PCI_SRIOV_VFM_MO 0x2 /* Active.MigrateOut */
#define PCI_SRIOV_VFM_AV 0x3 /* Active.Available */
+/* Access Control Service */
+#define PCI_ACS_CAP 0x04 /* ACS Capability Register */
+#define PCI_ACS_CTRL 0x06 /* ACS Control Register */
+#define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */
+
#endif /* LINUX_PCI_REGS_H */
On Tue, 2009-09-15 at 16:44 -0700, Kay, Allen M wrote:
> This patch enables P2P upstream forwarding in ACS capable PCIe switches. This solves two potential problems in virtualization environment where a PCIe device is assigned to a guest domain using a HW iommu such as VT-d:
>
> 1) Unintentional failure caused by guest physical address programmed into the device's DMA that happens to match the memory address range of other downstream ports in the same PCIe switch. This causes the PCI transaction to go to the matching downstream port instead of go to the root complex to get translated by VT-d as it should be.
>
> 2) Malicious guest software intentionally attacks another downstream PCIe device by programming the DMA address into the assigned device that matches memory address range of the downstream PCIe port.
>
> We are in process of implementing device filtering software in KVM/XEN management software to allow device assignment of PCIe devices behind a PCIe switch only if it has ACS capability and with the P2P upstream forwarding bits enabled. This patch is intended to work for both KVM and Xen environments.
>
> Signed-off-by: Allen Kay [email protected]
Both times that you've submitted this the patch has been corrupt.. Your
mailer, or something is adding little "=20" or "=3D" all over. For
instance,
"Signed-off-by: Allen Kay [email protected]
---
drivers/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 3 +++
include/linux/pci_regs.h | 14 ++++++++++++++
4 files changed, 55 insertions(+)
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7b70312..e70abde 100644
..."
So there's no way to test or apply your patch .. Most reviewers are just
going to walk away if they see stuff like this..
Daniel
Are you able to look at the file in the attachment?
-----Original Message-----
From: Daniel Walker [mailto:[email protected]]
Sent: Tuesday, September 15, 2009 5:12 PM
To: Kay, Allen M
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] enabling ACS P2P upstream forwarding
On Tue, 2009-09-15 at 16:44 -0700, Kay, Allen M wrote:
> This patch enables P2P upstream forwarding in ACS capable PCIe switches. This solves two potential problems in virtualization environment where a PCIe device is assigned to a guest domain using a HW iommu such as VT-d:
>
> 1) Unintentional failure caused by guest physical address programmed into the device's DMA that happens to match the memory address range of other downstream ports in the same PCIe switch. This causes the PCI transaction to go to the matching downstream port instead of go to the root complex to get translated by VT-d as it should be.
>
> 2) Malicious guest software intentionally attacks another downstream PCIe device by programming the DMA address into the assigned device that matches memory address range of the downstream PCIe port.
>
> We are in process of implementing device filtering software in KVM/XEN management software to allow device assignment of PCIe devices behind a PCIe switch only if it has ACS capability and with the P2P upstream forwarding bits enabled. This patch is intended to work for both KVM and Xen environments.
>
> Signed-off-by: Allen Kay [email protected]
Both times that you've submitted this the patch has been corrupt.. Your
mailer, or something is adding little "=20" or "=3D" all over. For
instance,
"Signed-off-by: Allen Kay [email protected]
---
drivers/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 1 +
drivers/pci/probe.c | 3 +++
include/linux/pci_regs.h | 14 ++++++++++++++
4 files changed, 55 insertions(+)
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7b70312..e70abde 100644
..."
So there's no way to test or apply your patch .. Most reviewers are just
going to walk away if they see stuff like this..
Daniel
On Tue, 2009-09-15 at 17:13 -0700, Kay, Allen M wrote:
> Are you able to look at the file in the attachment?
>
Yeah I can, but attachments aren't the proper way to submit patches to
this list.. There are some easy way to do it. For instance, if your
using git, git includes programs called "git-format-patch" and
"git-send-email" which you can use to submit your changes un-corrupted.
They aren't very difficult to use..
Daniel
On Tue, Sep 15, 2009 at 05:12:17PM -0700, Daniel Walker wrote:
...
> Both times that you've submitted this the patch has been corrupt.. Your
> mailer, or something is adding little "=20" or "=3D" all over. For
> instance,
>
> "Signed-off-by: Allen Kay [email protected]
>
> ---
>
> drivers/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 1 +
> drivers/pci/probe.c | 3 +++
> include/linux/pci_regs.h | 14 ++++++++++++++
> 4 files changed, 55 insertions(+)
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
I did not see any "=3D" in the posting - using "mutt" to read the email.
And AFAIK, many maintainers will accept attachments if that's what works.
Sure, it's not preferred, but pragmatic folks manage to get over that.
hth
grant
On Tue, 2009-09-15 at 23:46 -0600, Grant Grundler wrote:
> On Tue, Sep 15, 2009 at 05:12:17PM -0700, Daniel Walker wrote:
> ...
> > Both times that you've submitted this the patch has been corrupt.. Your
> > mailer, or something is adding little "=20" or "=3D" all over. For
> > instance,
> >
> > "Signed-off-by: Allen Kay [email protected]
> >
> > ---
> >
> > drivers/pci/pci.c | 37 +++++++++++++++++++++++++++++++++++++
> > drivers/pci/pci.h | 1 +
> > drivers/pci/probe.c | 3 +++
> > include/linux/pci_regs.h | 14 ++++++++++++++
> > 4 files changed, 55 insertions(+)
> >
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> I did not see any "=3D" in the posting - using "mutt" to read the email.
I've seen them in two different mailing systems, so I'm pretty sure they
are there.
> And AFAIK, many maintainers will accept attachments if that's what works.
> Sure, it's not preferred, but pragmatic folks manage to get over that.
Sure in a pinch, but learning how to do it a better way is good too.
Daniel
On Tue, Sep 15, 2009 at 04:44:00PM -0700, Kay, Allen M wrote:
> This patch enables P2P upstream forwarding in ACS capable PCIe switches.
> This solves two potential problems in virtualization environment where
> a PCIe device is assigned to a guest domain using a HW iommu such as VT-d:
Seems like a good idea.
> @@ -1513,6 +1513,49 @@ void pci_enable_ari(struct pci_dev *dev)
> }
>
> /**
> + * pci_acs_enable - enable ACS if hardware support it
> + * @dev: the PCI device
> + */
> +#define ACS_ENABLED (PCI_EXP_ACS_V|PCI_EXP_ACS_R|PCI_EXP_ACS_C|PCI_EXP_ACS_U)
This is a little hard to read. I find it easier with spaces, ie:
#define ACS_ENABLED (PCI_EXP_ACS_V | PCI_EXP_ACS_R | PCI_EXP_ACS_C | \
PCI_EXP_ACS_U)
Now it doesn't fit on one line, but see below.
> +void pci_acs_init(struct pci_dev *dev)
> +{
> + int pos;
> + u16 cap;
> + u16 ctrl;
> +
> + if (!dev->is_pcie)
> + return;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> + if (!pos)
> + return;
> +
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + if ((cap & ACS_ENABLED) != ACS_ENABLED)
> + dev_info(&dev->dev, "ACS P2P upstream not supported\n");
As I read the ACS spec, the Source Validation and Upstream Forwarding
bits must not be implemented for multifunction devices, so you'll produce
this warning for all non-downstream ports that implement ACS. Not that
I have any of those.
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
Couldn't this:
> + /* Source Validation */
> + if (cap & PCI_EXP_ACS_V)
> + ctrl |= PCI_EXP_ACS_V;
> +
> + /* P2P Request Redirect */
> + if (cap & PCI_EXP_ACS_R)
> + ctrl |= PCI_EXP_ACS_R;
> +
> + /* P2P Completion Redirect */
> + if (cap & PCI_EXP_ACS_C)
> + ctrl |= PCI_EXP_ACS_C;
> +
> + /* Upstream Forwarding */
> + if (cap & PCI_EXP_ACS_U)
> + ctrl |= PCI_EXP_ACS_U;
be written more pithily as:
ctrl |= (cap & ACS_ENABLED);
?
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> +}
> +
> +/**
> * pci_swizzle_interrupt_pin - swizzle INTx for device behind bridge
> * @dev: the PCI device
> * @pin: the INTx pin (1=INTA, 2=INTB, 3=INTD, 4=INTD)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5ff4d25..1d8976d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -202,6 +202,7 @@ static inline int pci_ari_enabled(struct pci_bus *bus)
> {
> return bus->self && bus->self->ari_enabled;
> }
> +extern void pci_acs_init(struct pci_dev *dev);
>
> #ifdef CONFIG_PCI_QUIRKS
> extern int pci_is_reassigndev(struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 40e75f6..4a5ec9e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -991,6 +991,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
>
> /* Single Root I/O Virtualization */
> pci_iov_init(dev);
> +
> + /* Access Control Service */
> + pci_acs_init(dev);
> }
>
> void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index fcaee42..90014ad 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -492,6 +492,14 @@
> #define PCI_EXP_LNKCTL2 48 /* Link Control 2 */
> #define PCI_EXP_SLTCTL2 56 /* Slot Control 2 */
>
> +#define PCI_EXP_ACS_V 0x01 /* Source Validation */
> +#define PCI_EXP_ACS_B 0x02 /* Translation Blocking */
> +#define PCI_EXP_ACS_R 0x04 /* P2P Request Redirect */
> +#define PCI_EXP_ACS_C 0x08 /* P2P Completion Redirect */
> +#define PCI_EXP_ACS_U 0x10 /* Upstream Forwarding */
> +#define PCI_EXP_ACS_E 0x20 /* P2P Egress Control */
> +#define PCI_EXP_ACS_T 0x40 /* Direct Translated P2P */
These definitions are in the wrong place and have the wrong name ;-)
You've put them in the part of the file used for the PCI Express capability,
and named them as if they're part of the PCI Express capability.
> /* Extended Capabilities (PCI-X 2.0 and Express) */
> #define PCI_EXT_CAP_ID(header) (header & 0x0000ffff)
> #define PCI_EXT_CAP_VER(header) ((header >> 16) & 0xf)
> @@ -501,6 +509,7 @@
> #define PCI_EXT_CAP_ID_VC 2
> #define PCI_EXT_CAP_ID_DSN 3
> #define PCI_EXT_CAP_ID_PWR 4
> +#define PCI_EXT_CAP_ID_ACS 13
> #define PCI_EXT_CAP_ID_ARI 14
> #define PCI_EXT_CAP_ID_ATS 15
> #define PCI_EXT_CAP_ID_SRIOV 16
> @@ -661,4 +670,9 @@
> #define PCI_SRIOV_VFM_MO 0x2 /* Active.MigrateOut */
> #define PCI_SRIOV_VFM_AV 0x3 /* Active.Available */
>
> +/* Access Control Service */
> +#define PCI_ACS_CAP 0x04 /* ACS Capability Register */
They should be down here instead and named like this:
+#define PCI_ACS_SV 0x01 /* Source Validation */
+#define PCI_ACS_TB 0x02 /* Translation Blocking */
+#define PCI_ACS_RR 0x04 /* P2P Request Redirect */
+#define PCI_ACS_CR 0x08 /* P2P Completion Redirect */
+#define PCI_ACS_UF 0x10 /* Upstream Forwarding */
+#define PCI_ACS_EC 0x20 /* P2P Egress Control */
+#define PCI_ACS_DT 0x40 /* Direct Translated P2P */
Now ACS_ENABLED fits on one line again ;-)
#define ACS_ENABLED (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
> +#define PCI_ACS_CTRL 0x06 /* ACS Control Register */
> +#define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */
> +
> #endif /* LINUX_PCI_REGS_H */
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, 15 Sep 2009 17:12:17 -0700
Daniel Walker <[email protected]> wrote:
> Both times that you've submitted this the patch has been corrupt.. Your
> mailer, or something is adding little "=20" or "=3D" all over.
That looks a lot like *your* mail client not understanding
quoted-printable encoding. QP can be a pain, but it is a legitimate
encoding; the message in question looks fine in claws.
jon
On Wed, 2009-09-16 at 09:34 -0600, Jonathan Corbet wrote:
> On Tue, 15 Sep 2009 17:12:17 -0700
> Daniel Walker <[email protected]> wrote:
>
> > Both times that you've submitted this the patch has been corrupt.. Your
> > mailer, or something is adding little "=20" or "=3D" all over.
>
> That looks a lot like *your* mail client not understanding
> quoted-printable encoding. QP can be a pain, but it is a legitimate
> encoding; the message in question looks fine in claws.
Shouldn't all patches be sent as plain text untouched? I don't think I
should necessarily need a specific email client to strip off some
characters..
Daniel
> Shouldn't all patches be sent as plain text untouched? I don't think I
> should necessarily need a specific email client to strip off some
> characters..
Plain text is 7bit (netascii). People have unicode names.
On Wed, 2009-09-16 at 17:41 +0100, Alan Cox wrote:
> > Shouldn't all patches be sent as plain text untouched? I don't think I
> > should necessarily need a specific email client to strip off some
> > characters..
>
> Plain text is 7bit (netascii). People have unicode names.
Is "=20" or "=3D" unicode encodings? Seems like we do allow UTF-8 for
patches.. For this patch I don't see why you would need unicode in the
actual patch content or for names tho.
Daniel
Thank you for your review and comments. I've sent out a new version of the patch that incorporates your feedbacks.
Allen
-----Original Message-----
From: Matthew Wilcox [mailto:[email protected]]
Sent: Wednesday, September 16, 2009 2:58 AM
To: Kay, Allen M
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] enabling ACS P2P upstream forwarding
On Tue, Sep 15, 2009 at 04:44:00PM -0700, Kay, Allen M wrote:
> This patch enables P2P upstream forwarding in ACS capable PCIe switches.
> This solves two potential problems in virtualization environment where
> a PCIe device is assigned to a guest domain using a HW iommu such as VT-d:
Seems like a good idea.
> @@ -1513,6 +1513,49 @@ void pci_enable_ari(struct pci_dev *dev)
> }
>
> /**
> + * pci_acs_enable - enable ACS if hardware support it
> + * @dev: the PCI device
> + */
> +#define ACS_ENABLED (PCI_EXP_ACS_V|PCI_EXP_ACS_R|PCI_EXP_ACS_C|PCI_EXP_ACS_U)
This is a little hard to read. I find it easier with spaces, ie:
#define ACS_ENABLED (PCI_EXP_ACS_V | PCI_EXP_ACS_R | PCI_EXP_ACS_C | \
PCI_EXP_ACS_U)
Now it doesn't fit on one line, but see below.
> +void pci_acs_init(struct pci_dev *dev)
> +{
> + int pos;
> + u16 cap;
> + u16 ctrl;
> +
> + if (!dev->is_pcie)
> + return;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> + if (!pos)
> + return;
> +
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> + if ((cap & ACS_ENABLED) != ACS_ENABLED)
> + dev_info(&dev->dev, "ACS P2P upstream not supported\n");
As I read the ACS spec, the Source Validation and Upstream Forwarding
bits must not be implemented for multifunction devices, so you'll produce
this warning for all non-downstream ports that implement ACS. Not that
I have any of those.
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
Couldn't this:
> + /* Source Validation */
> + if (cap & PCI_EXP_ACS_V)
> + ctrl |= PCI_EXP_ACS_V;
> +
> + /* P2P Request Redirect */
> + if (cap & PCI_EXP_ACS_R)
> + ctrl |= PCI_EXP_ACS_R;
> +
> + /* P2P Completion Redirect */
> + if (cap & PCI_EXP_ACS_C)
> + ctrl |= PCI_EXP_ACS_C;
> +
> + /* Upstream Forwarding */
> + if (cap & PCI_EXP_ACS_U)
> + ctrl |= PCI_EXP_ACS_U;
be written more pithily as:
ctrl |= (cap & ACS_ENABLED);
?
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> +}
> +
> +/**
> * pci_swizzle_interrupt_pin - swizzle INTx for device behind bridge
> * @dev: the PCI device
> * @pin: the INTx pin (1=INTA, 2=INTB, 3=INTD, 4=INTD)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5ff4d25..1d8976d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -202,6 +202,7 @@ static inline int pci_ari_enabled(struct pci_bus *bus)
> {
> return bus->self && bus->self->ari_enabled;
> }
> +extern void pci_acs_init(struct pci_dev *dev);
>
> #ifdef CONFIG_PCI_QUIRKS
> extern int pci_is_reassigndev(struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 40e75f6..4a5ec9e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -991,6 +991,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
>
> /* Single Root I/O Virtualization */
> pci_iov_init(dev);
> +
> + /* Access Control Service */
> + pci_acs_init(dev);
> }
>
> void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index fcaee42..90014ad 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -492,6 +492,14 @@
> #define PCI_EXP_LNKCTL2 48 /* Link Control 2 */
> #define PCI_EXP_SLTCTL2 56 /* Slot Control 2 */
>
> +#define PCI_EXP_ACS_V 0x01 /* Source Validation */
> +#define PCI_EXP_ACS_B 0x02 /* Translation Blocking */
> +#define PCI_EXP_ACS_R 0x04 /* P2P Request Redirect */
> +#define PCI_EXP_ACS_C 0x08 /* P2P Completion Redirect */
> +#define PCI_EXP_ACS_U 0x10 /* Upstream Forwarding */
> +#define PCI_EXP_ACS_E 0x20 /* P2P Egress Control */
> +#define PCI_EXP_ACS_T 0x40 /* Direct Translated P2P */
These definitions are in the wrong place and have the wrong name ;-)
You've put them in the part of the file used for the PCI Express capability,
and named them as if they're part of the PCI Express capability.
> /* Extended Capabilities (PCI-X 2.0 and Express) */
> #define PCI_EXT_CAP_ID(header) (header & 0x0000ffff)
> #define PCI_EXT_CAP_VER(header) ((header >> 16) & 0xf)
> @@ -501,6 +509,7 @@
> #define PCI_EXT_CAP_ID_VC 2
> #define PCI_EXT_CAP_ID_DSN 3
> #define PCI_EXT_CAP_ID_PWR 4
> +#define PCI_EXT_CAP_ID_ACS 13
> #define PCI_EXT_CAP_ID_ARI 14
> #define PCI_EXT_CAP_ID_ATS 15
> #define PCI_EXT_CAP_ID_SRIOV 16
> @@ -661,4 +670,9 @@
> #define PCI_SRIOV_VFM_MO 0x2 /* Active.MigrateOut */
> #define PCI_SRIOV_VFM_AV 0x3 /* Active.Available */
>
> +/* Access Control Service */
> +#define PCI_ACS_CAP 0x04 /* ACS Capability Register */
They should be down here instead and named like this:
+#define PCI_ACS_SV 0x01 /* Source Validation */
+#define PCI_ACS_TB 0x02 /* Translation Blocking */
+#define PCI_ACS_RR 0x04 /* P2P Request Redirect */
+#define PCI_ACS_CR 0x08 /* P2P Completion Redirect */
+#define PCI_ACS_UF 0x10 /* Upstream Forwarding */
+#define PCI_ACS_EC 0x20 /* P2P Egress Control */
+#define PCI_ACS_DT 0x40 /* Direct Translated P2P */
Now ACS_ENABLED fits on one line again ;-)
#define ACS_ENABLED (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
> +#define PCI_ACS_CTRL 0x06 /* ACS Control Register */
> +#define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */
> +
> #endif /* LINUX_PCI_REGS_H */
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."