[PATCH] Dom0-kernel: Fix buggy mask_base in saving/restoring MSI-X table during S3

[PATCH] Dom0-kernel: Fix buggy mask_base in saving/restoring MSI-X table during S3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Hi, Keir, Jan,

This patch is a bugfix pointed by Jan. Fix mask_base(actually MSI-X table base, copy name from native) to be a virtual address rather than a physical address. And remove wrong printk in pci_disable_msix.

Jan, the error message you saw is wrong output from kernel's MSI code. Really sorry for my dirty code there.

Could you please review the patch and give me feedback?

Best Regards
Haitao Shan

[-- Attachment #2: fix_msix_table_base.patch --]
[-- Type: application/octet-stream, Size: 4296 bytes --]

diff -r 0b859c9516ba drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Wed Nov 26 11:13:49 2008 +0000
+++ b/drivers/pci/msi-xen.c	Thu Nov 27 11:38:25 2008 +0800
@@ -42,6 +42,8 @@
 	struct list_head list;
 	spinlock_t pirq_list_lock;
 	struct list_head pirq_list_head;
+	/* Used for saving/restoring MSI-X tables */
+	void __iomem *mask_base;
 };
 
 struct msi_pirq_entry {
@@ -50,7 +52,6 @@
 	int entry_nr;
 #ifdef CONFIG_PM
 	/* PM save area for MSIX address/data */
-	void __iomem *mask_base;
 	u32	address_hi_save;
 	u32	address_lo_save;
 	u32	data_save;
@@ -90,7 +91,7 @@
 	return ret;
 }
 
-static int attach_pirq_entry(int pirq, int entry_nr, u64 table_base,
+static int attach_pirq_entry(int pirq, int entry_nr,
                              struct msi_dev_list *msi_dev_entry)
 {
 	struct msi_pirq_entry *entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
@@ -100,9 +101,6 @@
 		return -ENOMEM;
 	entry->pirq = pirq;
 	entry->entry_nr = entry_nr;
-#ifdef COMFIG_PM
-	entry->mask_base = table_base;
-#endif
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_add_tail(&entry->list, &msi_dev_entry->pirq_list_head);
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
@@ -381,6 +379,7 @@
 	unsigned long flags;
 	struct msi_dev_list *msi_dev_entry;
 	struct msi_pirq_entry *pirq_entry;
+	void __iomem *base;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0 || dev->no_msi)
@@ -401,14 +400,13 @@
 	*((u16 *)&save_state->data[0]) = control;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		int j;
-		void __iomem *base;
 
 		/* save the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
 		printk(KERN_CRIT "Save msix table entry %d pirq %x base %p\n",
 		       j, pirq_entry->pirq, base);
@@ -454,11 +452,11 @@
 		return;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		/* route the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
 
 		printk(KERN_CRIT "Restore msix table entry %d pirq %x base %p\n",
@@ -523,7 +521,8 @@
 				struct msix_entry *entries, int nvec)
 {
 	u64 table_base;
-	int pirq, i, j, mapped, pos;
+	u16 control;
+	int pirq, i, j, mapped, pos, nr_entries;
 	struct msi_dev_list *msi_dev_entry = get_msi_dev_pirq_list(dev);
 	struct msi_pirq_entry *pirq_entry;
 
@@ -534,6 +533,12 @@
 	table_base = find_table_base(dev, pos);
 	if (!table_base)
 		return -ENODEV;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	nr_entries = multi_msix_capable(control);
+	if (!msi_dev_entry->mask_base)
+		msi_dev_entry->mask_base = 
+			ioremap_nocache(table_base, nr_entries * PCI_MSIX_ENTRY_SIZE);
 
 	/* MSI-X Table Initialization */
 	for (i = 0; i < nvec; i++) {
@@ -554,7 +559,7 @@
 		pirq = msi_map_vector(dev, entries[i].entry, table_base);
 		if (pirq < 0)
 			break;
-		attach_pirq_entry(pirq, entries[i].entry, table_base, msi_dev_entry);
+		attach_pirq_entry(pirq, entries[i].entry, msi_dev_entry);
 		(entries + i)->vector = pirq;
 	}
 
@@ -739,7 +744,7 @@
 			if (mapped)
 				continue;
 			irq = evtchn_map_pirq(-1, entries[i].vector);
-			attach_pirq_entry(irq, entries[i].entry, 0, msi_dev_entry);
+			attach_pirq_entry(irq, entries[i].entry, msi_dev_entry);
 			entries[i].vector = irq;
 		}
         return 0;
