RE: [PATCH] enabling ACS P2P upstream forwarding
From: Kay, Allen M
Date: Wed Sep 16 2009 - 22:14:26 EST
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:matthew@xxxxxx]
Sent: Wednesday, September 16, 2009 2:58 AM
To: Kay, Allen M
Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; jbarnes@xxxxxxxxxxxxxxxx
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/