[PATCH] Xen PCI and Xen SWIOTLB patches for 3.2

[PATCH] Xen PCI and Xen SWIOTLB patches for 3.2

[Konrad Rzeszutek Wilk]

I am proposing these five patches for 3.2.

Nothing really exciting about them - just update some comments, fix
comments, and also make Xen SWIOTLB a bit smarter. Please take a look.

 drivers/pci/xen-pcifront.c |    4 ++--
 drivers/xen/pci.c          |   11 +++++++----
 drivers/xen/swiotlb-xen.c  |   42 +++++++++++++++++++++++++++++-------------
 3 files changed, 38 insertions(+), 19 deletions(-)

Jan Beulich (1):
      xen/pci: make bus notifier handler return sane values

Konrad Rzeszutek Wilk (3):
      xen-pcifront: Update warning comment to use 'e820_host' option.
      xen-swiotlb: Retry up three times to allocate Xen-SWIOTLB
      xen-swiotlb: Fix wrong panic.

Randy Dunlap (1):
      xen-swiotlb: fix printk and panic args

[PATCH 1/5] xen-pcifront: Update warning comment to use 'e820_host' option.

[Konrad Rzeszutek Wilk]

With Xen changeset 23428 "libxl: Add 'e820_host' option to config file"
the E820 as seen from the host can now be passed into the guest.
This means that a PV guest can now:
 - Use the correct PCI I/O gap. Before these patches, Linux guest would
   boot up and would tell:
   [    0.000000] Allocating PCI resources starting at 40000000 (gap: 40000000:c0000000)
   while in actuality the PCI I/O gap should have been:
   [    0.000000] Allocating PCI resources starting at b0000000 (gap: b0000000:4c000000)

 - The PV domain with PCI devices was limited to 3GB. It now can be booted
   with 4GB, 8GB, or whatever number you want. The PCI devices will now _not_ conflict
   with System RAM. Meaning the drivers can load.

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
CC: linux-pci@vger.kernel.org
CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 492b7d8..659efd1 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -401,8 +401,8 @@ static int pcifront_claim_resource(struct pci_dev *dev, void *data)
 				pci_name(dev), i);
 			if (pci_claim_resource(dev, i)) {
 				dev_err(&pdev->xdev->dev, "Could not claim "
-					"resource %s/%d! Device offline. Try "
-					"giving less than 4GB to domain.\n",
+					"resource %s/%d! Device offline. Try"
+					"using e820_host=1 in the guest config.\n",
 					pci_name(dev), i);
 			}
 		}
-- 
1.7.4.1

Re: [PATCH 1/5] xen-pcifront: Update warning comment to use 'e820_host' option.

[Joe Perches]