@@ -857,18 +862,15 @@
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	if (!list_empty(&msi_dev_entry->pirq_list_head))
-	{
-		printk(KERN_WARNING "msix pirqs for dev %02x:%02x:%01x are not freed \
-		       before acquire again.\n", dev->bus->number, PCI_SLOT(dev->devfn),
-			   PCI_FUNC(dev->devfn));
 		list_for_each_entry_safe(pirq_entry, tmp,
 		                         &msi_dev_entry->pirq_list_head, list) {
 			msi_unmap_pirq(dev, pirq_entry->pirq);
 			list_del(&pirq_entry->list);
 			kfree(pirq_entry);
 		}
-	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
+	iounmap(msi_dev_entry->mask_base);
+	msi_dev_entry->mask_base = NULL;
 	dev->irq = dev->irq_old;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] Dom0-kernel: Fix buggy mask_base in saving/restoring MSI-X table during S3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

Sorry, I attache a wrong version of my patch.
Please use this patch instead.

Best Regards
Haitao Shan

Shan, Haitao wrote:
> Hi, Keir, Jan,
> 
> This patch is a bugfix pointed by Jan. Fix mask_base(actually MSI-X
> table base, copy name from native) to be a virtual address rather
> than a physical address. And remove wrong printk in pci_disable_msix.
> 
> Jan, the error message you saw is wrong output from kernel's MSI
> code. Really sorry for my dirty code there. 
> 
> Could you please review the patch and give me feedback?
> 
> Best Regards
> Haitao Shan

[-- Attachment #2: fix_msix_table_base.patch --]
[-- Type: application/octet-stream, Size: 4951 bytes --]

diff -r 0b859c9516ba drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Wed Nov 26 11:13:49 2008 +0000
+++ b/drivers/pci/msi-xen.c	Thu Nov 27 12:06:55 2008 +0800
@@ -42,6 +42,8 @@
 	struct list_head list;
 	spinlock_t pirq_list_lock;
 	struct list_head pirq_list_head;
+	/* Used for saving/restoring MSI-X tables */
+	void __iomem *mask_base;
 };
 
 struct msi_pirq_entry {
@@ -50,7 +52,6 @@
 	int entry_nr;
 #ifdef CONFIG_PM
 	/* PM save area for MSIX address/data */
-	void __iomem *mask_base;
 	u32	address_hi_save;
 	u32	address_lo_save;
 	u32	data_save;
@@ -90,7 +91,7 @@
 	return ret;
 }
 
