From: "Kay, Allen M" <allen.m.kay@intel.com>
To: Matthew Wilcox <matthew@wil.cx>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
       "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
       "jbarnes@virtuousgeek.org" <jbarnes@virtuousgeek.org>
Date: Wed, 16 Sep 2009 19:14:15 -0700
Subject: RE: [PATCH] enabling ACS P2P upstream forwarding
Thread-Topic: [PATCH] enabling ACS P2P upstream forwarding
Thread-Index: Aco2tCRMP1fYDeiTQTaTjeI9s1+txwAiAHUw
Message-ID: <57C9024A16AD2D4C97DC78E552063EA3E064029D@orsmsx505.amr.corp.intel.com>
References: <57C9024A16AD2D4C97DC78E552063EA3E05DC218@orsmsx505.amr.corp.intel.com>
 <20090916095741.GK29320@parisc-linux.org>
In-Reply-To: <20090916095741.GK29320@parisc-linux.org>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6114
Lines: 182

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@wil.cx] 
Sent: Wednesday, September 16, 2009 2:58 AM
To: Kay, Allen M
Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; jbarnes@virtuousgeek.org
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/