[PATCH] msi: set 'En' bit of MSI Mapping Capability

[PATCH] msi: set 'En' bit of MSI Mapping Capability

[peerchen]

According to the HyperTransport spec, 'En' indicate if the MSI Mapping is active.
Set the 'En' bit when setup pci and add the quirk for some nvidia devices. 

The patch base on kernel 2.6.24-rc5

Signed-off-by: Andy Currid <acurrid@nvidia.com>
Signed-off-by: Peer Chen <pchen@nvidia.com>

---
diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/drivers/pci/probe.c linux-2.6.24-rc5/drivers/pci/probe.c
--- linux-2.6.24-rc5-vanilla/drivers/pci/probe.c	2007-12-18 14:35:46.000000000 -0500
+++ linux-2.6.24-rc5/drivers/pci/probe.c	2007-12-18 16:28:29.000000000 -0500
@@ -721,6 +721,9 @@ static int pci_setup_device(struct pci_d
 
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
+	
+	/* Enable HT MSI mapping */
+	ht_enable_msi_mapping(dev);
 
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c linux-2.6.24-rc5/drivers/pci/quirks.c
--- linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c	2007-12-18 14:35:46.000000000 -0500
+++ linux-2.6.24-rc5/drivers/pci/quirks.c	2007-12-18 16:28:41.000000000 -0500
@@ -1705,6 +1705,45 @@ static void __devinit quirk_nvidia_ck804
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
 			quirk_nvidia_ck804_msi_ht_cap);
 
+static void __devinit quirk_msi_ht_cap_disable(struct pci_dev *dev) {
+	struct pci_dev *host_bridge;
+	int pos, ttl = 48;
+
+	/* HT MSI mapping should be disabled on devices that are below
+	 * a non-Hypertransport host bridge. Locate the host bridge...
+ 	 */
+
+	if ((host_bridge = pci_get_bus_and_slot(0, PCI_DEVFN(0,0))) == NULL) {
+		printk(KERN_WARNING
+			 "PCI: quirk_msi_ht_cap_disable didn't locate host bridge\n");
+		return;
+	}
+
+	if ((pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE)) != 0) {
+		/* Host bridge is to HT */
+		return;
+	}
+
+	/* Host bridge is not to HT, disable HT MSI mapping on this device */
+
+	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
+	while (pos && ttl--) {
+		u8 flags;
+
+		if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
+			printk(KERN_INFO "PCI: Quirk disabling HT MSI mapping on %s\n",
+			       pci_name(dev));
+
+			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
+					      flags & ~HT_MSI_FLAGS_ENABLE);
+		}
+		pos = pci_find_next_ht_capability(dev, pos,
+						  HT_CAPTYPE_MSI_MAPPING);
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+							quirk_msi_ht_cap_disable);
+
 static void __devinit quirk_msi_intx_disable_bug(struct pci_dev *dev)
 {
 	dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h linux-2.6.24-rc5/include/asm-generic/pci.h
--- linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h	2007-12-18 14:35:52.000000000 -0500
+++ linux-2.6.24-rc5/include/asm-generic/pci.h	2007-12-18 16:29:12.000000000 -0500
@@ -45,6 +45,10 @@ pcibios_select_root(struct pci_dev *pdev
 
 #define pcibios_scan_all_fns(a, b)	0
 
+#ifndef HAVE_ARCH_HT_ENABLE_MSI_MAPPING
+#define ht_enable_msi_mapping(a)	0
+#endif /* HAVE_ARCH_HT_ENABLE_MSI_MAPPING */
+
 #ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
 static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
 {
diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h linux-2.6.24-rc5/include/asm-x86/pci.h
--- linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h	2007-12-18 14:35:51.000000000 -0500
+++ linux-2.6.24-rc5/include/asm-x86/pci.h	2007-12-18 16:28:58.000000000 -0500
@@ -75,6 +75,27 @@ static inline void pci_dma_burst_advice(
 }
 #endif
 
+#ifdef CONFIG_PCI
+#define HAVE_ARCH_HT_ENABLE_MSI_MAPPING
+static inline void ht_enable_msi_mapping(struct pci_dev *dev)
+{
+	int pos, ttl = 48;
+
+	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
+	while (pos && ttl--) {
+		u8 flags;
+
+		if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
+			printk(KERN_INFO "PCI: Enabling HT MSI Mapping on %s\n",
+						dev->dev.bus_id);
+
+			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
+					      flags | HT_MSI_FLAGS_ENABLE);
+		}
+		pos = pci_find_next_ht_capability(dev, pos, HT_CAPTYPE_MSI_MAPPING);
+	}
+}
+#endif
 
 #endif  /* __KERNEL__ */
-

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Eric W. Biederman]