-static int attach_pirq_entry(int pirq, int entry_nr, u64 table_base,
+static int attach_pirq_entry(int pirq, int entry_nr,
                              struct msi_dev_list *msi_dev_entry)
 {
 	struct msi_pirq_entry *entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
@@ -100,9 +101,6 @@
 		return -ENOMEM;
 	entry->pirq = pirq;
 	entry->entry_nr = entry_nr;
-#ifdef COMFIG_PM
-	entry->mask_base = table_base;
-#endif
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_add_tail(&entry->list, &msi_dev_entry->pirq_list_head);
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
@@ -381,12 +379,11 @@
 	unsigned long flags;
 	struct msi_dev_list *msi_dev_entry;
 	struct msi_pirq_entry *pirq_entry;
+	void __iomem *base;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0 || dev->no_msi)
 		return 0;
-
-	printk(KERN_CRIT "Saving MSIX cap\n");
 
 	/* save the capability */
 	pci_read_config_word(dev, msi_control_reg(pos), &control);
@@ -401,18 +398,14 @@
 	*((u16 *)&save_state->data[0]) = control;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		int j;
-		void __iomem *base;
 
 		/* save the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-		printk(KERN_CRIT "Save msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
-
 		pirq_entry->address_lo_save =
 			readl(base + j * PCI_MSIX_ENTRY_SIZE +
 			      PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -443,7 +436,6 @@
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSIX);
 	if (!save_state)
 		return;
-	printk(KERN_CRIT "Restoring MSIX cap\n");
 
 	save = *((u16 *)&save_state->data[0]);
 	pci_remove_saved_cap(save_state);
@@ -454,15 +446,12 @@
 		return;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		/* route the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-
-		printk(KERN_CRIT "Restore msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
 		writel(pirq_entry->address_lo_save,
 			base + j * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -523,7 +512,8 @@
 				struct msix_entry *entries, int nvec)
 {
 	u64 table_base;
-	int pirq, i, j, mapped, pos;
+	u16 control;
+	int pirq, i, j, mapped, pos, nr_entries;
 	struct msi_dev_list *msi_dev_entry = get_msi_dev_pirq_list(dev);
 	struct msi_pirq_entry *pirq_entry;
 
@@ -534,6 +524,12 @@
 	table_base = find_table_base(dev, pos);
 	if (!table_base)
 		return -ENODEV;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	nr_entries = multi_msix_capable(control);
+	if (!msi_dev_entry->mask_base)
+		msi_dev_entry->mask_base = 
+			ioremap_nocache(table_base, nr_entries * PCI_MSIX_ENTRY_SIZE);
 
 	/* MSI-X Table Initialization */
 	for (i = 0; i < nvec; i++) {
@@ -554,7 +550,7 @@
 		pirq = msi_map_vector(dev, entries[i].entry, table_base);
 		if (pirq < 0)
 			break;
-		attach_pirq_entry(pirq, entries[i].entry, table_base, msi_dev_entry);
+		attach_pirq_entry(pirq, entries[i].entry, msi_dev_entry);
 		(entries + i)->vector = pirq;
 	}
 
@@ -739,7 +735,7 @@
 			if (mapped)
 				continue;
 			irq = evtchn_map_pirq(-1, entries[i].vector);
-			attach_pirq_entry(irq, entries[i].entry, 0, msi_dev_entry);
+			attach_pirq_entry(irq, entries[i].entry, msi_dev_entry);
 			entries[i].vector = irq;
 		}
         return 0;
@@ -857,18 +853,15 @@
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	if (!list_empty(&msi_dev_entry->pirq_list_head))
-	{
-		printk(KERN_WARNING "msix pirqs for dev %02x:%02x:%01x are not freed \
-		       before acquire again.\n", dev->bus->number, PCI_SLOT(dev->devfn),
-			   PCI_FUNC(dev->devfn));
 		list_for_each_entry_safe(pirq_entry, tmp,
 		                         &msi_dev_entry->pirq_list_head, list) {
 			msi_unmap_pirq(dev, pirq_entry->pirq);
 			list_del(&pirq_entry->list);
 			kfree(pirq_entry);
 		}
-	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
+	iounmap(msi_dev_entry->mask_base);
+	msi_dev_entry->mask_base = NULL;
 	dev->irq = dev->irq_old;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] Dom0-kernel: Fix buggy mask_base insaving/restoring MSI-X table during S3

[Jan Beulich]

>>> "Shan, Haitao" <haitao.shan@intel.com> 28.11.08 05:37 >>>
> Could you please review the patch and give me feedback?

Looks a lot better this way. The only issue remaining is the missing error
handling if the ioremap_nocache() fails.

Thanks, Jan

RE: [PATCH] Dom0-kernel: Fix buggy mask_base insaving/restoring MSI-X table during S3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

Thanks! And updated.
OK for you?

Best Regards
Haitao Shan

Jan Beulich wrote:
>>>> "Shan, Haitao" <haitao.shan@intel.com> 28.11.08 05:37 >>>
>> Could you please review the patch and give me feedback?
> 
> Looks a lot better this way. The only issue remaining is the missing
> error handling if the ioremap_nocache() fails.
> 
> Thanks, Jan

[-- Attachment #2: fix_msix_table_base.patch --]
[-- Type: application/octet-stream, Size: 5011 bytes --]

diff -r 0b859c9516ba drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Wed Nov 26 11:13:49 2008 +0000
+++ b/drivers/pci/msi-xen.c	Thu Nov 27 15:43:36 2008 +0800
@@ -42,6 +42,8 @@
 	struct list_head list;
 	spinlock_t pirq_list_lock;
 	struct list_head pirq_list_head;
+	/* Used for saving/restoring MSI-X tables */
+	void __iomem *mask_base;
 };
 
 struct msi_pirq_entry {
@@ -50,7 +52,6 @@
 	int entry_nr;
 #ifdef CONFIG_PM
 	/* PM save area for MSIX address/data */
-	void __iomem *mask_base;
 	u32	address_hi_save;
 	u32	address_lo_save;
 	u32	data_save;
@@ -90,7 +91,7 @@
 	return ret;
 }
 
