diff mbox

x86: PCI config space accessor functions should not ignore the segment argument

Message ID 4E293F21020000780004F194@nat28.tlf.novell.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Jan Beulich July 22, 2011, 7:13 a.m. UTC 
Without this change, the majority of the raw PCI config space access
functions silently ignore a non-zero segment argument, which is
certainly wrong.

Apart from pci_direct_conf1, all other non-MMCFG access methods get
used only for non-extended accesses (i.e. assigned to raw_pci_ops
only). Consequently, with the way raw_pci_{read,write}() work, it would
be a coding error to call these functions with a non-zero segment (with
the current call flow this cannot happen afaict).

The access method 1 accessor, as it can be used for extended accesses
(on AMD systems) instead gets checks added for the passed in segment to
be zero. This would be the case when on such a system having multiple
PCI segments (don't know whether any exist in practice) MMCFG for some
reason is not usable, and method 1 gets selected for doing extended
accesses. Rather than accessing the wrong device's config space, the
function will now error out.

v2: Convert BUG_ON() to WARN_ON(), and extend description as per Ingo's
request.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Ingo Molnar <mingo@elte.hu>

---
 arch/x86/pci/ce4100.c   |    2 ++
 arch/x86/pci/direct.c   |    6 ++++--
 arch/x86/pci/numaq_32.c |    2 ++
 arch/x86/pci/olpc.c     |    4 ++++
 arch/x86/pci/pcbios.c   |    2 ++
 5 files changed, 14 insertions(+), 2 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ingo Molnar July 22, 2011, 8:25 a.m. UTC | #1 
* Jan Beulich <JBeulich@novell.com> wrote:

> Without this change, the majority of the raw PCI config space access
> functions silently ignore a non-zero segment argument, which is
> certainly wrong.
> 
> Apart from pci_direct_conf1, all other non-MMCFG access methods get
> used only for non-extended accesses (i.e. assigned to raw_pci_ops
> only). Consequently, with the way raw_pci_{read,write}() work, it would
> be a coding error to call these functions with a non-zero segment (with
> the current call flow this cannot happen afaict).
> 
> The access method 1 accessor, as it can be used for extended accesses
> (on AMD systems) instead gets checks added for the passed in segment to
> be zero. This would be the case when on such a system having multiple
> PCI segments (don't know whether any exist in practice) MMCFG for some
> reason is not usable, and method 1 gets selected for doing extended
> accesses. Rather than accessing the wrong device's config space, the
> function will now error out.
> 
> v2: Convert BUG_ON() to WARN_ON(), and extend description as per Ingo's
> request.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Ingo Molnar <mingo@elte.hu>

Thanks Jan!

Reviewed-by: Ingo Molnar <mingo@elte.hu>

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Barnes July 22, 2011, 3:23 p.m. UTC | #2 
On Fri, 22 Jul 2011 08:13:05 +0100
"Jan Beulich" <JBeulich@novell.com> wrote:

> Without this change, the majority of the raw PCI config space access
> functions silently ignore a non-zero segment argument, which is
> certainly wrong.
> 
> Apart from pci_direct_conf1, all other non-MMCFG access methods get
> used only for non-extended accesses (i.e. assigned to raw_pci_ops
> only). Consequently, with the way raw_pci_{read,write}() work, it would
> be a coding error to call these functions with a non-zero segment (with
> the current call flow this cannot happen afaict).
> 
> The access method 1 accessor, as it can be used for extended accesses
> (on AMD systems) instead gets checks added for the passed in segment to
> be zero. This would be the case when on such a system having multiple
> PCI segments (don't know whether any exist in practice) MMCFG for some
> reason is not usable, and method 1 gets selected for doing extended
> accesses. Rather than accessing the wrong device's config space, the
> function will now error out.
> 
> v2: Convert BUG_ON() to WARN_ON(), and extend description as per Ingo's
> request.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> 

Applied, thanks.
diff mbox

Patch

--- 3.0/arch/x86/pci/ce4100.c
+++ 3.0-x86-pci-access-seg/arch/x86/pci/ce4100.c
@@ -257,6 +257,7 @@  static int ce4100_conf_read(unsigned int
 {
 	int i;
 
+	WARN_ON(seg);
 	if (bus == 1) {
 		for (i = 0; i < ARRAY_SIZE(bus1_fixups); i++) {
 			if (bus1_fixups[i].dev_func == devfn &&
@@ -282,6 +283,7 @@  static int ce4100_conf_write(unsigned in
 {
 	int i;
 
+	WARN_ON(seg);
 	if (bus == 1) {
 		for (i = 0; i < ARRAY_SIZE(bus1_fixups); i++) {
 			if (bus1_fixups[i].dev_func == devfn &&
--- 3.0/arch/x86/pci/direct.c
+++ 3.0-x86-pci-access-seg/arch/x86/pci/direct.c
@@ -22,7 +22,7 @@  static int pci_conf1_read(unsigned int s
 {
 	unsigned long flags;
 
-	if ((bus > 255) || (devfn > 255) || (reg > 4095)) {
+	if (seg || (bus > 255) || (devfn > 255) || (reg > 4095)) {
 		*value = -1;
 		return -EINVAL;
 	}
@@ -53,7 +53,7 @@  static int pci_conf1_write(unsigned int 
 {
 	unsigned long flags;
 
-	if ((bus > 255) || (devfn > 255) || (reg > 4095))
+	if (seg || (bus > 255) || (devfn > 255) || (reg > 4095))
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&pci_config_lock, flags);
@@ -97,6 +97,7 @@  static int pci_conf2_read(unsigned int s
 	unsigned long flags;
 	int dev, fn;
 
+	WARN_ON(seg);
 	if ((bus > 255) || (devfn > 255) || (reg > 255)) {
 		*value = -1;
 		return -EINVAL;
@@ -138,6 +139,7 @@  static int pci_conf2_write(unsigned int 
 	unsigned long flags;
 	int dev, fn;
 
+	WARN_ON(seg);
 	if ((bus > 255) || (devfn > 255) || (reg > 255)) 
 		return -EINVAL;
 
--- 3.0/arch/x86/pci/numaq_32.c
+++ 3.0-x86-pci-access-seg/arch/x86/pci/numaq_32.c
@@ -34,6 +34,7 @@  static int pci_conf1_mq_read(unsigned in
 	unsigned long flags;
 	void *adr __iomem = XQUAD_PORT_ADDR(0xcfc, BUS2QUAD(bus));
 
+	WARN_ON(seg);
 	if (!value || (bus >= MAX_MP_BUSSES) || (devfn > 255) || (reg > 255))
 		return -EINVAL;
 
@@ -73,6 +74,7 @@  static int pci_conf1_mq_write(unsigned i
 	unsigned long flags;
 	void *adr __iomem = XQUAD_PORT_ADDR(0xcfc, BUS2QUAD(bus));
 
+	WARN_ON(seg);
 	if ((bus >= MAX_MP_BUSSES) || (devfn > 255) || (reg > 255)) 
 		return -EINVAL;
 
--- 3.0/arch/x86/pci/olpc.c
+++ 3.0-x86-pci-access-seg/arch/x86/pci/olpc.c
@@ -206,6 +206,8 @@  static int pci_olpc_read(unsigned int se
 {
 	uint32_t *addr;
 
+	WARN_ON(seg);
+
 	/* Use the hardware mechanism for non-simulated devices */
 	if (!is_simulated(bus, devfn))
 		return pci_direct_conf1.read(seg, bus, devfn, reg, len, value);
@@ -264,6 +266,8 @@  static int pci_olpc_read(unsigned int se
 static int pci_olpc_write(unsigned int seg, unsigned int bus,
 		unsigned int devfn, int reg, int len, uint32_t value)
 {
+	WARN_ON(seg);
+
 	/* Use the hardware mechanism for non-simulated devices */
 	if (!is_simulated(bus, devfn))
 		return pci_direct_conf1.write(seg, bus, devfn, reg, len, value);
--- 3.0/arch/x86/pci/pcbios.c
+++ 3.0-x86-pci-access-seg/arch/x86/pci/pcbios.c
@@ -181,6 +181,7 @@  static int pci_bios_read(unsigned int se
 	unsigned long flags;
 	unsigned long bx = (bus << 8) | devfn;
 
+	WARN_ON(seg);
 	if (!value || (bus > 255) || (devfn > 255) || (reg > 255))
 		return -EINVAL;
 
@@ -247,6 +248,7 @@  static int pci_bios_write(unsigned int s
 	unsigned long flags;
 	unsigned long bx = (bus << 8) | devfn;
 
+	WARN_ON(seg);
 	if ((bus > 255) || (devfn > 255) || (reg > 255)) 
 		return -EINVAL;