"peerchen" <peerchen@gmail.com> writes:

> According to the HyperTransport spec, 'En' indicate if the MSI Mapping is
> active.
> Set the 'En' bit when setup pci and add the quirk for some nvidia devices. 
>
> The patch base on kernel 2.6.24-rc5

Ok.  This is starting to look good.

> Signed-off-by: Andy Currid <acurrid@nvidia.com>
> Signed-off-by: Peer Chen <pchen@nvidia.com>
>
> ---
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/drivers/pci/probe.c
> linux-2.6.24-rc5/drivers/pci/probe.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/probe.c 2007-12-18 14:35:46.000000000
> -0500
> +++ linux-2.6.24-rc5/drivers/pci/probe.c 2007-12-18 16:28:29.000000000 -0500
> @@ -721,6 +721,9 @@ static int pci_setup_device(struct pci_d
>  
>  	/* "Unknown power state" */
>  	dev->current_state = PCI_UNKNOWN;
> +	
> +	/* Enable HT MSI mapping */
> +	ht_enable_msi_mapping(dev);
>  
>  	/* Early fixups, before probing the BARs */
>  	pci_fixup_device(pci_fixup_early, dev);
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c
> linux-2.6.24-rc5/drivers/pci/quirks.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c 2007-12-18 14:35:46.000000000
> -0500
> +++ linux-2.6.24-rc5/drivers/pci/quirks.c 2007-12-18 16:28:41.000000000 -0500
> @@ -1705,6 +1705,45 @@ static void __devinit quirk_nvidia_ck804
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
>  			quirk_nvidia_ck804_msi_ht_cap);
>  
> +static void __devinit quirk_msi_ht_cap_disable(struct pci_dev *dev) {
> +	struct pci_dev *host_bridge;
> +	int pos, ttl = 48;
> +
> +	/* HT MSI mapping should be disabled on devices that are below
> +	 * a non-Hypertransport host bridge. Locate the host bridge...
> + 	 */
> +
> +	if ((host_bridge = pci_get_bus_and_slot(0, PCI_DEVFN(0,0))) == NULL) {
> +		printk(KERN_WARNING
> + "PCI: quirk_msi_ht_cap_disable didn't locate host bridge\n");
> +		return;
> +	}
> +
> + if ((pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE)) != 0) {
> +		/* Host bridge is to HT */
> +		return;
> +	}
> +
> +	/* Host bridge is not to HT, disable HT MSI mapping on this device */
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> + if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> + printk(KERN_INFO "PCI: Quirk disabling HT MSI mapping on %s\n",
> +			       pci_name(dev));
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags & ~HT_MSI_FLAGS_ENABLE);
> +		}
> +		pos = pci_find_next_ht_capability(dev, pos,
> +						  HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> + quirk_msi_ht_cap_disable);

Could you explain the need for this quirk?

My expectation would be that if we turned an MSI interrupt into a
hypertransport interrupt then if we hit a non-hyptransport bridge
upstream it would just turn the hypertransport interrupts into
whatever makes sense for the upstream bridge.  I can see it being
excess work and adding some latency but I don't see it being a
correctness problem. 

Or are my expectations off and the NVIDIA chipsets do not handle
hypertransport interrupts the way I would expect.  Dropping them
instead of converting when going to a non-hypertransport host bridge.