On Wed, 2011-08-24 at 18:31 -0400, Konrad Rzeszutek Wilk wrote:
> With Xen changeset 23428 "libxl: Add 'e820_host' option to config file"
> the E820 as seen from the host can now be passed into the guest.
> This means that a PV guest can now:
>  - Use the correct PCI I/O gap. Before these patches, Linux guest would
>    boot up and would tell:
>    [    0.000000] Allocating PCI resources starting at 40000000 (gap: 40000000:c0000000)
>    while in actuality the PCI I/O gap should have been:
>    [    0.000000] Allocating PCI resources starting at b0000000 (gap: b0000000:4c000000)
>  - The PV domain with PCI devices was limited to 3GB. It now can be booted
>    with 4GB, 8GB, or whatever number you want. The PCI devices will now _not_ conflict
>    with System RAM. Meaning the drivers can load.
[]
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
[]
> @@ -401,8 +401,8 @@ static int pcifront_claim_resource(struct pci_dev *dev, void *data)
>  				pci_name(dev), i);
>  			if (pci_claim_resource(dev, i)) {
>  				dev_err(&pdev->xdev->dev, "Could not claim "
> -					"resource %s/%d! Device offline. Try "
> -					"giving less than 4GB to domain.\n",
> +					"resource %s/%d! Device offline. Try"
> +					"using e820_host=1 in the guest config.\n",

Trivia: You lost the space after "Try".

It's generally better not to split format strings into
multiple pieces.

Perhaps:
 				dev_err(&pdev->xdev->dev, "Could not claim resource %s/%d! Device offline. Try using e820_host=1 in the guest config.\n",

or
 				dev_err(&pdev->xdev->dev, "Could not claim resource %s/%d! "
					"Device offline. Try using e820_host=1 in the guest config.\n",

Re: [Xen-devel] Re: [PATCH 1/5] xen-pcifront: Update warning comment to use 'e820_host' option.

[Konrad Rzeszutek Wilk]

>  				dev_err(&pdev->xdev->dev, "Could not claim resource %s/%d! "
> 					"Device offline. Try using e820_host=1 in the guest config.\n",
> 

Done. Picked the one above. Thanks!

[PATCH 2/5] xen-swiotlb: Retry up three times to allocate Xen-SWIOTLB

[Konrad Rzeszutek Wilk]

We can fail seting up Xen-SWIOTLB if:
 - The host does not have enough contiguous DMA32 memory available
   (can happen on a machine that has fragmented memory from starting,
   stopping many guests).
 - Not enough low memory (almost never happens).

We retry allocating and exchanging the swath of contiguous memory
up to three times. Each time we decrease the amount we need  - the
minimum being of 2MB.

If we completly fail, we will print the reason for failure on the Xen
console on top of doing it to earlyprintk=xen console.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/swiotlb-xen.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 6e8c15a..d45cbac 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -38,6 +38,7 @@
 #include <xen/swiotlb-xen.h>
 #include <xen/page.h>
 #include <xen/xen-ops.h>
+#include <xen/hvc-console.h>
 /*
  * Used to do a quick range check in swiotlb_tbl_unmap_single and
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
@@ -146,8 +147,10 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
 void __init xen_swiotlb_init(int verbose)
 {
 	unsigned long bytes;
-	int rc;
+	int rc = -ENOMEM;
 	unsigned long nr_tbl;
+	char *m = NULL;
+	unsigned int repeat = 3;
 
 	nr_tbl = swioltb_nr_tbl();
 	if (nr_tbl)
@@ -156,16 +159,17 @@ void __init xen_swiotlb_init(int verbose)
 		xen_io_tlb_nslabs = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
 		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, IO_TLB_SEGSIZE);
 	}
-
+retry:
 	bytes = xen_io_tlb_nslabs << IO_TLB_SHIFT;
 
 	/*
 	 * Get IO TLB memory from any location.
 	 */
 	xen_io_tlb_start = alloc_bootmem(bytes);
-	if (!xen_io_tlb_start)
-		panic("Cannot allocate SWIOTLB buffer");
-
+	if (!xen_io_tlb_start) {
+		m = "Cannot allocate Xen-SWIOTLB buffer!\n";
+		goto error;
+	}
 	xen_io_tlb_end = xen_io_tlb_start + bytes;
 	/*
 	 * And replace that memory with pages under 4GB.
@@ -173,17 +177,28 @@ void __init xen_swiotlb_init(int verbose)
 	rc = xen_swiotlb_fixup(xen_io_tlb_start,
 			       bytes,
 			       xen_io_tlb_nslabs);
-	if (rc)
+	if (rc) {
+		free_bootmem(__pa(xen_io_tlb_start), bytes);
+		m = "Failed to get contiguous memory for DMA from Xen!\n"\
+		    "You either: don't have the permissions, do not have"\
+		    " enough free memory under 4GB, or the hypervisor memory"\
+		    "is too fragmented!";
 		goto error;
-
+	}
 	start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
 	swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs, verbose);
 
 	return;
 error:
-	panic("DMA(%d): Failed to exchange pages allocated for DMA with Xen! "\
-	      "We either don't have the permission or you do not have enough"\
-	      "free memory under 4GB!\n", rc);
+	if (repeat--) {
+		xen_io_tlb_nslabs = max(1024UL, /* Min is 2MB */
+					(xen_io_tlb_nslabs >> 1));
+		printk(KERN_INFO "Xen-SWIOTLB: Lowering to %luMB\n",
+		      (xen_io_tlb_nslabs << IO_TLB_SHIFT) >> 20);
+		goto retry;
+	}
+	xen_raw_printk("%s (rc:%d)", rc, m);
+	panic("%s (rc:%d)", rc, m);
 }
 
 void *
-- 
1.7.4.1

[PATCH 3/5] xen-swiotlb: Fix wrong panic.

[Konrad Rzeszutek Wilk]

Propagate the baremetal git commit "swiotlb: fix wrong panic"
(fba99fa38b023224680308a482e12a0eca87e4e1) in the Xen-SWIOTLB version.
wherein swiotlb's map_page wrongly calls panic() when it can't find
a buffer fit for device's dma mask.  It should return an error instead.

Devices with an odd dma mask (i.e.  under 4G) like b44 network card hit
this bug (the system crashes):

http://marc.info/?l=linux-kernel&m=129648943830106&w=2

If xen-swiotlb returns an error, b44 driver can use the own bouncing
mechanism.

CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/swiotlb-xen.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index d45cbac..ea8c289 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -293,9 +293,10 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
 	/*
 	 * Ensure that the address returned is DMA'ble
 	 */
-	if (!dma_capable(dev, dev_addr, size))
-		panic("map_single: bounce buffer is not DMA'ble");
-
+	if (!dma_capable(dev, dev_addr, size)) {
+		swiotlb_tbl_unmap_single(dev, map, size, dir);
+		dev_addr = 0;
+	}
 	return dev_addr;
 }
 EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
-- 
1.7.4.1

_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable

[PATCH 4/5] xen-swiotlb: fix printk and panic args

[Konrad Rzeszutek Wilk]

From: Randy Dunlap <rdunlap@xenotime.net>

Fix printk() and panic() args [swap them] to fix build warnings:

drivers/xen/swiotlb-xen.c:201: warning: format '%s' expects type 'char *', but argument 2 has type 'int'
drivers/xen/swiotlb-xen.c:201: warning: format '%d' expects type 'int', but argument 3 has type 'char *'
drivers/xen/swiotlb-xen.c:202: warning: format '%s' expects type 'char *', but argument 2 has type 'int'
drivers/xen/swiotlb-xen.c:202: warning: format '%d' expects type 'int', but argument 3 has type 'char *'

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/swiotlb-xen.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index ea8c289..0408f32 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -197,8 +197,8 @@ error:
 		      (xen_io_tlb_nslabs << IO_TLB_SHIFT) >> 20);
 		goto retry;
 	}