-static int attach_pirq_entry(int pirq, int entry_nr, u64 table_base,
+static int attach_pirq_entry(int pirq, int entry_nr,
                              struct msi_dev_list *msi_dev_entry)
 {
 	struct msi_pirq_entry *entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
@@ -100,9 +101,6 @@
 		return -ENOMEM;
 	entry->pirq = pirq;
 	entry->entry_nr = entry_nr;
-#ifdef COMFIG_PM
-	entry->mask_base = table_base;
-#endif
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_add_tail(&entry->list, &msi_dev_entry->pirq_list_head);
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
@@ -381,12 +379,11 @@
 	unsigned long flags;
 	struct msi_dev_list *msi_dev_entry;
 	struct msi_pirq_entry *pirq_entry;
+	void __iomem *base;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0 || dev->no_msi)
 		return 0;
-
-	printk(KERN_CRIT "Saving MSIX cap\n");
 
 	/* save the capability */
 	pci_read_config_word(dev, msi_control_reg(pos), &control);
@@ -401,18 +398,14 @@
 	*((u16 *)&save_state->data[0]) = control;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		int j;
-		void __iomem *base;
 
 		/* save the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-		printk(KERN_CRIT "Save msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
-
 		pirq_entry->address_lo_save =
 			readl(base + j * PCI_MSIX_ENTRY_SIZE +
 			      PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -443,7 +436,6 @@
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSIX);
 	if (!save_state)
 		return;
-	printk(KERN_CRIT "Restoring MSIX cap\n");
 
 	save = *((u16 *)&save_state->data[0]);
 	pci_remove_saved_cap(save_state);
@@ -454,15 +446,12 @@
 		return;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		/* route the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-
-		printk(KERN_CRIT "Restore msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
 		writel(pirq_entry->address_lo_save,
 			base + j * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -523,7 +512,8 @@
 				struct msix_entry *entries, int nvec)
 {
 	u64 table_base;
-	int pirq, i, j, mapped, pos;
+	u16 control;
+	int pirq, i, j, mapped, pos, nr_entries;
 	struct msi_dev_list *msi_dev_entry = get_msi_dev_pirq_list(dev);
 	struct msi_pirq_entry *pirq_entry;
 
@@ -534,6 +524,15 @@
 	table_base = find_table_base(dev, pos);
 	if (!table_base)
 		return -ENODEV;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	nr_entries = multi_msix_capable(control);
+	if (!msi_dev_entry->mask_base) {
+		msi_dev_entry->mask_base = 
+			ioremap_nocache(table_base, nr_entries * PCI_MSIX_ENTRY_SIZE);
+		if (!msi_dev_entry->mask_base)
+			return -ENOMEM;
+	}
 
 	/* MSI-X Table Initialization */
 	for (i = 0; i < nvec; i++) {
@@ -554,7 +553,7 @@
 		pirq = msi_map_vector(dev, entries[i].entry, table_base);
 		if (pirq < 0)
 			break;
-		attach_pirq_entry(pirq, entries[i].entry, table_base, msi_dev_entry);
+		attach_pirq_entry(pirq, entries[i].entry, msi_dev_entry);
 		(entries + i)->vector = pirq;
 	}
 
@@ -739,7 +738,7 @@
 			if (mapped)
 				continue;
 			irq = evtchn_map_pirq(-1, entries[i].vector);
-			attach_pirq_entry(irq, entries[i].entry, 0, msi_dev_entry);
+			attach_pirq_entry(irq, entries[i].entry, msi_dev_entry);
 			entries[i].vector = irq;
 		}
         return 0;
@@ -857,18 +856,15 @@
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	if (!list_empty(&msi_dev_entry->pirq_list_head))
-	{
-		printk(KERN_WARNING "msix pirqs for dev %02x:%02x:%01x are not freed \
-		       before acquire again.\n", dev->bus->number, PCI_SLOT(dev->devfn),
-			   PCI_FUNC(dev->devfn));
 		list_for_each_entry_safe(pirq_entry, tmp,
 		                         &msi_dev_entry->pirq_list_head, list) {
 			msi_unmap_pirq(dev, pirq_entry->pirq);
 			list_del(&pirq_entry->list);
 			kfree(pirq_entry);
 		}
-	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
+	iounmap(msi_dev_entry->mask_base);
+	msi_dev_entry->mask_base = NULL;
 	dev->irq = dev->irq_old;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] Dom0-kernel: Fix buggy mask_base insaving/restoring MSI-X table during S3