>  static void __devinit quirk_msi_intx_disable_bug(struct pci_dev *dev)
>  {
>  	dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h
> linux-2.6.24-rc5/include/asm-generic/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h 2007-12-18
> 14:35:52.000000000 -0500
> +++ linux-2.6.24-rc5/include/asm-generic/pci.h 2007-12-18 16:29:12.000000000
> -0500
> @@ -45,6 +45,10 @@ pcibios_select_root(struct pci_dev *pdev
>  
>  #define pcibios_scan_all_fns(a, b)	0
>  
> +#ifndef HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +#define ht_enable_msi_mapping(a)	0
> +#endif /* HAVE_ARCH_HT_ENABLE_MSI_MAPPING */
> +
>  #ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
>  static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>  {
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h
> linux-2.6.24-rc5/include/asm-x86/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h 2007-12-18 14:35:51.000000000
> -0500
> +++ linux-2.6.24-rc5/include/asm-x86/pci.h 2007-12-18 16:28:58.000000000 -0500
> @@ -75,6 +75,27 @@ static inline void pci_dma_burst_advice(
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI
> +#define HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +static inline void ht_enable_msi_mapping(struct pci_dev *dev)
> +{
> +	int pos, ttl = 48;
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> + if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> + printk(KERN_INFO "PCI: Enabling HT MSI Mapping on %s\n",
> +						dev->dev.bus_id);
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags | HT_MSI_FLAGS_ENABLE);

Here I see two things that we should do better.
1) If the BIOS has already properly enabled the capability we don't
   need to do anything or print any message.  i.e.
   if (!(flags & HT_MSI_FLAGS_ENABLE)) {
	printk(...)
        pci_write_config_byte()
   }

2) The hypertransport specification allows for an optional register
   that specifies the address of the msi mapping window.  If that
   register is present we should program it to the architectural
   defined address on x86 before enabling the device.
        
   
> +		}
> + pos = pci_find_next_ht_capability(dev, pos, HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +#endif
>  
>  #endif  /* __KERNEL__ */
> -

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Andrew Morton]

On Tue, 18 Dec 2007 23:00:52 +0800
"peerchen" <peerchen@gmail.com> wrote:

> According to the HyperTransport spec, 'En' indicate if the MSI Mapping is active.
> Set the 'En' bit when setup pci and add the quirk for some nvidia devices. 
> 
> The patch base on kernel 2.6.24-rc5
> 
> Signed-off-by: Andy Currid <acurrid@nvidia.com>
> Signed-off-by: Peer Chen <pchen@nvidia.com>
> 
> ---
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/drivers/pci/probe.c linux-2.6.24-rc5/drivers/pci/probe.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/probe.c	2007-12-18 14:35:46.000000000 -0500
> +++ linux-2.6.24-rc5/drivers/pci/probe.c	2007-12-18 16:28:29.000000000 -0500
> @@ -721,6 +721,9 @@ static int pci_setup_device(struct pci_d
>  
>  	/* "Unknown power state" */
>  	dev->current_state = PCI_UNKNOWN;
> +	
> +	/* Enable HT MSI mapping */
> +	ht_enable_msi_mapping(dev);
>  
>  	/* Early fixups, before probing the BARs */
>  	pci_fixup_device(pci_fixup_early, dev);
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c linux-2.6.24-rc5/drivers/pci/quirks.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c	2007-12-18 14:35:46.000000000 -0500
> +++ linux-2.6.24-rc5/drivers/pci/quirks.c	2007-12-18 16:28:41.000000000 -0500
> @@ -1705,6 +1705,45 @@ static void __devinit quirk_nvidia_ck804
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
>  			quirk_nvidia_ck804_msi_ht_cap);
>  
> +static void __devinit quirk_msi_ht_cap_disable(struct pci_dev *dev) {

Please feed all diffs through scripts/checkpatch.pl and review the results.


> +	struct pci_dev *host_bridge;
> +	int pos, ttl = 48;
> +
> +	/* HT MSI mapping should be disabled on devices that are below
> +	 * a non-Hypertransport host bridge. Locate the host bridge...
> + 	 */
> +
> +	if ((host_bridge = pci_get_bus_and_slot(0, PCI_DEVFN(0,0))) == NULL) {
> +		printk(KERN_WARNING
> +			 "PCI: quirk_msi_ht_cap_disable didn't locate host bridge\n");
> +		return;
> +	}
> +
> +	if ((pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE)) != 0) {
> +		/* Host bridge is to HT */
> +		return;
> +	}
> +
> +	/* Host bridge is not to HT, disable HT MSI mapping on this device */
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> +		if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> +			printk(KERN_INFO "PCI: Quirk disabling HT MSI mapping on %s\n",
> +			       pci_name(dev));
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags & ~HT_MSI_FLAGS_ENABLE);
> +		}
> +		pos = pci_find_next_ht_capability(dev, pos,
> +						  HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> +							quirk_msi_ht_cap_disable);