-	xen_raw_printk("%s (rc:%d)", rc, m);
-	panic("%s (rc:%d)", rc, m);
+	xen_raw_printk("%s (rc:%d)", m, rc);
+	panic("%s (rc:%d)", m, rc);
 }
 
 void *
-- 
1.7.4.1

[PATCH 5/5] xen/pci: make bus notifier handler return sane values

[Konrad Rzeszutek Wilk]

From: Jan Beulich <JBeulich@novell.com>

Notifier functions are expected to return NOTIFY_* codes, not -E...
ones. In particular, since the respective hypercalls failing is not
fatal to the operation of the Dom0 kernel, it must be avoided to
return negative values here as those would make it appear as if
NOTIFY_STOP_MASK wa set, suppressing further notification calls to
other interested parties (which is also why we don't want to use
notifier_from_errno() here).

While at it, also notify the user of a failed hypercall.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
[v1: Added dev_err and the disable MSI/MSI-X call]
[v2: Removed the disable MSI/MSI-X call]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pci.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index cef4baf..02c402b 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -96,13 +96,16 @@ static int xen_pci_notifier(struct notifier_block *nb,
 		r = xen_remove_device(dev);
 		break;
 	default:
-		break;
+		return NOTIFY_DONE;
 	}
-
-	return r;
+	if (r)
+		dev_err(dev, "Failed to %s - passthrough or MSI/MSI-X might fail!\n",
+			action == BUS_NOTIFY_ADD_DEVICE ? "add" :
+			(action == BUS_NOTIFY_DEL_DEVICE ? "delete" : "?"));
+	return NOTIFY_OK;
 }
 
-struct notifier_block device_nb = {
+static struct notifier_block device_nb = {
 	.notifier_call = xen_pci_notifier,
 };
 
-- 
1.7.4.1