[Jan Beulich]

>>> "Shan, Haitao" <haitao.shan@intel.com> 28.11.08 09:14 >>>
>Thanks! And updated.
>OK for you?

Only half way: You shouldn't really fail device initialization in that case, all
that should not work is suspend/resume.

Jan

RE: [PATCH] Dom0-kernel: Fix buggy mask_base insaving/restoring MSI-X table during S3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Any idea of how to fail suspend/resume?
For example, if we failed in ioremap_nocache, we just do nothing at suspend/resume? Let the device to handle it?

Best Regards
Shan Haitao
-----Original Message-----
From: Jan Beulich [mailto:jbeulich@novell.com] 
Sent: 2008年11月28日 16:50
To: Shan, Haitao
Cc: 'Keir Fraser'; 'xen-devel@lists.xensource.com'
Subject: RE: [Xen-devel] [PATCH] Dom0-kernel: Fix buggy mask_base insaving/restoring MSI-X table during S3

>>> "Shan, Haitao" <haitao.shan@intel.com> 28.11.08 09:14 >>>
>Thanks! And updated.
>OK for you?

Only half way: You shouldn't really fail device initialization in that case, all
that should not work is suspend/resume.

Jan


[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH] Dom0-kernel: Fix buggy mask_base in saving/restoring MSI-X table during S3 - Version 3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

Hi, Keir,

This is the most updated version of my patch. Thanks for Jan's kind comments. It looks better now.
Would you please apply the patch?

Best Regards
Haitao Shan

Shan, Haitao wrote:
> Any idea of how to fail suspend/resume?
> For example, if we failed in ioremap_nocache, we just do nothing at
> suspend/resume? Let the device to handle it?
> 
> Best Regards
> Shan Haitao
Jan Beulich wrote:
>>>> "Shan, Haitao" <haitao.shan@intel.com> 28.11.08 09:14 >>>
>> Thanks! And updated.
>> OK for you?
> 
> Only half way: You shouldn't really fail device initialization in
> that case, all that should not work is suspend/resume.
> 
> Jan

[-- Attachment #2: fix_msix_table_base.patch --]
[-- Type: application/octet-stream, Size: 5307 bytes --]

diff -r 0b859c9516ba drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Wed Nov 26 11:13:49 2008 +0000
+++ b/drivers/pci/msi-xen.c	Thu Nov 27 23:12:02 2008 +0800
@@ -42,6 +42,8 @@
 	struct list_head list;
 	spinlock_t pirq_list_lock;
 	struct list_head pirq_list_head;
+	/* Used for saving/restoring MSI-X tables */
+	void __iomem *mask_base;
 };
 
 struct msi_pirq_entry {
@@ -50,7 +52,6 @@
 	int entry_nr;
 #ifdef CONFIG_PM
 	/* PM save area for MSIX address/data */
-	void __iomem *mask_base;
 	u32	address_hi_save;
 	u32	address_lo_save;
 	u32	data_save;
@@ -90,7 +91,7 @@
 	return ret;
 }
 