This limit of 48 iterations (ttl) is mysterious.  Perhaps it is described
in the spec?  Either way, I believe it should be documented in code
comments because there is no way on earth that the code reader can tell why
it is there.

>  static void __devinit quirk_msi_intx_disable_bug(struct pci_dev *dev)
>  {
>  	dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h linux-2.6.24-rc5/include/asm-generic/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h	2007-12-18 14:35:52.000000000 -0500
> +++ linux-2.6.24-rc5/include/asm-generic/pci.h	2007-12-18 16:29:12.000000000 -0500
> @@ -45,6 +45,10 @@ pcibios_select_root(struct pci_dev *pdev
>  
>  #define pcibios_scan_all_fns(a, b)	0
>  
> +#ifndef HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +#define ht_enable_msi_mapping(a)	0
> +#endif /* HAVE_ARCH_HT_ENABLE_MSI_MAPPING */

Please try to avoid the HAVE_ARCH_foo trick.  It's fairly ugly.  You can do
the Linus trick of:

#ifndef ht_enable_msi_mapping
#define ht_enable_msi_mapping(x) do { } while (0)
#endif

and then, in include/asm-x86/pci.h, do

static inline void ht_enable_msi_mapping(struct pci_dev *dev)
{
	...
}

#define ht_enable_msi_mapping(d) ht_enable_msi_mapping(d)

which has the merit of reducing the number of global symbols, but it's
still pretty unpleasant.


It would be better to just add a stub implementation of
ht_enable_msi_mapping() for all the other architectures - avoid fancy cpp
tricks.

Also, your proposed non-x86 implementation of ht_enable_msi_mapping() is
(effectively) an integer-returning thing whereas your x86 implementation is
a void-returning thing.  Consequently non-x86 architectures will get a
statement-with-no-effect warning when this patch is applied.


>  #ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
>  static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>  {
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h linux-2.6.24-rc5/include/asm-x86/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h	2007-12-18 14:35:51.000000000 -0500
> +++ linux-2.6.24-rc5/include/asm-x86/pci.h	2007-12-18 16:28:58.000000000 -0500
> @@ -75,6 +75,27 @@ static inline void pci_dma_burst_advice(
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI
> +#define HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +static inline void ht_enable_msi_mapping(struct pci_dev *dev)
> +{
> +	int pos, ttl = 48;
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> +		if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> +			printk(KERN_INFO "PCI: Enabling HT MSI Mapping on %s\n",
> +						dev->dev.bus_id);
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags | HT_MSI_FLAGS_ENABLE);
> +		}
> +		pos = pci_find_next_ht_capability(dev, pos, HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +#endif

And even though this has only a single callsite, there really is no point
in inlining it.  It's not a fastpath and putting a large and complex
function like this in a header file risks increasing that header file's
include dependencies.

iow, let's put it in arch/x86/pci/ somewhere?

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Andrew Morton]

On Wed, 19 Dec 2007 16:41:25 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> It would be better to just add a stub implementation of
> ht_enable_msi_mapping() for all the other architectures - avoid fancy cpp
> tricks.


And by this I really do mean going into each include/asm-*/pci.h and adding


struct pci_dev;		(if needed)
...

static inline void ht_enable_msi_mapping(struct pci_dev *dev)
{
}


no macros, no ifdef tricks, no include tricks.  Just straight, clean, fully
typechecked C.

RE: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Peer Chen]

The quirk is for our Intel platform, we don't want HT MSI mapping
enabled in any of our devices.

BRs
Peer Chen

-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: Wednesday, December 19, 2007 5:59 AM
To: peerchen
Cc: linux-kernel; akpm; Andy Currid; Peer Chen
Subject: Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

"peerchen" <peerchen@gmail.com> writes:

> According to the HyperTransport spec, 'En' indicate if the MSI Mapping
is
> active.
> Set the 'En' bit when setup pci and add the quirk for some nvidia
devices. 
>
> The patch base on kernel 2.6.24-rc5

Ok.  This is starting to look good.

> Signed-off-by: Andy Currid <acurrid@nvidia.com>
> Signed-off-by: Peer Chen <pchen@nvidia.com>
>
> ---
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/drivers/pci/probe.c
> linux-2.6.24-rc5/drivers/pci/probe.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/probe.c 2007-12-18
14:35:46.000000000
> -0500
> +++ linux-2.6.24-rc5/drivers/pci/probe.c 2007-12-18 16:28:29.000000000
-0500
> @@ -721,6 +721,9 @@ static int pci_setup_device(struct pci_d
>  
>  	/* "Unknown power state" */
>  	dev->current_state = PCI_UNKNOWN;
> +	
> +	/* Enable HT MSI mapping */
> +	ht_enable_msi_mapping(dev);
>  
>  	/* Early fixups, before probing the BARs */
>  	pci_fixup_device(pci_fixup_early, dev);
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c
> linux-2.6.24-rc5/drivers/pci/quirks.c
> --- linux-2.6.24-rc5-vanilla/drivers/pci/quirks.c 2007-12-18
14:35:46.000000000
> -0500
> +++ linux-2.6.24-rc5/drivers/pci/quirks.c 2007-12-18
16:28:41.000000000 -0500
> @@ -1705,6 +1705,45 @@ static void __devinit quirk_nvidia_ck804
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA,
PCI_DEVICE_ID_NVIDIA_CK804_PCIE,
>  			quirk_nvidia_ck804_msi_ht_cap);
>  
> +static void __devinit quirk_msi_ht_cap_disable(struct pci_dev *dev) {
> +	struct pci_dev *host_bridge;
> +	int pos, ttl = 48;
> +
> +	/* HT MSI mapping should be disabled on devices that are below
> +	 * a non-Hypertransport host bridge. Locate the host bridge...
> + 	 */
> +
> +	if ((host_bridge = pci_get_bus_and_slot(0, PCI_DEVFN(0,0))) ==
NULL) {
> +		printk(KERN_WARNING
> + "PCI: quirk_msi_ht_cap_disable didn't locate host bridge\n");
> +		return;
> +	}
> +
> + if ((pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE)) !=
0) {
> +		/* Host bridge is to HT */
> +		return;
> +	}
> +
> +	/* Host bridge is not to HT, disable HT MSI mapping on this
device */
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> + if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> + printk(KERN_INFO "PCI: Quirk disabling HT MSI mapping on %s\n",
> +			       pci_name(dev));
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags &
~HT_MSI_FLAGS_ENABLE);
> +		}
> +		pos = pci_find_next_ht_capability(dev, pos,
> +
HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> + quirk_msi_ht_cap_disable);

Could you explain the need for this quirk?

My expectation would be that if we turned an MSI interrupt into a
hypertransport interrupt then if we hit a non-hyptransport bridge
upstream it would just turn the hypertransport interrupts into
whatever makes sense for the upstream bridge.  I can see it being
excess work and adding some latency but I don't see it being a
correctness problem. 

Or are my expectations off and the NVIDIA chipsets do not handle
hypertransport interrupts the way I would expect.  Dropping them
instead of converting when going to a non-hypertransport host bridge.


>  static void __devinit quirk_msi_intx_disable_bug(struct pci_dev *dev)
>  {
>  	dev->dev_flags |= PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG;
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h
> linux-2.6.24-rc5/include/asm-generic/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-generic/pci.h 2007-12-18
> 14:35:52.000000000 -0500
> +++ linux-2.6.24-rc5/include/asm-generic/pci.h 2007-12-18
16:29:12.000000000
> -0500
> @@ -45,6 +45,10 @@ pcibios_select_root(struct pci_dev *pdev
>  
>  #define pcibios_scan_all_fns(a, b)	0
>  
> +#ifndef HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +#define ht_enable_msi_mapping(a)	0
> +#endif /* HAVE_ARCH_HT_ENABLE_MSI_MAPPING */
> +
>  #ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
>  static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int
channel)
>  {
> diff -uprN -X linux-2.6.24-rc5-vanilla/Documentation/dontdiff
> linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h
> linux-2.6.24-rc5/include/asm-x86/pci.h
> --- linux-2.6.24-rc5-vanilla/include/asm-x86/pci.h 2007-12-18
14:35:51.000000000
> -0500
> +++ linux-2.6.24-rc5/include/asm-x86/pci.h 2007-12-18
16:28:58.000000000 -0500
> @@ -75,6 +75,27 @@ static inline void pci_dma_burst_advice(
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI
> +#define HAVE_ARCH_HT_ENABLE_MSI_MAPPING
> +static inline void ht_enable_msi_mapping(struct pci_dev *dev)
> +{
> +	int pos, ttl = 48;
> +
> +	pos = pci_find_ht_capability(dev, HT_CAPTYPE_MSI_MAPPING);
> +	while (pos && ttl--) {
> +		u8 flags;
> +
> + if (pci_read_config_byte(dev, pos + HT_MSI_FLAGS, &flags) == 0) {
> + printk(KERN_INFO "PCI: Enabling HT MSI Mapping on %s\n",
> +						dev->dev.bus_id);
> +
> +			pci_write_config_byte(dev, pos + HT_MSI_FLAGS,
> +					      flags |
HT_MSI_FLAGS_ENABLE);

Here I see two things that we should do better.
1) If the BIOS has already properly enabled the capability we don't
   need to do anything or print any message.  i.e.
   if (!(flags & HT_MSI_FLAGS_ENABLE)) {
	printk(...)
        pci_write_config_byte()
   }

2) The hypertransport specification allows for an optional register
   that specifies the address of the msi mapping window.  If that
   register is present we should program it to the architectural
   defined address on x86 before enabling the device.
        
   
> +		}
> + pos = pci_find_next_ht_capability(dev, pos, HT_CAPTYPE_MSI_MAPPING);
> +	}
> +}
> +#endif
>  
>  #endif  /* __KERNEL__ */
> -
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Eric W. Biederman]

"Peer Chen" <pchen@nvidia.com> writes:

> The quirk is for our Intel platform, we don't want HT MSI mapping
> enabled in any of our devices.

Why is this a problem?  I seem to recall a real hypertransport bus
downstream of the Intel cpu.

If there is a real hypertransport bus in the middle then what happens
if someone puts a tunnel that uses hypertransport between the two chips?

I feel very dense right now.  I don't understand why enabling the
mapping on an Intel based system is a problem.  I am afraid there is
some important gap in my understanding in which case we may need to
rethink enabling the hypertransport capability by default.

If disabling the hypertransport bus is simply an optimization, or it
is to deal with an issue that appears exclusive to Nvidia chips
I have no problem with your quirk.

Eric

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Wed, 19 Dec 2007 16:41:25 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> It would be better to just add a stub implementation of
>> ht_enable_msi_mapping() for all the other architectures - avoid fancy cpp
>> tricks.
>
>
> And by this I really do mean going into each include/asm-*/pci.h and adding
>
>
> struct pci_dev;		(if needed)
> ...
>
> static inline void ht_enable_msi_mapping(struct pci_dev *dev)
> {
> }
>
>
> no macros, no ifdef tricks, no include tricks.  Just straight, clean, fully
> typechecked C.

Andrew thanks for the code style review.  I goofed about recommending the
ARCH_HAVE_XXXX thing.

I'm going to concentrate on the content for the moment.  I think we
are very close to a general solution to a very common problem with
MSI interrupts.

Eric

RE: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Peer Chen]

I feel it's dangerous to set the En bit on Intel platform, If the HT MSI
En is set, the MSI should be expected to transform to HT INT message
format. It may cause interrupt lost or hardware internal state machine
failed depend on the hardware design.

BRs
Peer Chen

-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: Friday, December 21, 2007 6:48 AM
To: Peer Chen
Cc: peerchen; linux-kernel; akpm; Andy Currid
Subject: Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

"Peer Chen" <pchen@nvidia.com> writes:

> The quirk is for our Intel platform, we don't want HT MSI mapping
> enabled in any of our devices.

Why is this a problem?  I seem to recall a real hypertransport bus
downstream of the Intel cpu.

If there is a real hypertransport bus in the middle then what happens
if someone puts a tunnel that uses hypertransport between the two chips?

I feel very dense right now.  I don't understand why enabling the
mapping on an Intel based system is a problem.  I am afraid there is
some important gap in my understanding in which case we may need to
rethink enabling the hypertransport capability by default.

If disabling the hypertransport bus is simply an optimization, or it
is to deal with an issue that appears exclusive to Nvidia chips
I have no problem with your quirk.

Eric
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Eric W. Biederman]

"Peer Chen" <pchen@nvidia.com> writes:

> I feel it's dangerous to set the En bit on Intel platform, If the HT MSI
> En is set, the MSI should be expected to transform to HT INT message
> format. It may cause interrupt lost or hardware internal state machine
> failed depend on the hardware design.

Reasonable.  As long as what the quirk is to avoid errata and
chipset specific issues I don't have a problem with it.

I figure the quirk should be a separate patch though.

My concern is that the general rule that always enabling HT MSI
mapping capabilities is reasonable.  Even if there are some
specific exceptions where we don't want to do that.

I want code that requires the smallest list of chipset
exceptions that we can make. 

Eric

RE: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Peer Chen]

I think it's more reasonable to only apply this rule onto AMD platform.

BRs
Peer Chen

-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: Monday, December 24, 2007 7:49 PM
To: Peer Chen
Cc: peerchen; linux-kernel; akpm; Andy Currid
Subject: Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

"Peer Chen" <pchen@nvidia.com> writes:

> I feel it's dangerous to set the En bit on Intel platform, If the HT
MSI
> En is set, the MSI should be expected to transform to HT INT message
> format. It may cause interrupt lost or hardware internal state machine
> failed depend on the hardware design.

Reasonable.  As long as what the quirk is to avoid errata and
chipset specific issues I don't have a problem with it.

I figure the quirk should be a separate patch though.

My concern is that the general rule that always enabling HT MSI
mapping capabilities is reasonable.  Even if there are some
specific exceptions where we don't want to do that.

I want code that requires the smallest list of chipset
exceptions that we can make. 

Eric
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

RE: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Peer Chen]

Eric,
Any decision for this patch, if not, currently we prefer to add all our
code to quirks.c.

BRs
Peer Chen

-----Original Message-----
From: Peer Chen 
Sent: Wednesday, January 02, 2008 5:58 PM
To: 'Eric W. Biederman'
Cc: peerchen; linux-kernel; akpm; Andy Currid
Subject: RE: [PATCH] msi: set 'En' bit of MSI Mapping Capability

I think it's more reasonable to only apply this rule onto AMD platform.

BRs
Peer Chen

-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: Monday, December 24, 2007 7:49 PM
To: Peer Chen
Cc: peerchen; linux-kernel; akpm; Andy Currid
Subject: Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

"Peer Chen" <pchen@nvidia.com> writes:

> I feel it's dangerous to set the En bit on Intel platform, If the HT
MSI
> En is set, the MSI should be expected to transform to HT INT message
> format. It may cause interrupt lost or hardware internal state machine
> failed depend on the hardware design.

Reasonable.  As long as what the quirk is to avoid errata and
chipset specific issues I don't have a problem with it.

I figure the quirk should be a separate patch though.

My concern is that the general rule that always enabling HT MSI
mapping capabilities is reasonable.  Even if there are some
specific exceptions where we don't want to do that.

I want code that requires the smallest list of chipset
exceptions that we can make. 

Eric
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] msi: set 'En' bit of MSI Mapping Capability

[Eric W. Biederman]

"Peer Chen" <pchen@nvidia.com> writes:

> Eric,
> Any decision for this patch, if not, currently we prefer to add all our
> code to quirks.c.

Sorry.  I think adding the code to quirks.c is fine.

For bisection and code inspection purposes I would prefer the code
to come as a patchset of two patches.  With the generic change
first.

I wanted to understand where you were coming from to make certain
I had not looked over something generic.  The closest to generic
I can make of your concern is that we are doing useless work if
we know there is an upstream msi to irq mapper that we can just
tunnel to over hypertransport.  At the generic level we can't know
that there is an upstream mapping capability as nothing reports
it, so we even if it is a good idea we can't do anything.

So a quirk looks fine to do what you are doing. 

Just please next round handle the address part of the msi mapping
capability if it is present, in the x86 part of the generic code.
Just in case someone implements that.

Eric