-static int attach_pirq_entry(int pirq, int entry_nr, u64 table_base,
+static int attach_pirq_entry(int pirq, int entry_nr,
                              struct msi_dev_list *msi_dev_entry)
 {
 	struct msi_pirq_entry *entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
@@ -100,9 +101,6 @@
 		return -ENOMEM;
 	entry->pirq = pirq;
 	entry->entry_nr = entry_nr;
-#ifdef COMFIG_PM
-	entry->mask_base = table_base;
-#endif
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_add_tail(&entry->list, &msi_dev_entry->pirq_list_head);
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
@@ -381,17 +379,24 @@
 	unsigned long flags;
 	struct msi_dev_list *msi_dev_entry;
 	struct msi_pirq_entry *pirq_entry;
+	void __iomem *base;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0 || dev->no_msi)
 		return 0;
 
-	printk(KERN_CRIT "Saving MSIX cap\n");
-
 	/* save the capability */
 	pci_read_config_word(dev, msi_control_reg(pos), &control);
 	if (!(control & PCI_MSIX_FLAGS_ENABLE))
 		return 0;
+
+	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	/* If we failed to map the MSI-X table at pci_enable_msix,
+	 * We could not support saving them here.
+	 */
+	if (!(base = msi_dev_entry->mask_base))
+		return -ENOMEM;
+
 	save_state = kzalloc(sizeof(struct pci_cap_saved_state) + sizeof(u16),
 		GFP_KERNEL);
 	if (!save_state) {
@@ -400,19 +405,12 @@
 	}
 	*((u16 *)&save_state->data[0]) = control;
 
-	msi_dev_entry = get_msi_dev_pirq_list(dev);
-
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		int j;
-		void __iomem *base;
 
 		/* save the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-		printk(KERN_CRIT "Save msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
-
 		pirq_entry->address_lo_save =
 			readl(base + j * PCI_MSIX_ENTRY_SIZE +
 			      PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -443,7 +441,6 @@
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSIX);
 	if (!save_state)
 		return;
-	printk(KERN_CRIT "Restoring MSIX cap\n");
 
 	save = *((u16 *)&save_state->data[0]);
 	pci_remove_saved_cap(save_state);
@@ -454,15 +451,12 @@
 		return;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
+	base = msi_dev_entry->mask_base;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
 		/* route the table */
-		base = pirq_entry->mask_base;
 		j = pirq_entry->entry_nr;
-
-		printk(KERN_CRIT "Restore msix table entry %d pirq %x base %p\n",
-		       j, pirq_entry->pirq, base);
 		writel(pirq_entry->address_lo_save,
 			base + j * PCI_MSIX_ENTRY_SIZE +
 			PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
@@ -523,7 +517,8 @@
 				struct msix_entry *entries, int nvec)
 {
 	u64 table_base;
-	int pirq, i, j, mapped, pos;
+	u16 control;
+	int pirq, i, j, mapped, pos, nr_entries;
 	struct msi_dev_list *msi_dev_entry = get_msi_dev_pirq_list(dev);
 	struct msi_pirq_entry *pirq_entry;
 
@@ -534,6 +529,12 @@
 	table_base = find_table_base(dev, pos);
 	if (!table_base)
 		return -ENODEV;
+
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	nr_entries = multi_msix_capable(control);
+	if (!msi_dev_entry->mask_base)
+		msi_dev_entry->mask_base = 
+			ioremap_nocache(table_base, nr_entries * PCI_MSIX_ENTRY_SIZE);
 
 	/* MSI-X Table Initialization */
 	for (i = 0; i < nvec; i++) {
@@ -554,7 +555,7 @@
 		pirq = msi_map_vector(dev, entries[i].entry, table_base);
 		if (pirq < 0)
 			break;
-		attach_pirq_entry(pirq, entries[i].entry, table_base, msi_dev_entry);
+		attach_pirq_entry(pirq, entries[i].entry, msi_dev_entry);
 		(entries + i)->vector = pirq;
 	}
 
@@ -739,7 +740,7 @@
 			if (mapped)
 				continue;
 			irq = evtchn_map_pirq(-1, entries[i].vector);
-			attach_pirq_entry(irq, entries[i].entry, 0, msi_dev_entry);
+			attach_pirq_entry(irq, entries[i].entry, msi_dev_entry);
 			entries[i].vector = irq;
 		}
         return 0;
@@ -857,18 +858,15 @@
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	if (!list_empty(&msi_dev_entry->pirq_list_head))
-	{
-		printk(KERN_WARNING "msix pirqs for dev %02x:%02x:%01x are not freed \
-		       before acquire again.\n", dev->bus->number, PCI_SLOT(dev->devfn),
-			   PCI_FUNC(dev->devfn));
 		list_for_each_entry_safe(pirq_entry, tmp,
 		                         &msi_dev_entry->pirq_list_head, list) {
 			msi_unmap_pirq(dev, pirq_entry->pirq);
 			list_del(&pirq_entry->list);
 			kfree(pirq_entry);
 		}
-	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
+	iounmap(msi_dev_entry->mask_base);
+	msi_dev_entry->mask_base = NULL;
 	dev->irq = dev->irq_old;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel