[PATCH] Xen PCI fronted fixes for 2.6.39

[PATCH] Xen PCI fronted fixes for 2.6.39

[Konrad Rzeszutek Wilk]

I am proposing these three patches for 2.6.39.

The first is to take advantage of the new method of requesting
a Linux IRQ and providing the Xen PIRQ value. The second
makes it possible for a PV guest to bootup if the backend has provided
incorrect values. [*2]

Lastly, the third is to remove deprecated code.

Konrad Rzeszutek Wilk (2):
      pci/xen: Use xen_allocate_pirq_msi
      xen-pcifront: Sanity check the MSI/MSI-X values

Tejun Heo (1):
      xen-pcifront: don't use flush_scheduled_work()

 arch/x86/pci/xen.c         |    6 +++---
 drivers/pci/xen-pcifront.c |   20 +++++++++++++++-----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 25cd4a0..6432f75 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
+		xen_allocate_pirq_msi(
 			(type == PCI_CAP_ID_MSIX) ?
-			"pcifront-msi-x" : "pcifront-msi");
+			"pcifront-msi-x" : "pcifront-msi",
+			&irq, &v[i], XEN_ALLOC_IRQ);
 		if (irq < 0) {
 			ret = -1;
 			goto free;
 		}
-
 		ret = set_irq_msi(irq, msidesc);
 		if (ret)
 			goto error_while;
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 3a5a6fc..dd615d9 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 	if (likely(!err)) {
 		if (likely(!op.value)) {
 			/* we get the result */
-			for (i = 0; i < nvec; i++)
+			for (i = 0; i < nvec; i++) {
+				if (op.msix_entries[i].vector <= 0) {
+					dev_warn(&dev->dev, "MSI-X entry %d" \
+						" is zero!\n", i);
+					err = -EINVAL;
+					continue;
+				}
 				*(*vector+i) = op.msix_entries[i].vector;
-			return 0;
+			}
+			return err;
 		} else {
 			printk(KERN_DEBUG "enable msix get value %x\n",
 				op.value);
-			return op.value;
+			return err;
 		}
 	} else {
 		dev_err(&dev->dev, "enable msix get err %x\n", err);
@@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
 	err = do_pci_op(pdev, &op);
 	if (likely(!err)) {
 		*(*vector) = op.value;
+		if (op.value <= 0) {
+			dev_warn(&dev->dev, "MSI entry is zero!\n");
+			err = -EINVAL;
+		}
 	} else {
 		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
 				    "%x:%x\n", op.bus, op.devfn);
@@ -733,8 +744,7 @@ static void free_pdev(struct pcifront_device *pdev)
 
 	pcifront_free_roots(pdev);
 
-	/*For PCIE_AER error handling job*/
-	flush_scheduled_work();
+	cancel_work_sync(&pdev->op_work);
 
 	if (pdev->irq >= 0)
 		unbind_from_irqhandler(pdev->irq, pdev);


[*2]: as so:

    0.877864] e1000e: Intel(R) PRO/1000 Network Driver - 1.2.20-k2
[    0.877874] e1000e: Copyright(c) 1999 - 2011 Intel Corporation.
[    0.877957] e1000e 0000:00:00.0: enabling device (0000 -> 0002)
[    0.878054] e1000e 0000:00:00.0: Xen PCI mapped GSI16 to IRQ27
[    0.878056] e1000e 0000:00:00.0: setting latency timer to 64
[    0.879924] e1000e 0000:00:00.0: MSI-X entry 0 is zero!
[    0.879935] e1000e 0000:00:00.0: MSI-X entry 1 is zero!
[    0.879940] e1000e 0000:00:00.0: MSI-X entry 2 is zero!
[    0.880207] e1000e 0000:00:00.0: (unregistered net_device): Failed to initialize MSI-X interrupts.  Falling back to MSI interrupts.
[    0.880730] e1000e 0000:00:00.0: MSI entry is zero!
[    0.880788] e1000e 0000:00:00.0: (unregistered net_device): Failed to initialize MSI interrupts.  Falling back to legacy interrupts.
[    0.880945] e1000e 0000:00:00.0: Disabling ASPM L0s 
[    0.977188] e1000e 0000:00:00.0: eth0: (PCI Express:2.5GB/s:Width x1) 00:1b:21:61:49:00
[    0.977201] e1000e 0000:00:00.0: eth0: Intel(R) PRO/1000 Network Connection
[    0.977217] e1000e 0000:00:00.0: eth0: MAC: 3, PHY: 8, PBA No: E46981-003
...

[PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Konrad Rzeszutek Wilk]

There is no need to use the old interface.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/pci/xen.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 25cd4a0..6432f75 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
+		xen_allocate_pirq_msi(
 			(type == PCI_CAP_ID_MSIX) ?
-			"pcifront-msi-x" : "pcifront-msi");
+			"pcifront-msi-x" : "pcifront-msi",
+			&irq, &v[i], XEN_ALLOC_IRQ);
 		if (irq < 0) {
 			ret = -1;
 			goto free;
 		}
-
 		ret = set_irq_msi(irq, msidesc);
 		if (ret)
 			goto error_while;
-- 
1.7.1

[PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

Check the returned vector values for any values that are
odd or plain incorrect (say vector value zero), and if so
print a warning. Also fixup the return values.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 3a5a6fc..6acf6ae 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 	if (likely(!err)) {
 		if (likely(!op.value)) {
 			/* we get the result */
-			for (i = 0; i < nvec; i++)
+			for (i = 0; i < nvec; i++) {
+				if (op.msix_entries[i].vector <= 0) {
+					dev_warn(&dev->dev, "MSI-X entry %d" \
+						" is zero!\n", i);
+					err = -EINVAL;
+					continue;
+				}
 				*(*vector+i) = op.msix_entries[i].vector;
-			return 0;
+			}
+			return err;
 		} else {
 			printk(KERN_DEBUG "enable msix get value %x\n",
 				op.value);
-			return op.value;
+			return err;
 		}
 	} else {
 		dev_err(&dev->dev, "enable msix get err %x\n", err);
@@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
 	err = do_pci_op(pdev, &op);
 	if (likely(!err)) {
 		*(*vector) = op.value;
+		if (op.value <= 0) {
+			dev_warn(&dev->dev, "MSI entry is zero!\n");
+			err = -EINVAL;
+		}
 	} else {
 		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
 				    "%x:%x\n", op.bus, op.devfn);
-- 
1.7.1

[PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

[Konrad Rzeszutek Wilk]

Check the returned vector values for any values that are
odd or plain incorrect (say vector value zero), and if so
print a warning. Also fixup the return values.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 3a5a6fc..6acf6ae 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 	if (likely(!err)) {
 		if (likely(!op.value)) {
 			/* we get the result */
-			for (i = 0; i < nvec; i++)
+			for (i = 0; i < nvec; i++) {
+				if (op.msix_entries[i].vector <= 0) {
+					dev_warn(&dev->dev, "MSI-X entry %d" \
+						" is zero!\n", i);
+					err = -EINVAL;
+					continue;
+				}
 				*(*vector+i) = op.msix_entries[i].vector;
-			return 0;
+			}
+			return err;
 		} else {
 			printk(KERN_DEBUG "enable msix get value %x\n",
 				op.value);
-			return op.value;
+			return err;
 		}
 	} else {
 		dev_err(&dev->dev, "enable msix get err %x\n", err);
@@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
 	err = do_pci_op(pdev, &op);
 	if (likely(!err)) {
 		*(*vector) = op.value;
+		if (op.value <= 0) {
+			dev_warn(&dev->dev, "MSI entry is zero!\n");
+			err = -EINVAL;
+		}
 	} else {
 		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
 				    "%x:%x\n", op.bus, op.devfn);
-- 
1.7.1

[PATCH 3/3] xen-pcifront: don't use flush_scheduled_work()

[Konrad Rzeszutek Wilk]

From: Tejun Heo <tj@kernel.org>

flush_scheduled_work() is scheduled for deprecation.  Cancel ->op_work
directly instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ryan Wilson <hap9@epoch.ncsc.mil>
Cc: Jan Beulich <JBeulich@novell.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 6acf6ae..dd615d9 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -744,8 +744,7 @@ static void free_pdev(struct pcifront_device *pdev)
 
 	pcifront_free_roots(pdev);
 
-	/*For PCIE_AER error handling job*/
-	flush_scheduled_work();
+	cancel_work_sync(&pdev->op_work);
 
 	if (pdev->irq >= 0)
 		unbind_from_irqhandler(pdev->irq, pdev);
-- 
1.7.1

Re: [Xen-devel] [PATCH] Xen PCI fronted fixes for 2.6.39

[Ian Campbell]

On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> I am proposing these three patches for 2.6.39.
> 
> The first is to take advantage of the new method of requesting
> a Linux IRQ and providing the Xen PIRQ value. The second
> makes it possible for a PV guest to bootup if the backend has provided
> incorrect values. [*2]

I approve of being liberal in what is accepted but do we also have a
handle on why the backend is providing incorrect values in the first
place?

> 
> Lastly, the third is to remove deprecated code.
> 
> Konrad Rzeszutek Wilk (2):
>       pci/xen: Use xen_allocate_pirq_msi
>       xen-pcifront: Sanity check the MSI/MSI-X values
> 
> Tejun Heo (1):
>       xen-pcifront: don't use flush_scheduled_work()
> 
>  arch/x86/pci/xen.c         |    6 +++---
>  drivers/pci/xen-pcifront.c |   20 +++++++++++++++-----
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 25cd4a0..6432f75 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		goto error;
>  	i = 0;
>  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> +		xen_allocate_pirq_msi(
>  			(type == PCI_CAP_ID_MSIX) ?
> -			"pcifront-msi-x" : "pcifront-msi");
> +			"pcifront-msi-x" : "pcifront-msi",
> +			&irq, &v[i], XEN_ALLOC_IRQ);
>  		if (irq < 0) {
>  			ret = -1;
>  			goto free;
>  		}
> -
>  		ret = set_irq_msi(irq, msidesc);
>  		if (ret)
>  			goto error_while;
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 3a5a6fc..dd615d9 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
>  	if (likely(!err)) {
>  		if (likely(!op.value)) {
>  			/* we get the result */
> -			for (i = 0; i < nvec; i++)
> +			for (i = 0; i < nvec; i++) {
> +				if (op.msix_entries[i].vector <= 0) {
> +					dev_warn(&dev->dev, "MSI-X entry %d" \
> +						" is zero!\n", i);
> +					err = -EINVAL;
> +					continue;
> +				}
>  				*(*vector+i) = op.msix_entries[i].vector;
> -			return 0;
> +			}
> +			return err;
>  		} else {
>  			printk(KERN_DEBUG "enable msix get value %x\n",
>  				op.value);
> -			return op.value;
> +			return err;
>  		}
>  	} else {
>  		dev_err(&dev->dev, "enable msix get err %x\n", err);
> @@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
>  	err = do_pci_op(pdev, &op);
>  	if (likely(!err)) {
>  		*(*vector) = op.value;
> +		if (op.value <= 0) {
> +			dev_warn(&dev->dev, "MSI entry is zero!\n");
> +			err = -EINVAL;
> +		}
>  	} else {
>  		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
>  				    "%x:%x\n", op.bus, op.devfn);
> @@ -733,8 +744,7 @@ static void free_pdev(struct pcifront_device *pdev)
>  
>  	pcifront_free_roots(pdev);
>  
> -	/*For PCIE_AER error handling job*/
> -	flush_scheduled_work();
> +	cancel_work_sync(&pdev->op_work);
>  
>  	if (pdev->irq >= 0)
>  		unbind_from_irqhandler(pdev->irq, pdev);
> 
> 
> [*2]: as so:
> 
>     0.877864] e1000e: Intel(R) PRO/1000 Network Driver - 1.2.20-k2
> [    0.877874] e1000e: Copyright(c) 1999 - 2011 Intel Corporation.
> [    0.877957] e1000e 0000:00:00.0: enabling device (0000 -> 0002)
> [    0.878054] e1000e 0000:00:00.0: Xen PCI mapped GSI16 to IRQ27
> [    0.878056] e1000e 0000:00:00.0: setting latency timer to 64
> [    0.879924] e1000e 0000:00:00.0: MSI-X entry 0 is zero!
> [    0.879935] e1000e 0000:00:00.0: MSI-X entry 1 is zero!
> [    0.879940] e1000e 0000:00:00.0: MSI-X entry 2 is zero!
> [    0.880207] e1000e 0000:00:00.0: (unregistered net_device): Failed to initialize MSI-X interrupts.  Falling back to MSI interrupts.
> [    0.880730] e1000e 0000:00:00.0: MSI entry is zero!
> [    0.880788] e1000e 0000:00:00.0: (unregistered net_device): Failed to initialize MSI interrupts.  Falling back to legacy interrupts.
> [    0.880945] e1000e 0000:00:00.0: Disabling ASPM L0s 
> [    0.977188] e1000e 0000:00:00.0: eth0: (PCI Express:2.5GB/s:Width x1) 00:1b:21:61:49:00
> [    0.977201] e1000e 0000:00:00.0: eth0: Intel(R) PRO/1000 Network Connection
> [    0.977217] e1000e 0000:00:00.0: eth0: MAC: 3, PHY: 8, PBA No: E46981-003
> ...
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Ian Campbell]

On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> There is no need to use the old interface.

xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
xen_initial_domain() in addition to the kernel side book-keeping side of
things (set chip and handler, update irq_info etc) whereas
xen_allocate_pirq_msi just does the kernel book keeping.

Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.

So this change is actually a semantic change and not just a switch to a
new interface. I think the change is OK (because the caller is domU
only?) but a comment explaining this would be appreciated.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/pci/xen.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 25cd4a0..6432f75 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		goto error;
>  	i = 0;
>  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> +		xen_allocate_pirq_msi(
>  			(type == PCI_CAP_ID_MSIX) ?
> -			"pcifront-msi-x" : "pcifront-msi");
> +			"pcifront-msi-x" : "pcifront-msi",
> +			&irq, &v[i], XEN_ALLOC_IRQ);

All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
differing only in their use of a "pcifront-" prefix. Something which
could be hoisted into the function perhaps?

Another aside: I think there are other cleanups which could be made to
the various pirq allocation/mapping interfaces (of which there seem to
be too many, or at least their individual unique qualities are
indistinguishable by their names), I made a start on this at one point
since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
reasons, but didn't finish it off. I'll hopefully revisit this soon.

Ian.

>  		if (irq < 0) {
>  			ret = -1;
>  			goto free;
>  		}
> -
>  		ret = set_irq_msi(irq, msidesc);
>  		if (ret)
>  			goto error_while;

Re: [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Ian Campbell]

On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> There is no need to use the old interface.

xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
xen_initial_domain() in addition to the kernel side book-keeping side of
things (set chip and handler, update irq_info etc) whereas
xen_allocate_pirq_msi just does the kernel book keeping.

Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.

So this change is actually a semantic change and not just a switch to a
new interface. I think the change is OK (because the caller is domU
only?) but a comment explaining this would be appreciated.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/pci/xen.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 25cd4a0..6432f75 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		goto error;
>  	i = 0;
>  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> +		xen_allocate_pirq_msi(
>  			(type == PCI_CAP_ID_MSIX) ?
> -			"pcifront-msi-x" : "pcifront-msi");
> +			"pcifront-msi-x" : "pcifront-msi",
> +			&irq, &v[i], XEN_ALLOC_IRQ);

All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
differing only in their use of a "pcifront-" prefix. Something which
could be hoisted into the function perhaps?

Another aside: I think there are other cleanups which could be made to
the various pirq allocation/mapping interfaces (of which there seem to
be too many, or at least their individual unique qualities are
indistinguishable by their names), I made a start on this at one point
since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
reasons, but didn't finish it off. I'll hopefully revisit this soon.

Ian.

>  		if (irq < 0) {
>  			ret = -1;
>  			goto free;
>  		}
> -
>  		ret = set_irq_msi(irq, msidesc);
>  		if (ret)
>  			goto error_while;

Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

[Ian Campbell]

On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> Check the returned vector values for any values that are
> odd or plain incorrect (say vector value zero), and if so
> print a warning. Also fixup the return values.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/pci/xen-pcifront.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 3a5a6fc..6acf6ae 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
>  	if (likely(!err)) {
>  		if (likely(!op.value)) {
>  			/* we get the result */
> -			for (i = 0; i < nvec; i++)
> +			for (i = 0; i < nvec; i++) {
> +				if (op.msix_entries[i].vector <= 0) {
> +					dev_warn(&dev->dev, "MSI-X entry %d" \
> +						" is zero!\n", i);

The test says "<= 0" but the text says "== 0". Perhaps
					dev_warn(&dev->dev, "MSI-X entry %d has invalid vector %d\n",
						 i, op.msix_entries[i].vector);

> 
> +					err = -EINVAL;
> +					continue;

Do we need / should we set *(*vector+i) to something to indicate its
invalidness rather than leave it potentially uninitialised?

> +				}
>  				*(*vector+i) = op.msix_entries[i].vector;

BTW does the double indirection of the vector serve a purpose?
Everywhere I can see just updates *(*vector+i), I can't see any realloc
of the array itself etc.

Removing the extra level of indirection leads to vector[i] = foo instead
which is much easier on the eye.

> -			return 0;
> +			}
> +			return err;
>  		} else {
>  			printk(KERN_DEBUG "enable msix get value %x\n",
>  				op.value);
> -			return op.value;
> +			return err;

I think all of these "return err"s can now be pulled out to the end of
the function? makes it clearer what the return is.

>  		}
>  	} else {
>  		dev_err(&dev->dev, "enable msix get err %x\n", err);
> @@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
>  	err = do_pci_op(pdev, &op);
>  	if (likely(!err)) {
>  		*(*vector) = op.value;
> +		if (op.value <= 0) {
> +			dev_warn(&dev->dev, "MSI entry is zero!\n");

Same comment re <= vs == 0.

> +			err = -EINVAL;
> +		}
>  	} else {
>  		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
>  				    "%x:%x\n", op.bus, op.devfn);

Re: [Xen-devel] [PATCH] Xen PCI fronted fixes for 2.6.39

[Konrad Rzeszutek Wilk]

On Thu, Feb 17, 2011 at 08:29:04AM +0000, Ian Campbell wrote:
> On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > I am proposing these three patches for 2.6.39.
> > 
> > The first is to take advantage of the new method of requesting
> > a Linux IRQ and providing the Xen PIRQ value. The second
> > makes it possible for a PV guest to bootup if the backend has provided
> > incorrect values. [*2]
> 
> I approve of being liberal in what is accepted but do we also have a
> handle on why the backend is providing incorrect values in the first
> place?

I got those fixed too - was using 'xen_irq_from_gsi' while
it should have used 'xen_irq_from_pirq'. This was the new mechanism
to obtain the vector values after .. 2.6.38-rc1 ish.. (as the
GSI values now have the correct value of zero, and the vector value
for MSI/MSI-X is written in the PIRQ entry).

But those patches are in a different branch (devel/xen-pciback-0.3).

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Konrad Rzeszutek Wilk]

On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > There is no need to use the old interface.
> 
> xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> xen_initial_domain() in addition to the kernel side book-keeping side of
> things (set chip and handler, update irq_info etc) whereas
> xen_allocate_pirq_msi just does the kernel book keeping.
> 
> Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.

Which is OK. These are MSIs.
> 
> So this change is actually a semantic change and not just a switch to a
> new interface. I think the change is OK (because the caller is domU

Right.

> only?) but a comment explaining this would be appreciated.

Correct: "domU side".

Will fix it up.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  arch/x86/pci/xen.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index 25cd4a0..6432f75 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >  		goto error;
> >  	i = 0;
> >  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> > -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> > +		xen_allocate_pirq_msi(
> >  			(type == PCI_CAP_ID_MSIX) ?
> > -			"pcifront-msi-x" : "pcifront-msi");
> > +			"pcifront-msi-x" : "pcifront-msi",
> > +			&irq, &v[i], XEN_ALLOC_IRQ);
> 
> All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
> differing only in their use of a "pcifront-" prefix. Something which
> could be hoisted into the function perhaps?
> 
> Another aside: I think there are other cleanups which could be made to
> the various pirq allocation/mapping interfaces (of which there seem to
> be too many, or at least their individual unique qualities are
> indistinguishable by their names), I made a start on this at one point
> since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
> reasons, but didn't finish it off. I'll hopefully revisit this soon.
> 
> Ian.
> 
> >  		if (irq < 0) {
> >  			ret = -1;
> >  			goto free;
> >  		}
> > -
> >  		ret = set_irq_msi(irq, msidesc);
> >  		if (ret)
> >  			goto error_while;
> 

Re: [Xen-devel] [PATCH] Xen PCI fronted fixes for 2.6.39

[Ian Campbell]

On Thu, 2011-02-17 at 14:28 +0000, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 08:29:04AM +0000, Ian Campbell wrote:
> > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > I am proposing these three patches for 2.6.39.
> > > 
> > > The first is to take advantage of the new method of requesting
> > > a Linux IRQ and providing the Xen PIRQ value. The second
> > > makes it possible for a PV guest to bootup if the backend has provided
> > > incorrect values. [*2]
> > 
> > I approve of being liberal in what is accepted but do we also have a
> > handle on why the backend is providing incorrect values in the first
> > place?
> 
> I got those fixed too - was using 'xen_irq_from_gsi' while
> it should have used 'xen_irq_from_pirq'. This was the new mechanism
> to obtain the vector values after .. 2.6.38-rc1 ish.. (as the
> GSI values now have the correct value of zero, and the vector value
> for MSI/MSI-X is written in the PIRQ entry).
> 
> But those patches are in a different branch (devel/xen-pciback-0.3).

Thanks, it may be worth co-referencing the front and backend fixes in
their respective commit messages.

Ian.

Re: [PATCH] Xen PCI fronted fixes for 2.6.39

[Ian Campbell]

On Thu, 2011-02-17 at 14:28 +0000, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 08:29:04AM +0000, Ian Campbell wrote:
> > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > I am proposing these three patches for 2.6.39.
> > > 
> > > The first is to take advantage of the new method of requesting
> > > a Linux IRQ and providing the Xen PIRQ value. The second
> > > makes it possible for a PV guest to bootup if the backend has provided
> > > incorrect values. [*2]
> > 
> > I approve of being liberal in what is accepted but do we also have a
> > handle on why the backend is providing incorrect values in the first
> > place?
> 
> I got those fixed too - was using 'xen_irq_from_gsi' while
> it should have used 'xen_irq_from_pirq'. This was the new mechanism
> to obtain the vector values after .. 2.6.38-rc1 ish.. (as the
> GSI values now have the correct value of zero, and the vector value
> for MSI/MSI-X is written in the PIRQ entry).
> 
> But those patches are in a different branch (devel/xen-pciback-0.3).

Thanks, it may be worth co-referencing the front and backend fixes in
their respective commit messages.

Ian.

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Konrad Rzeszutek Wilk]

> All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
> differing only in their use of a "pcifront-" prefix. Something which
> could be hoisted into the function perhaps?

I was thinking to defer this for a cleanup patch.
> 
> Another aside: I think there are other cleanups which could be made to
> the various pirq allocation/mapping interfaces (of which there seem to
> be too many, or at least their individual unique qualities are
> indistinguishable by their names), I made a start on this at one point
> since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
> reasons, but didn't finish it off. I'll hopefully revisit this soon.

Excellent. Looking forward to it.

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Konrad Rzeszutek Wilk]

On Thu, Feb 17, 2011 at 09:30:03AM -0500, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > There is no need to use the old interface.
> > 
> > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > xen_initial_domain() in addition to the kernel side book-keeping side of
> > things (set chip and handler, update irq_info etc) whereas
> > xen_allocate_pirq_msi just does the kernel book keeping.
> > 
> > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> 
> Which is OK. These are MSIs.
> > 
> > So this change is actually a semantic change and not just a switch to a
> > new interface. I think the change is OK (because the caller is domU
> 
> Right.
> 
> > only?) but a comment explaining this would be appreciated.
> 
> Correct: "domU side".
> 
> Will fix it up.

How does this look to you?

>From eb832bece3131ecbdb509f7f2a9bc53f6692177c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 16 Feb 2011 13:43:04 -0500
Subject: [PATCH 3/5] pci/xen: Use xen_allocate_pirq_msi instead of xen_allocate_pirq

xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
xen_initial_domain() in addition to the kernel side book-keeping side of
things (set chip and handler, update irq_info etc) whereas
xen_allocate_pirq_msi just does the kernel book keeping.

Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.

All of this is uneccessary as this code path is only executed
when we run as a domU PV guest with an MSI/MSI-X PCI card passed in.
Hence we can jump straight to allocating an dynamic IRQ (and
binding it to the proper PIRQ) and skip the rest.

In short: this change is a cosmetic one.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/pci/xen.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 25cd4a0..6432f75 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
+		xen_allocate_pirq_msi(
 			(type == PCI_CAP_ID_MSIX) ?
-			"pcifront-msi-x" : "pcifront-msi");
+			"pcifront-msi-x" : "pcifront-msi",
+			&irq, &v[i], XEN_ALLOC_IRQ);
 		if (irq < 0) {
 			ret = -1;
 			goto free;
 		}
-
 		ret = set_irq_msi(irq, msidesc);
 		if (ret)
 			goto error_while;
-- 
1.7.1

Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

[Konrad Rzeszutek Wilk]

On Thu, Feb 17, 2011 at 08:53:40AM +0000, Ian Campbell wrote:
> On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > Check the returned vector values for any values that are
> > odd or plain incorrect (say vector value zero), and if so
> > print a warning. Also fixup the return values.
> > 

How about this one (and there is a cleanup patch shortly following for the *(vector)...)

>From 631b743a5a587d195bf612112026eec8ea649344 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 16 Feb 2011 13:43:22 -0500
Subject: [PATCH 2/5] xen-pcifront: Sanity check the MSI/MSI-X values

Check the returned vector values for any values that are
odd or plain incorrect (say vector value zero), and if so
print a warning. Also fixup the return values.

This patch was precipiated by the Xen PCIBack returning the
incorrect values due to how it was retrieving PIRQ values.
This has been fixed in the xen-pciback by
"xen/pciback: Utilize 'xen_pirq_from_irq' to get PIRQ value"
patch.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 030ce37..d9fd1e0 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -277,18 +277,25 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 	if (likely(!err)) {
 		if (likely(!op.value)) {
 			/* we get the result */
-			for (i = 0; i < nvec; i++)
+			for (i = 0; i < nvec; i++) {
+				if (op.msix_entries[i].vector <= 0) {
+					dev_warn(&dev->dev, "MSI-X entry %d" \
+						" is invalid: %d!\n", i,
+						op.msix_entries[i].vector);
+					err = -EINVAL;
+					*(*vector+i) = -1;
+					continue;
+				}
 				*(*vector+i) = op.msix_entries[i].vector;
-			return 0;
+			}
 		} else {
 			printk(KERN_DEBUG "enable msix get value %x\n",
 				op.value);
-			return op.value;
 		}
 	} else {
 		dev_err(&dev->dev, "enable msix get err %x\n", err);
-		return err;
 	}
+	return err;
 }
 
 static void pci_frontend_disable_msix(struct pci_dev *dev)
@@ -325,6 +332,12 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
 	err = do_pci_op(pdev, &op);
 	if (likely(!err)) {
 		*(*vector) = op.value;
+		if (op.value <= 0) {
+			dev_warn(&dev->dev, "MSI entry is invalid: %d!\n",
+				op.value);
+			err = -EINVAL;
+			*(*vector) = -1;	
+		}
 	} else {
 		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
 				    "%x:%x\n", op.bus, op.devfn);
-- 
1.7.1

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Ian Campbell]

On Fri, 2011-02-18 at 14:07 +0000, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 09:30:03AM -0500, Konrad Rzeszutek Wilk wrote:
> > On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> > > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > > There is no need to use the old interface.
> > > 
> > > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > > xen_initial_domain() in addition to the kernel side book-keeping side of
> > > things (set chip and handler, update irq_info etc) whereas
> > > xen_allocate_pirq_msi just does the kernel book keeping.
> > > 
> > > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> > 
> > Which is OK. These are MSIs.
> > > 
> > > So this change is actually a semantic change and not just a switch to a
> > > new interface. I think the change is OK (because the caller is domU
> > 
> > Right.
> > 
> > > only?) but a comment explaining this would be appreciated.
> > 
> > Correct: "domU side".
> > 
> > Will fix it up.
> 
> How does this look to you?
> 
> From eb832bece3131ecbdb509f7f2a9bc53f6692177c Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 16 Feb 2011 13:43:04 -0500
> Subject: [PATCH 3/5] pci/xen: Use xen_allocate_pirq_msi instead of xen_allocate_pirq
> 
> xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> xen_initial_domain() in addition to the kernel side book-keeping side of
> things (set chip and handler, update irq_info etc) whereas
> xen_allocate_pirq_msi just does the kernel book keeping.
> 
> Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> 
> All of this is uneccessary as this code path is only executed
> when we run as a domU PV guest with an MSI/MSI-X PCI card passed in.
> Hence we can jump straight to allocating an dynamic IRQ (and
> binding it to the proper PIRQ) and skip the rest.
> 
> In short: this change is a cosmetic one.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Looks good,

Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

BTW I'm currently testing a series of further cleanups, to the
xen_create_msi_irq and xen_allocate_pcirq_msi stuff in particular.

I hope to post it today, assuming my test boxes co-operate...

Ian.

> ---
>  arch/x86/pci/xen.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 25cd4a0..6432f75 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		goto error;
>  	i = 0;
>  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> +		xen_allocate_pirq_msi(
>  			(type == PCI_CAP_ID_MSIX) ?
> -			"pcifront-msi-x" : "pcifront-msi");
> +			"pcifront-msi-x" : "pcifront-msi",
> +			&irq, &v[i], XEN_ALLOC_IRQ);
>  		if (irq < 0) {
>  			ret = -1;
>  			goto free;
>  		}
> -
>  		ret = set_irq_msi(irq, msidesc);
>  		if (ret)
>  			goto error_while;

Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Stefano Stabellini]

On Fri, 18 Feb 2011, Ian Campbell wrote:
> On Fri, 2011-02-18 at 14:07 +0000, Konrad Rzeszutek Wilk wrote:
> > On Thu, Feb 17, 2011 at 09:30:03AM -0500, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> > > > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > > > There is no need to use the old interface.
> > > > 
> > > > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > > > xen_initial_domain() in addition to the kernel side book-keeping side of
> > > > things (set chip and handler, update irq_info etc) whereas
> > > > xen_allocate_pirq_msi just does the kernel book keeping.
> > > > 
> > > > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > > > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> > > 
> > > Which is OK. These are MSIs.
> > > > 
> > > > So this change is actually a semantic change and not just a switch to a
> > > > new interface. I think the change is OK (because the caller is domU
> > > 
> > > Right.
> > > 
> > > > only?) but a comment explaining this would be appreciated.
> > > 
> > > Correct: "domU side".
> > > 
> > > Will fix it up.
> > 
> > How does this look to you?
> > 
> > From eb832bece3131ecbdb509f7f2a9bc53f6692177c Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Wed, 16 Feb 2011 13:43:04 -0500
> > Subject: [PATCH 3/5] pci/xen: Use xen_allocate_pirq_msi instead of xen_allocate_pirq
> > 
> > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > xen_initial_domain() in addition to the kernel side book-keeping side of
> > things (set chip and handler, update irq_info etc) whereas
> > xen_allocate_pirq_msi just does the kernel book keeping.
> > 
> > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> > 
> > All of this is uneccessary as this code path is only executed
> > when we run as a domU PV guest with an MSI/MSI-X PCI card passed in.
> > Hence we can jump straight to allocating an dynamic IRQ (and
> > binding it to the proper PIRQ) and skip the rest.
> > 
> > In short: this change is a cosmetic one.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Looks good,
> 
> Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

You can add my reviewed-by too.

Re: [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi

[Stefano Stabellini]

On Fri, 18 Feb 2011, Ian Campbell wrote:
> On Fri, 2011-02-18 at 14:07 +0000, Konrad Rzeszutek Wilk wrote:
> > On Thu, Feb 17, 2011 at 09:30:03AM -0500, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> > > > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > > > There is no need to use the old interface.
> > > > 
> > > > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > > > xen_initial_domain() in addition to the kernel side book-keeping side of
> > > > things (set chip and handler, update irq_info etc) whereas
> > > > xen_allocate_pirq_msi just does the kernel book keeping.
> > > > 
> > > > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > > > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> > > 
> > > Which is OK. These are MSIs.
> > > > 
> > > > So this change is actually a semantic change and not just a switch to a
> > > > new interface. I think the change is OK (because the caller is domU
> > > 
> > > Right.
> > > 
> > > > only?) but a comment explaining this would be appreciated.
> > > 
> > > Correct: "domU side".
> > > 
> > > Will fix it up.
> > 
> > How does this look to you?
> > 
> > From eb832bece3131ecbdb509f7f2a9bc53f6692177c Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Wed, 16 Feb 2011 13:43:04 -0500
> > Subject: [PATCH 3/5] pci/xen: Use xen_allocate_pirq_msi instead of xen_allocate_pirq
> > 
> > xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> > xen_initial_domain() in addition to the kernel side book-keeping side of
> > things (set chip and handler, update irq_info etc) whereas
> > xen_allocate_pirq_msi just does the kernel book keeping.
> > 
> > Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> > xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.
> > 
> > All of this is uneccessary as this code path is only executed
> > when we run as a domU PV guest with an MSI/MSI-X PCI card passed in.
> > Hence we can jump straight to allocating an dynamic IRQ (and
> > binding it to the proper PIRQ) and skip the rest.
> > 
> > In short: this change is a cosmetic one.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Looks good,
> 
> Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

You can add my reviewed-by too.

Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

[Ian Campbell]

On Fri, 2011-02-18 at 14:08 +0000, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 08:53:40AM +0000, Ian Campbell wrote:
> > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > Check the returned vector values for any values that are
> > > odd or plain incorrect (say vector value zero), and if so
> > > print a warning. Also fixup the return values.
> > > 
> 
> How about this one (and there is a cleanup patch shortly following for the *(vector)...)
> 
> From 631b743a5a587d195bf612112026eec8ea649344 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 16 Feb 2011 13:43:22 -0500
> Subject: [PATCH 2/5] xen-pcifront: Sanity check the MSI/MSI-X values
> 
> Check the returned vector values for any values that are
> odd or plain incorrect (say vector value zero), and if so
> print a warning. Also fixup the return values.
> 
> This patch was precipiated by the Xen PCIBack returning the
> incorrect values due to how it was retrieving PIRQ values.
> This has been fixed in the xen-pciback by
> "xen/pciback: Utilize 'xen_pirq_from_irq' to get PIRQ value"
> patch.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/pci/xen-pcifront.c |   21 +++++++++++++++++----
>  1 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 030ce37..d9fd1e0 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -277,18 +277,25 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
>  	if (likely(!err)) {
>  		if (likely(!op.value)) {
>  			/* we get the result */
> -			for (i = 0; i < nvec; i++)
> +			for (i = 0; i < nvec; i++) {
> +				if (op.msix_entries[i].vector <= 0) {
> +					dev_warn(&dev->dev, "MSI-X entry %d" \
> +						" is invalid: %d!\n", i,

String constants are not subject to the 80-column limit in these days
and the preference is to not split them to make them more greppable.

Otherwise:

Reviewed-by: Ian Campbell <ian.campbell@citrix.com>

> +						op.msix_entries[i].vector);
> +					err = -EINVAL;
> +					*(*vector+i) = -1;
> +					continue;
> +				}
>  				*(*vector+i) = op.msix_entries[i].vector;
> -			return 0;
> +			}
>  		} else {
>  			printk(KERN_DEBUG "enable msix get value %x\n",
>  				op.value);
> -			return op.value;
>  		}
>  	} else {
>  		dev_err(&dev->dev, "enable msix get err %x\n", err);
> -		return err;
>  	}
> +	return err;
>  }
>  
>  static void pci_frontend_disable_msix(struct pci_dev *dev)
> @@ -325,6 +332,12 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
>  	err = do_pci_op(pdev, &op);
>  	if (likely(!err)) {
>  		*(*vector) = op.value;
> +		if (op.value <= 0) {
> +			dev_warn(&dev->dev, "MSI entry is invalid: %d!\n",
> +				op.value);
> +			err = -EINVAL;
> +			*(*vector) = -1;	
> +		}
>  	} else {
>  		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
>  				    "%x:%x\n", op.bus, op.devfn);

Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values

[Konrad Rzeszutek Wilk]

On Fri, Feb 18, 2011 at 09:08:45AM -0500, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 17, 2011 at 08:53:40AM +0000, Ian Campbell wrote:
> > On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > > Check the returned vector values for any values that are
> > > odd or plain incorrect (say vector value zero), and if so
> > > print a warning. Also fixup the return values.
> > > 
> 
> How about this one (and there is a cleanup patch shortly following for the *(vector)...)
> 
The cleanup patch:

>From ee7eb70ab9c56a005b6023664c65da1305e3aa56 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 17 Feb 2011 12:02:23 -0500
Subject: [PATCH 4/5] pci/xen: Cleanup: convert int** to int[]

Cleanup code. Cosmetic change to make the code look easier
to read.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/xen/pci.h |    8 ++++----
 arch/x86/pci/xen.c             |    4 ++--
 drivers/pci/xen-pcifront.c     |   12 ++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 2329b3e..aa86209 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -27,16 +27,16 @@ static inline void __init xen_setup_pirqs(void)
  * its own functions.
  */
 struct xen_pci_frontend_ops {
-	int (*enable_msi)(struct pci_dev *dev, int **vectors);
+	int (*enable_msi)(struct pci_dev *dev, int vectors[]);
 	void (*disable_msi)(struct pci_dev *dev);
-	int (*enable_msix)(struct pci_dev *dev, int **vectors, int nvec);
+	int (*enable_msix)(struct pci_dev *dev, int vectors[], int nvec);
 	void (*disable_msix)(struct pci_dev *dev);
 };
 
 extern struct xen_pci_frontend_ops *xen_pci_frontend;
 
 static inline int xen_pci_frontend_enable_msi(struct pci_dev *dev,
-					      int **vectors)
+					      int vectors[])
 {
 	if (xen_pci_frontend && xen_pci_frontend->enable_msi)
 		return xen_pci_frontend->enable_msi(dev, vectors);
@@ -48,7 +48,7 @@ static inline void xen_pci_frontend_disable_msi(struct pci_dev *dev)
 			xen_pci_frontend->disable_msi(dev);
 }
 static inline int xen_pci_frontend_enable_msix(struct pci_dev *dev,
-					       int **vectors, int nvec)
+					       int vectors[], int nvec)
 {
 	if (xen_pci_frontend && xen_pci_frontend->enable_msix)
 		return xen_pci_frontend->enable_msix(dev, vectors, nvec);
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 6432f75..30fdd09 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -150,9 +150,9 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		return -ENOMEM;
 
 	if (type == PCI_CAP_ID_MSIX)
-		ret = xen_pci_frontend_enable_msix(dev, &v, nvec);
+		ret = xen_pci_frontend_enable_msix(dev, v, nvec);
 	else
-		ret = xen_pci_frontend_enable_msi(dev, &v);
+		ret = xen_pci_frontend_enable_msi(dev, v);
 	if (ret)
 		goto error;
 	i = 0;
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index d9fd1e0..27dae44 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -243,7 +243,7 @@ struct pci_ops pcifront_bus_ops = {
 
 #ifdef CONFIG_PCI_MSI
 static int pci_frontend_enable_msix(struct pci_dev *dev,
-				    int **vector, int nvec)
+				    int vector[], int nvec)
 {
 	int err;
 	int i;
@@ -283,10 +283,10 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 						" is invalid: %d!\n", i,
 						op.msix_entries[i].vector);
 					err = -EINVAL;
-					*(*vector+i) = -1;
+					vector[i] = -1;
 					continue;
 				}
-				*(*vector+i) = op.msix_entries[i].vector;
+				vector[i] = op.msix_entries[i].vector;
 			}
 		} else {
 			printk(KERN_DEBUG "enable msix get value %x\n",
@@ -317,7 +317,7 @@ static void pci_frontend_disable_msix(struct pci_dev *dev)
 		dev_err(&dev->dev, "pci_disable_msix get err %x\n", err);
 }
 
-static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
+static int pci_frontend_enable_msi(struct pci_dev *dev, int vector[])
 {
 	int err;
 	struct xen_pci_op op = {
@@ -331,12 +331,12 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, int **vector)
 
 	err = do_pci_op(pdev, &op);
 	if (likely(!err)) {
-		*(*vector) = op.value;
+		vector[0] = op.value;
 		if (op.value <= 0) {
 			dev_warn(&dev->dev, "MSI entry is invalid: %d!\n",
 				op.value);
 			err = -EINVAL;
-			*(*vector) = -1;	
+			vector[0] = -1;
 		}
 	} else {
 		dev_err(&dev->dev, "pci frontend enable msi failed for dev "
-- 
1.7.1

Re: [PATCH] Xen PCI fronted fixes for 2.6.39

[Konrad Rzeszutek Wilk]

On Wed, Feb 16, 2011 at 05:17:15PM -0500, Konrad Rzeszutek Wilk wrote:
> I am proposing these three patches for 2.6.39.
> 
> The first is to take advantage of the new method of requesting
> a Linux IRQ and providing the Xen PIRQ value. The second
> makes it possible for a PV guest to bootup if the backend has provided
> incorrect values. [*2]
> 
> Lastly, the third is to remove deprecated code.

And lets add one more:

>From e4e8523b1d374a3f4276c34e6d017b425ce0d1ae Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 17 Feb 2011 16:12:51 -0500
Subject: [PATCH 5/5] pci/xen: When free-ing MSI-X/MSI irq->desc also use generic code.

This code path is only run when an MSI/MSI-X PCI device is passed
in to PV DomU.

In 2.6.37 time-frame we over-wrote the default cleanup handler for
MSI/MSI-X irq->desc to be "xen_teardown_msi_irqs". That function
calls the the xen-pcifront driver which can tell the backend to
cleanup/take back the MSI/MSI-X device.

However, we forgot to continue the process of free-ing the MSI/MSI-X
device resources (irq->desc) in the PV domU side. Which is what
the default cleanup handler "default_teardown_msi_irqs" did.

Hence we would leak IRQ descriptors.

Without this patch, doing "rmmod igbvf;modprobe igbvf" multiple
times ends with abandoned IRQ descriptors:

 28:          5  xen-pirq-pcifront-msi-x
 29:          8  xen-pirq-pcifront-msi-x
...
130:         10  xen-pirq-pcifront-msi-x

with the end result of running out of IRQ descriptors.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/pci/xen.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 30fdd09..57afd1d 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -193,6 +193,9 @@ static void xen_teardown_msi_irqs(struct pci_dev *dev)
 		xen_pci_frontend_disable_msix(dev);
 	else
 		xen_pci_frontend_disable_msi(dev);
+
+	/* Free the IRQ's and the msidesc using the generic code. */
+	default_teardown_msi_irqs(dev);
 }
 
 static void xen_teardown_msi_irq(unsigned int irq)
-- 
1.7.1

Re: [PATCH] Xen PCI fronted fixes for 2.6.39

[Konrad Rzeszutek Wilk]

On Wed, Feb 16, 2011 at 05:17:15PM -0500, Konrad Rzeszutek Wilk wrote:
> I am proposing these three patches for 2.6.39.
> 
> The first is to take advantage of the new method of requesting
> a Linux IRQ and providing the Xen PIRQ value. The second
> makes it possible for a PV guest to bootup if the backend has provided
> incorrect values. [*2]
> 
> Lastly, the third is to remove deprecated code.

And lets add one more:

>From e4e8523b1d374a3f4276c34e6d017b425ce0d1ae Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 17 Feb 2011 16:12:51 -0500
Subject: [PATCH 5/5] pci/xen: When free-ing MSI-X/MSI irq->desc also use generic code.

This code path is only run when an MSI/MSI-X PCI device is passed
in to PV DomU.

In 2.6.37 time-frame we over-wrote the default cleanup handler for
MSI/MSI-X irq->desc to be "xen_teardown_msi_irqs". That function
calls the the xen-pcifront driver which can tell the backend to
cleanup/take back the MSI/MSI-X device.

However, we forgot to continue the process of free-ing the MSI/MSI-X
device resources (irq->desc) in the PV domU side. Which is what
the default cleanup handler "default_teardown_msi_irqs" did.

Hence we would leak IRQ descriptors.

Without this patch, doing "rmmod igbvf;modprobe igbvf" multiple
times ends with abandoned IRQ descriptors:

 28:          5  xen-pirq-pcifront-msi-x
 29:          8  xen-pirq-pcifront-msi-x
...
130:         10  xen-pirq-pcifront-msi-x

with the end result of running out of IRQ descriptors.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/pci/xen.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 30fdd09..57afd1d 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -193,6 +193,9 @@ static void xen_teardown_msi_irqs(struct pci_dev *dev)
 		xen_pci_frontend_disable_msix(dev);
 	else
 		xen_pci_frontend_disable_msi(dev);
+
+	/* Free the IRQ's and the msidesc using the generic code. */
+	default_teardown_msi_irqs(dev);
 }
 
 static void xen_teardown_msi_irq(unsigned int irq)
-- 
1.7.1

[PATCH 0/12] xen: MSI managment cleanups (Was: Re: [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi)

[Ian Campbell]

On Thu, 2011-02-17 at 14:52 +0000, Konrad Rzeszutek Wilk wrote:
> > All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
> > differing only in their use of a "pcifront-" prefix. Something which
> > could be hoisted into the function perhaps?
> 
> I was thinking to defer this for a cleanup patch.
> > 
> > Another aside: I think there are other cleanups which could be made to
> > the various pirq allocation/mapping interfaces (of which there seem to
> > be too many, or at least their individual unique qualities are
> > indistinguishable by their names), I made a start on this at one point
> > since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
> > reasons, but didn't finish it off. I'll hopefully revisit this soon.
> 
> Excellent. Looking forward to it.

Here is a first pass at some cleanups (based upon your series here).

I think the early parts of this series are reasonably uncontentious.

The second half refactors xen_create_msi_irq and xen_allocate_pirq_msi
with the aim of reducing confusion (i.e. without looking at the code
which of these is a dom0 only function and which is used by PV domU and
PVHVM domU?).

To do this I refactor into xen_allocate_pirq_msi, which simply gets a
free IRQ, and xen_bind_pirq_msi_to_irq, which performs the actual
binding and is common to all domain types.

Tested (lightly) as dom0, PV domU with MSI device passthrough and PVHVM
domU with MSI device passthrough.

In the future I hope to also tackle xen_map_irq_gsi and
xen_allocate_pirq.

[PATCH 01/12] xen: pci: only define xen_initdom_setup_msi_irqs if CONFIG_XEN_DOM0

[Ian Campbell]

Fixes:
 CC      arch/x86/pci/xen.o
arch/x86/pci/xen.c:183: warning: 'xen_initdom_setup_msi_irqs' defined but not used

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 65b2b16..8e80ab3 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -200,6 +200,7 @@ static void xen_teardown_msi_irq(unsigned int irq)
 	xen_destroy_irq(irq);
 }
 
+#ifdef CONFIG_XEN_DOM0
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
 	int irq, ret;
@@ -221,6 +222,7 @@ error:
 	return ret;
 }
 #endif
+#endif
 
 static int xen_pcifront_enable_irq(struct pci_dev *dev)
 {
-- 
1.5.6.5

[PATCH 02/12] xen: events: do not leak IRQ from xen_allocate_pirq_msi when no pirq available.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: xen-devel@lists.xensource.com
---
 drivers/xen/events.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index c536157..41a8a65 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -666,8 +666,11 @@ void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
 
 	if (alloc & XEN_ALLOC_PIRQ) {
 		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
-		if (*pirq == -1)
+		if (*pirq == -1) {
+			xen_free_irq(*irq);
+			*irq = -1;
 			goto out;
+		}
 	}
 
 	set_irq_chip_and_handler_name(*irq, &xen_pirq_chip,
-- 
1.5.6.5

[PATCH 03/12] xen: events: drop XEN_ALLOC_IRQ flag to xen_allocate_pirq_msi

[Ian Campbell]

All callers pass this flag so it is pointless.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |    6 +++---
 drivers/xen/events.c |   12 +++++-------
 include/xen/events.h |    5 +----
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 8e80ab3..d1b72be 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -101,7 +101,7 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 			((msg.address_lo >> MSI_ADDR_DEST_ID_SHIFT) & 0xff);
 		if (xen_irq_from_pirq(pirq) >= 0 && msg.data == XEN_PIRQ_MSI_DATA) {
 			xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-					"msi-x" : "msi", &irq, &pirq, XEN_ALLOC_IRQ);
+					"msi-x" : "msi", &irq, &pirq, 0);
 			if (irq < 0)
 				goto error;
 			ret = set_irq_msi(irq, msidesc);
@@ -112,7 +112,7 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 			return 0;
 		}
 		xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-				"msi-x" : "msi", &irq, &pirq, (XEN_ALLOC_IRQ | XEN_ALLOC_PIRQ));
+				"msi-x" : "msi", &irq, &pirq, 1);
 		if (irq < 0 || pirq < 0)
 			goto error;
 		printk(KERN_DEBUG "xen: msi --> irq=%d, pirq=%d\n", irq, pirq);
@@ -160,7 +160,7 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		xen_allocate_pirq_msi(
 			(type == PCI_CAP_ID_MSIX) ?
 			"pcifront-msi-x" : "pcifront-msi",
-			&irq, &v[i], XEN_ALLOC_IRQ);
+			&irq, &v[i], 0);
 		if (irq < 0) {
 			ret = -1;
 			goto free;
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 41a8a65..e7c6c59 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -654,17 +654,15 @@ static int find_unbound_pirq(int type)
 	return -1;
 }
 
-void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
+void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc_pirq)
 {
 	spin_lock(&irq_mapping_update_lock);
 
-	if (alloc & XEN_ALLOC_IRQ) {
-		*irq = xen_allocate_irq_dynamic();
-		if (*irq == -1)
-			goto out;
-	}
+	*irq = xen_allocate_irq_dynamic();
+	if (*irq == -1)
+		goto out;
 
-	if (alloc & XEN_ALLOC_PIRQ) {
+	if (alloc_pirq) {
 		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
 		if (*pirq == -1) {
 			xen_free_irq(*irq);
diff --git a/include/xen/events.h b/include/xen/events.h
index bd03b1e..88c738c 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -81,10 +81,7 @@ int xen_allocate_pirq(unsigned gsi, int shareable, char *name);
 int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 
 #ifdef CONFIG_PCI_MSI
-/* Allocate an irq and a pirq to be used with MSIs. */
-#define XEN_ALLOC_PIRQ (1 << 0)
-#define XEN_ALLOC_IRQ  (1 << 1)
-void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc_mask);
+void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc_pirq);
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
-- 
1.5.6.5

[PATCH 04/12] xen: events: return irq from xen_allocate_pirq_msi

[Ian Campbell]

consistent with other similar functions.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   12 ++++++------
 drivers/xen/events.c |   19 +++++++++++--------
 include/xen/events.h |    2 +-
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index d1b72be..75a43a9 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -100,8 +100,8 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		pirq = MSI_ADDR_EXT_DEST_ID(msg.address_hi) |
 			((msg.address_lo >> MSI_ADDR_DEST_ID_SHIFT) & 0xff);
 		if (xen_irq_from_pirq(pirq) >= 0 && msg.data == XEN_PIRQ_MSI_DATA) {
-			xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-					"msi-x" : "msi", &irq, &pirq, 0);
+			irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
+						    "msi-x" : "msi", &pirq, 0);
 			if (irq < 0)
 				goto error;
 			ret = set_irq_msi(irq, msidesc);
@@ -111,8 +111,8 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 					" pirq=%d\n", irq, pirq);
 			return 0;
 		}
-		xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-				"msi-x" : "msi", &irq, &pirq, 1);
+		irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
+					    "msi-x" : "msi", &pirq, 1);
 		if (irq < 0 || pirq < 0)
 			goto error;
 		printk(KERN_DEBUG "xen: msi --> irq=%d, pirq=%d\n", irq, pirq);
@@ -157,10 +157,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		xen_allocate_pirq_msi(
+		irq = xen_allocate_pirq_msi(
 			(type == PCI_CAP_ID_MSIX) ?
 			"pcifront-msi-x" : "pcifront-msi",
-			&irq, &v[i], 0);
+			&v[i], 0);
 		if (irq < 0) {
 			ret = -1;
 			goto free;
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index e7c6c59..71d40bf 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -654,31 +654,34 @@ static int find_unbound_pirq(int type)
 	return -1;
 }
 
-void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc_pirq)
+int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq)
 {
+	int irq;
+
 	spin_lock(&irq_mapping_update_lock);
 
-	*irq = xen_allocate_irq_dynamic();
-	if (*irq == -1)
+	irq = xen_allocate_irq_dynamic();
+	if (irq == -1)
 		goto out;
 
 	if (alloc_pirq) {
 		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
 		if (*pirq == -1) {
-			xen_free_irq(*irq);
-			*irq = -1;
+			xen_free_irq(irq);
+			irq = -1;
 			goto out;
 		}
 	}
 
-	set_irq_chip_and_handler_name(*irq, &xen_pirq_chip,
+	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
 				      handle_level_irq, name);
 
-	irq_info[*irq] = mk_pirq_info(0, *pirq, 0, 0);
-	pirq_to_irq[*pirq] = *irq;
+	irq_info[irq] = mk_pirq_info(0, *pirq, 0, 0);
+	pirq_to_irq[*pirq] = irq;
 
 out:
 	spin_unlock(&irq_mapping_update_lock);
+	return irq;
 }
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
diff --git a/include/xen/events.h b/include/xen/events.h
index 88c738c..6c0360a 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -81,7 +81,7 @@ int xen_allocate_pirq(unsigned gsi, int shareable, char *name);
 int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 
 #ifdef CONFIG_PCI_MSI
-void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc_pirq);
+int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq);
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
-- 
1.5.6.5

[PATCH 05/12] xen: pci: collapse apic_register_gsi_xen_hvm and xen_hvm_register_pirq

[Ian Campbell]

apic_register_gsi_xen_hvm is a tiny wrapper around
xen_hvm_register_pirq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 75a43a9..b734358 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -20,7 +20,8 @@
 #include <asm/xen/pci.h>
 
 #ifdef CONFIG_ACPI
-static int xen_hvm_register_pirq(u32 gsi, int triggering)
+static int acpi_register_gsi_xen_hvm(struct device *dev, u32 gsi,
+				 int trigger, int polarity)
 {
 	int rc, irq;
 	struct physdev_map_pirq map_irq;
@@ -41,7 +42,7 @@ static int xen_hvm_register_pirq(u32 gsi, int triggering)
 		return -1;
 	}
 
-	if (triggering == ACPI_EDGE_SENSITIVE) {
+	if (trigger == ACPI_EDGE_SENSITIVE) {
 		shareable = 0;
 		name = "ioapic-edge";
 	} else {
@@ -55,12 +56,6 @@ static int xen_hvm_register_pirq(u32 gsi, int triggering)
 
 	return irq;
 }
-
-static int acpi_register_gsi_xen_hvm(struct device *dev, u32 gsi,
-				 int trigger, int polarity)
-{
-	return xen_hvm_register_pirq(gsi, trigger);
-}
 #endif
 
 #if defined(CONFIG_PCI_MSI)
-- 
1.5.6.5

[PATCH 06/12] xen: events: assume PHYSDEVOP_get_free_pirq exists

[Ian Campbell]

The find_unbound_pirq is called only from xen_allocate_pirq_msi and
only if alloc_pirq is true. The only caller which does this is
xen_hvm_setup_msi_irqs. The use of this function is gated, in
pci_xen_hvm_init, on XENFEAT_hvm_pirqs.

The PHYSDEVOP_get_free_pirq interfaces was added to the hypervisor in
22410:be96f6058c05 while XENFEAT_hvm_pirqs was added a couple of
minutes prior in 22409:6663214f06ac. Therefore we do not need to
concern ourselves with hypervisors which support XENFEAT_hvm_pirqs but
not PHYSDEVOP_get_free_pirq.

This eliminates the fallback path in find_unbound_pirq which walks to
pirq_to_irq array looking for a free pirq. Unlike the
PHYSDEVOP_get_free_pirq interface this fallback only looks up a free
pirq but does not reserve it. Removing this fallback will simplify
locking in the future.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/events.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 71d40bf..9995a1f 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -639,19 +639,16 @@ out:
 
 static int find_unbound_pirq(int type)
 {
-	int rc, i;
+	int rc;
 	struct physdev_get_free_pirq op_get_free_pirq;
-	op_get_free_pirq.type = type;
 
+	op_get_free_pirq.type = type;
 	rc = HYPERVISOR_physdev_op(PHYSDEVOP_get_free_pirq, &op_get_free_pirq);
-	if (!rc)
-		return op_get_free_pirq.pirq;
 
-	for (i = 0; i < nr_irqs; i++) {
-		if (pirq_to_irq[i] < 0)
-			return i;
-	}
-	return -1;
+	WARN_ONCE(rc == -ENOSYS,
+		  "hypervisor does not support the PHYSDEVOP_get_free_pirq interface\n");
+
+	return rc ? -1 : op_get_free_pirq.pirq;
 }
 
 int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq)
-- 
1.5.6.5

[PATCH 07/12] xen: events: separate MSI PIRQ allocation from PIRQ binding to IRQ

[Ian Campbell]

Split the binding aspect of xen_allocate_pirq_msi out into a new
xen_bind_pirq_to_irq function.

In xen_hvm_setup_msi_irq when allocating a pirq write the MSI message
to signal the PIRQ as soon as the pirq is obtained. There is no way to
free the pirq back so if the subsequent binding to an IRQ fails we
want to ensure that we will reuse the PIRQ next time rather than leak
it.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   68 +++++++++++++++++++------------------------------
 drivers/xen/events.c |   30 ++++++++++-----------
 include/xen/events.h |    4 ++-
 3 files changed, 43 insertions(+), 59 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index b734358..2b915c1 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -86,7 +86,7 @@ static void xen_msi_compose_msg(struct pci_dev *pdev, unsigned int pirq,
 
 static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int irq, pirq, ret = 0;
+	int irq, pirq;
 	struct msi_desc *msidesc;
 	struct msi_msg msg;
 
@@ -94,39 +94,32 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		__read_msi_msg(msidesc, &msg);
 		pirq = MSI_ADDR_EXT_DEST_ID(msg.address_hi) |
 			((msg.address_lo >> MSI_ADDR_DEST_ID_SHIFT) & 0xff);
-		if (xen_irq_from_pirq(pirq) >= 0 && msg.data == XEN_PIRQ_MSI_DATA) {
-			irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-						    "msi-x" : "msi", &pirq, 0);
-			if (irq < 0)
+		if (msg.data != XEN_PIRQ_MSI_DATA ||
+		    xen_irq_from_pirq(pirq) < 0) {
+			pirq = xen_allocate_pirq_msi(dev, msidesc);
+			if (pirq < 0)
 				goto error;
-			ret = set_irq_msi(irq, msidesc);
-			if (ret < 0)
-				goto error_while;
-			printk(KERN_DEBUG "xen: msi already setup: msi --> irq=%d"
-					" pirq=%d\n", irq, pirq);
-			return 0;
+			xen_msi_compose_msg(dev, pirq, &msg);
+			__write_msi_msg(msidesc, &msg);
+			dev_dbg(&dev->dev, "xen: msi bound to pirq=%d\n", pirq);
+		} else {
+			dev_dbg(&dev->dev,
+				"xen: msi already bound to pirq=%d\n", pirq);
 		}
-		irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
-					    "msi-x" : "msi", &pirq, 1);
-		if (irq < 0 || pirq < 0)
+		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, pirq,
+					       (type == PCI_CAP_ID_MSIX) ?
+					       "msi-x" : "msi");
+		if (irq < 0)
 			goto error;
-		printk(KERN_DEBUG "xen: msi --> irq=%d, pirq=%d\n", irq, pirq);
-		xen_msi_compose_msg(dev, pirq, &msg);
-		ret = set_irq_msi(irq, msidesc);
-		if (ret < 0)
-			goto error_while;
-		write_msi_msg(irq, &msg);
+		dev_dbg(&dev->dev,
+			"xen: msi --> pirq=%d --> irq=%d\n", pirq, irq);
 	}
 	return 0;
 
-error_while:
-	unbind_from_irqhandler(irq, NULL);
 error:
-	if (ret == -ENODEV)
-		dev_err(&dev->dev, "Xen PCI frontend has not registered" \
-				" MSI/MSI-X support!\n");
-
-	return ret;
+	dev_err(&dev->dev,
+		"Xen PCI frontend has not registered MSI/MSI-X support!\n");
+	return -ENODEV;
 }
 
 /*
@@ -152,28 +145,19 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_allocate_pirq_msi(
-			(type == PCI_CAP_ID_MSIX) ?
-			"pcifront-msi-x" : "pcifront-msi",
-			&v[i], 0);
-		if (irq < 0) {
-			ret = -1;
+		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, v[i],
+					       (type == PCI_CAP_ID_MSIX) ?
+					       "pcifront-msi-x" :
+					       "pcifront-msi");
+		if (irq < 0)
 			goto free;
-		}
-		ret = set_irq_msi(irq, msidesc);
-		if (ret)
-			goto error_while;
 		i++;
 	}
 	kfree(v);
 	return 0;
 
-error_while:
-	unbind_from_irqhandler(irq, NULL);
 error:
-	if (ret == -ENODEV)
-		dev_err(&dev->dev, "Xen PCI frontend has not registered" \
-			" MSI/MSI-X support!\n");
+	dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n");
 free:
 	kfree(v);
 	return ret;
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 9995a1f..6b52beb 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -637,12 +637,12 @@ out:
 #include <linux/msi.h>
 #include "../pci/msi.h"
 
-static int find_unbound_pirq(int type)
+int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc)
 {
 	int rc;
 	struct physdev_get_free_pirq op_get_free_pirq;
 
-	op_get_free_pirq.type = type;
+	op_get_free_pirq.type = MAP_PIRQ_TYPE_MSI;
 	rc = HYPERVISOR_physdev_op(PHYSDEVOP_get_free_pirq, &op_get_free_pirq);
 
 	WARN_ONCE(rc == -ENOSYS,
@@ -651,9 +651,10 @@ static int find_unbound_pirq(int type)
 	return rc ? -1 : op_get_free_pirq.pirq;
 }
 
-int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq)
+int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
+			     int pirq, const char *name)
 {
-	int irq;
+	int irq, ret;
 
 	spin_lock(&irq_mapping_update_lock);
 
@@ -661,24 +662,21 @@ int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq)
 	if (irq == -1)
 		goto out;
 
-	if (alloc_pirq) {
-		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
-		if (*pirq == -1) {
-			xen_free_irq(irq);
-			irq = -1;
-			goto out;
-		}
-	}
-
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
 				      handle_level_irq, name);
 
-	irq_info[irq] = mk_pirq_info(0, *pirq, 0, 0);
-	pirq_to_irq[*pirq] = irq;
-
+	irq_info[irq] = mk_pirq_info(0, pirq, 0, 0);
+	pirq_to_irq[pirq] = irq;
+	ret = set_irq_msi(irq, msidesc);
+	if (ret < 0)
+		goto error_irq;
 out:
 	spin_unlock(&irq_mapping_update_lock);
 	return irq;
+error_irq:
+	spin_unlock(&irq_mapping_update_lock);
+	xen_free_irq(irq);
+	return -1;
 }
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
diff --git a/include/xen/events.h b/include/xen/events.h
index 6c0360a..72310ed 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -81,7 +81,9 @@ int xen_allocate_pirq(unsigned gsi, int shareable, char *name);
 int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 
 #ifdef CONFIG_PCI_MSI
-int xen_allocate_pirq_msi(char *name, int *pirq, int alloc_pirq);
+int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc);
+int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
+			     int pirq, const char *name);
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
-- 
1.5.6.5

[PATCH 08/12] xen: events: refactor xen_create_msi_irq slightly

[Ian Campbell]

Calling PHYSDEVOP_map_pirq earlier simplifies error handling and
starts to make the tail end of this function look like
xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/events.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6b52beb..40780fc 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -706,6 +706,12 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 		map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
 	}
 
+	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
+	if (rc) {
+		dev_warn(&dev->dev, "xen map irq failed %d\n", rc);
+		goto out;
+	}
+
 	spin_lock(&irq_mapping_update_lock);
 
 	irq = xen_allocate_irq_dynamic();
@@ -713,15 +719,6 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	if (irq == -1)
 		goto out;
 
-	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
-	if (rc) {
-		printk(KERN_WARNING "xen map irq failed %d\n", rc);
-
-		xen_free_irq(irq);
-
-		irq = -1;
-		goto out;
-	}
 	irq_info[irq] = mk_pirq_info(0, map_irq.pirq, 0, map_irq.index);
 
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
-- 
1.5.6.5

[PATCH 09/12] xen: events: update pirq_to_irq in xen_create_msi_irq

[Ian Campbell]

I don't think this was a deliberate ommision.

Makes the tail end of this function look even more like
xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/events.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 40780fc..db115ff 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -720,6 +720,7 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 		goto out;
 
 	irq_info[irq] = mk_pirq_info(0, map_irq.pirq, 0, map_irq.index);
+	pirq_to_irq[map_irq.pirq] = irq;
 
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
 			handle_level_irq,
-- 
1.5.6.5

[PATCH 10/12] xen: events: push set_irq_msi down into xen_create_msi_irq

[Ian Campbell]

Makes the tail end of this function look even more like
xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   10 +---------
 drivers/xen/events.c |   10 +++++++++-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 2b915c1..df178a7 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -182,23 +182,15 @@ static void xen_teardown_msi_irq(unsigned int irq)
 #ifdef CONFIG_XEN_DOM0
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int irq, ret;
+	int irq;
 	struct msi_desc *msidesc;
 
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
 		irq = xen_create_msi_irq(dev, msidesc, type);
 		if (irq < 0)
 			return -1;
-
-		ret = set_irq_msi(irq, msidesc);
-		if (ret)
-			goto error;
 	}
 	return 0;
-
-error:
-	xen_destroy_irq(irq);
-	return ret;
 }
 #endif
 #endif
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index db115ff..d4d0987 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -681,7 +681,7 @@ error_irq:
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 {
-	int irq = -1;
+	int ret, irq = -1;
 	struct physdev_map_pirq map_irq;
 	int rc;
 	int pos;
@@ -726,9 +726,17 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 			handle_level_irq,
 			(type == PCI_CAP_ID_MSIX) ? "msi-x":"msi");
 
+	ret = set_irq_msi(irq, msidesc);
+	if (ret)
+		goto out_irq;
+
 out:
 	spin_unlock(&irq_mapping_update_lock);
 	return irq;
+out_irq:
+	spin_unlock(&irq_mapping_update_lock);
+	xen_free_irq(irq);
+	return -1;
 }
 #endif
 
-- 
1.5.6.5

[PATCH 11/12] xen: events: use xen_bind_pirq_msi_to_irq from xen_create_msi_irq

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |    4 ++--
 drivers/xen/events.c |   36 +++++++-----------------------------
 include/xen/events.h |    2 +-
 3 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index df178a7..1a8133a 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -106,7 +106,7 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 			dev_dbg(&dev->dev,
 				"xen: msi already bound to pirq=%d\n", pirq);
 		}
-		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, pirq,
+		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, pirq, 0,
 					       (type == PCI_CAP_ID_MSIX) ?
 					       "msi-x" : "msi");
 		if (irq < 0)
@@ -145,7 +145,7 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		goto error;
 	i = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, v[i],
+		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, v[i], 0,
 					       (type == PCI_CAP_ID_MSIX) ?
 					       "pcifront-msi-x" :
 					       "pcifront-msi");
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index d4d0987..fd3580a 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -652,7 +652,7 @@ int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc)
 }
 
 int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
-			     int pirq, const char *name)
+			     int pirq, int vector, const char *name)
 {
 	int irq, ret;
 
@@ -665,7 +665,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
 				      handle_level_irq, name);
 
-	irq_info[irq] = mk_pirq_info(0, pirq, 0, 0);
+	irq_info[irq] = mk_pirq_info(0, pirq, 0, vector);
 	pirq_to_irq[pirq] = irq;
 	ret = set_irq_msi(irq, msidesc);
 	if (ret < 0)
@@ -681,7 +681,6 @@ error_irq:
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 {
-	int ret, irq = -1;
 	struct physdev_map_pirq map_irq;
 	int rc;
 	int pos;
@@ -709,34 +708,13 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
 	if (rc) {
 		dev_warn(&dev->dev, "xen map irq failed %d\n", rc);
-		goto out;
+		return -1;
 	}
 
-	spin_lock(&irq_mapping_update_lock);
-
-	irq = xen_allocate_irq_dynamic();
-
-	if (irq == -1)
-		goto out;
-
-	irq_info[irq] = mk_pirq_info(0, map_irq.pirq, 0, map_irq.index);
-	pirq_to_irq[map_irq.pirq] = irq;
-
-	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
-			handle_level_irq,
-			(type == PCI_CAP_ID_MSIX) ? "msi-x":"msi");
-
-	ret = set_irq_msi(irq, msidesc);
-	if (ret)
-		goto out_irq;
-
-out:
-	spin_unlock(&irq_mapping_update_lock);
-	return irq;
-out_irq:
-	spin_unlock(&irq_mapping_update_lock);
-	xen_free_irq(irq);
-	return -1;
+	return xen_bind_pirq_msi_to_irq(dev, msidesc,
+					map_irq.pirq, map_irq.index,
+					(type == PCI_CAP_ID_MSIX) ?
+					"msi-x" : "msi");
 }
 #endif
 
diff --git a/include/xen/events.h b/include/xen/events.h
index 72310ed..7151f36 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -83,7 +83,7 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 #ifdef CONFIG_PCI_MSI
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc);
 int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
-			     int pirq, const char *name);
+			     int pirq, int vector, const char *name);
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
-- 
1.5.6.5

[PATCH 12/12] xen: events: remove dom0 specific xen_create_msi_irq

[Ian Campbell]

The function name does not distinguish it from xen_allocate_pirq_msi
(which operates on domU and pvhvm domains rather than dom0).

Hoist domain 0 specific functionality up into the only caller leaving
functionality common to all guest types in xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/xen/events.c |   41 -----------------------------------------
 include/xen/events.h |    1 -
 3 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 1a8133a..513209a 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -182,15 +182,49 @@ static void xen_teardown_msi_irq(unsigned int irq)
 #ifdef CONFIG_XEN_DOM0
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int irq;
+	int ret = 0;
 	struct msi_desc *msidesc;
 
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_create_msi_irq(dev, msidesc, type);
-		if (irq < 0)
-			return -1;
+		struct physdev_map_pirq map_irq;
+
+		memset(&map_irq, 0, sizeof(map_irq));
+		map_irq.domid = DOMID_SELF;
+		map_irq.type = MAP_PIRQ_TYPE_MSI;
+		map_irq.index = -1;
+		map_irq.pirq = -1;
+		map_irq.bus = dev->bus->number;
+		map_irq.devfn = dev->devfn;
+
+		if (type == PCI_CAP_ID_MSIX) {
+			int pos;
+			u32 table_offset, bir;
+
+			pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+
+			pci_read_config_dword(dev, pos + PCI_MSIX_TABLE,
+					      &table_offset);
+			bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
+
+			map_irq.table_base = pci_resource_start(dev, bir);
+			map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
+		}
+
+		ret = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
+		if (ret) {
+			dev_warn(&dev->dev, "xen map irq failed %d\n", ret);
+			goto out;
+		}
+
+		ret = xen_bind_pirq_msi_to_irq(dev, msidesc,
+					       map_irq.pirq, map_irq.index,
+					       (type == PCI_CAP_ID_MSIX) ?
+					       "msi-x" : "msi");
+		if (ret < 0)
+			goto out;
 	}
-	return 0;
+out:
+	return ret;
 }
 #endif
 #endif
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index fd3580a..1a3e760 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -634,9 +634,6 @@ out:
 }
 
 #ifdef CONFIG_PCI_MSI
-#include <linux/msi.h>
-#include "../pci/msi.h"
-
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc)
 {
 	int rc;
@@ -678,44 +675,6 @@ error_irq:
 	xen_free_irq(irq);
 	return -1;
 }
-
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
-{
-	struct physdev_map_pirq map_irq;
-	int rc;
-	int pos;
-	u32 table_offset, bir;
-
-	memset(&map_irq, 0, sizeof(map_irq));
-	map_irq.domid = DOMID_SELF;
-	map_irq.type = MAP_PIRQ_TYPE_MSI;
-	map_irq.index = -1;
-	map_irq.pirq = -1;
-	map_irq.bus = dev->bus->number;
-	map_irq.devfn = dev->devfn;
-
-	if (type == PCI_CAP_ID_MSIX) {
-		pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-
-		pci_read_config_dword(dev, msix_table_offset_reg(pos),
-					&table_offset);
-		bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
-
-		map_irq.table_base = pci_resource_start(dev, bir);
-		map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
-	}
-
-	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
-	if (rc) {
-		dev_warn(&dev->dev, "xen map irq failed %d\n", rc);
-		return -1;
-	}
-
-	return xen_bind_pirq_msi_to_irq(dev, msidesc,
-					map_irq.pirq, map_irq.index,
-					(type == PCI_CAP_ID_MSIX) ?
-					"msi-x" : "msi");
-}
 #endif
 
 int xen_destroy_irq(int irq)
diff --git a/include/xen/events.h b/include/xen/events.h
index 7151f36..d3b9010 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -84,7 +84,6 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc);
 int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 			     int pirq, int vector, const char *name);
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
 /* De-allocates the above mentioned physical interrupt. */
-- 
1.5.6.5

Re: [PATCH 12/12] xen: events: remove dom0 specific xen_create_msi_irq

[Ian Campbell]

On Fri, 2011-02-18 at 16:43 +0000, Ian Campbell wrote:
> The function name does not distinguish it from xen_allocate_pirq_msi
> (which operates on domU and pvhvm domains rather than dom0).
> 
> Hoist domain 0 specific functionality up into the only caller leaving
> functionality common to all guest types in xen_bind_pirq_msi_to_irq.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I missed a "guilt refresh" which added this small patch:
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 513209a..1e72960 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -223,6 +223,7 @@ static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 		if (ret < 0)
 			goto out;
 	}
+	ret = 0;
 out:
 	return ret;
 }

Complete thing:

>From a1b3678a8f5c268855f3d2c94a96b8f8de22bfdf Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 18 Feb 2011 15:46:42 +0000
Subject: [PATCH] xen: events: remove dom0 specific xen_create_msi_irq

The function name does not distinguish it from xen_allocate_pirq_msi
(which operates on domU and pvhvm domains rather than dom0).

Hoist domain 0 specific functionality up into the only caller leaving
functionality common to all guest types in xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   44 +++++++++++++++++++++++++++++++++++++++-----
 drivers/xen/events.c |   41 -----------------------------------------
 include/xen/events.h |    1 -
 3 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 1a8133a..513209a 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -182,15 +182,49 @@ static void xen_teardown_msi_irq(unsigned int irq)
 #ifdef CONFIG_XEN_DOM0
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int irq;
+	int ret = 0;
 	struct msi_desc *msidesc;
 
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_create_msi_irq(dev, msidesc, type);
-		if (irq < 0)
-			return -1;
+		struct physdev_map_pirq map_irq;
+
+		memset(&map_irq, 0, sizeof(map_irq));
+		map_irq.domid = DOMID_SELF;
+		map_irq.type = MAP_PIRQ_TYPE_MSI;
+		map_irq.index = -1;
+		map_irq.pirq = -1;
+		map_irq.bus = dev->bus->number;
+		map_irq.devfn = dev->devfn;
+
+		if (type == PCI_CAP_ID_MSIX) {
+			int pos;
+			u32 table_offset, bir;
+
+			pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+
+			pci_read_config_dword(dev, pos + PCI_MSIX_TABLE,
+					      &table_offset);
+			bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
+
+			map_irq.table_base = pci_resource_start(dev, bir);
+			map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
+		}
+
+		ret = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
+		if (ret) {
+			dev_warn(&dev->dev, "xen map irq failed %d\n", ret);
+			goto out;
+		}
+
+		ret = xen_bind_pirq_msi_to_irq(dev, msidesc,
+					       map_irq.pirq, map_irq.index,
+					       (type == PCI_CAP_ID_MSIX) ?
+					       "msi-x" : "msi");
+		if (ret < 0)
+			goto out;
 	}
-	return 0;
+out:
+	return ret;
 }
 #endif
 #endif
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index fd3580a..1a3e760 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -634,9 +634,6 @@ out:
 }
 
 #ifdef CONFIG_PCI_MSI
-#include <linux/msi.h>
-#include "../pci/msi.h"
-
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc)
 {
 	int rc;
@@ -678,44 +675,6 @@ error_irq:
 	xen_free_irq(irq);
 	return -1;
 }
-
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
-{
-	struct physdev_map_pirq map_irq;
-	int rc;
-	int pos;
-	u32 table_offset, bir;
-
-	memset(&map_irq, 0, sizeof(map_irq));
-	map_irq.domid = DOMID_SELF;
-	map_irq.type = MAP_PIRQ_TYPE_MSI;
-	map_irq.index = -1;
-	map_irq.pirq = -1;
-	map_irq.bus = dev->bus->number;
-	map_irq.devfn = dev->devfn;
-
-	if (type == PCI_CAP_ID_MSIX) {
-		pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-
-		pci_read_config_dword(dev, msix_table_offset_reg(pos),
-					&table_offset);
-		bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
-
-		map_irq.table_base = pci_resource_start(dev, bir);
-		map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
-	}
-
-	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
-	if (rc) {
-		dev_warn(&dev->dev, "xen map irq failed %d\n", rc);
-		return -1;
-	}
-
-	return xen_bind_pirq_msi_to_irq(dev, msidesc,
-					map_irq.pirq, map_irq.index,
-					(type == PCI_CAP_ID_MSIX) ?
-					"msi-x" : "msi");
-}
 #endif
 
 int xen_destroy_irq(int irq)
diff --git a/include/xen/events.h b/include/xen/events.h
index 7151f36..d3b9010 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -84,7 +84,6 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc);
 int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 			     int pirq, int vector, const char *name);
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
 /* De-allocates the above mentioned physical interrupt. */
-- 
1.5.6.5

Re: Re: [PATCH 12/12] xen: events: remove dom0 specific xen_create_msi_irq

[Ian Campbell]

On Fri, 2011-02-18 at 17:00 +0000, Ian Campbell wrote:
> On Fri, 2011-02-18 at 16:43 +0000, Ian Campbell wrote:
> > The function name does not distinguish it from xen_allocate_pirq_msi
> > (which operates on domU and pvhvm domains rather than dom0).
> > 
> > Hoist domain 0 specific functionality up into the only caller leaving
> > functionality common to all guest types in xen_bind_pirq_msi_to_irq.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I missed a "guilt refresh" which added this small patch:
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 513209a..1e72960 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -223,6 +223,7 @@ static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		if (ret < 0)
>  			goto out;
>  	}
> +	ret = 0;
>  out:
>  	return ret;
>  }
> 
> Complete thing:

Forgot to refresh again ;-0

>From f06a411e497bd3310458e539a5144b937697f2ad Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 18 Feb 2011 17:06:18 +0000
Subject: [PATCH] xen: events: remove dom0 specific xen_create_msi_irq

The function name does not distinguish it from xen_allocate_pirq_msi
(which operates on domU and pvhvm domains rather than dom0).

Hoist domain 0 specific functionality up into the only caller leaving
functionality common to all guest types in xen_bind_pirq_msi_to_irq.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/pci/xen.c   |   45 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/xen/events.c |   41 -----------------------------------------
 include/xen/events.h |    1 -
 3 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 1a8133a..1e72960 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -182,15 +182,50 @@ static void xen_teardown_msi_irq(unsigned int irq)
 #ifdef CONFIG_XEN_DOM0
 static int xen_initdom_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int irq;
+	int ret = 0;
 	struct msi_desc *msidesc;
 
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
-		irq = xen_create_msi_irq(dev, msidesc, type);
-		if (irq < 0)
-			return -1;
+		struct physdev_map_pirq map_irq;
+
+		memset(&map_irq, 0, sizeof(map_irq));
+		map_irq.domid = DOMID_SELF;
+		map_irq.type = MAP_PIRQ_TYPE_MSI;
+		map_irq.index = -1;
+		map_irq.pirq = -1;
+		map_irq.bus = dev->bus->number;
+		map_irq.devfn = dev->devfn;
+
+		if (type == PCI_CAP_ID_MSIX) {
+			int pos;
+			u32 table_offset, bir;
+
+			pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+
+			pci_read_config_dword(dev, pos + PCI_MSIX_TABLE,
+					      &table_offset);
+			bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
+
+			map_irq.table_base = pci_resource_start(dev, bir);
+			map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
+		}
+
+		ret = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
+		if (ret) {
+			dev_warn(&dev->dev, "xen map irq failed %d\n", ret);
+			goto out;
+		}
+
+		ret = xen_bind_pirq_msi_to_irq(dev, msidesc,
+					       map_irq.pirq, map_irq.index,
+					       (type == PCI_CAP_ID_MSIX) ?
+					       "msi-x" : "msi");
+		if (ret < 0)
+			goto out;
 	}
-	return 0;
+	ret = 0;
+out:
+	return ret;
 }
 #endif
 #endif
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index fd3580a..1a3e760 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -634,9 +634,6 @@ out:
 }
 
 #ifdef CONFIG_PCI_MSI
-#include <linux/msi.h>
-#include "../pci/msi.h"
-
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc)
 {
 	int rc;
@@ -678,44 +675,6 @@ error_irq:
 	xen_free_irq(irq);
 	return -1;
 }
-
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
-{
-	struct physdev_map_pirq map_irq;
-	int rc;
-	int pos;
-	u32 table_offset, bir;
-
-	memset(&map_irq, 0, sizeof(map_irq));
-	map_irq.domid = DOMID_SELF;
-	map_irq.type = MAP_PIRQ_TYPE_MSI;
-	map_irq.index = -1;
-	map_irq.pirq = -1;
-	map_irq.bus = dev->bus->number;
-	map_irq.devfn = dev->devfn;
-
-	if (type == PCI_CAP_ID_MSIX) {
-		pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-
-		pci_read_config_dword(dev, msix_table_offset_reg(pos),
-					&table_offset);
-		bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
-
-		map_irq.table_base = pci_resource_start(dev, bir);
-		map_irq.entry_nr = msidesc->msi_attrib.entry_nr;
-	}
-
-	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
-	if (rc) {
-		dev_warn(&dev->dev, "xen map irq failed %d\n", rc);
-		return -1;
-	}
-
-	return xen_bind_pirq_msi_to_irq(dev, msidesc,
-					map_irq.pirq, map_irq.index,
-					(type == PCI_CAP_ID_MSIX) ?
-					"msi-x" : "msi");
-}
 #endif
 
 int xen_destroy_irq(int irq)
diff --git a/include/xen/events.h b/include/xen/events.h
index 7151f36..d3b9010 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -84,7 +84,6 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name);
 int xen_allocate_pirq_msi(struct pci_dev *dev, struct msi_desc *msidesc);
 int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 			     int pirq, int vector, const char *name);
-int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type);
 #endif
 
 /* De-allocates the above mentioned physical interrupt. */
-- 
1.5.6.5

Re: [PATCH 02/12] xen: events: do not leak IRQ from xen_allocate_pirq_msi when no pirq available.

[Stefano Stabellini]

On Fri, 18 Feb 2011, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: xen-devel@lists.xensource.com
> ---
>  drivers/xen/events.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index c536157..41a8a65 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -666,8 +666,11 @@ void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
>  
>  	if (alloc & XEN_ALLOC_PIRQ) {
>  		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
> -		if (*pirq == -1)
> +		if (*pirq == -1) {
> +			xen_free_irq(*irq);
> +			*irq = -1;
>  			goto out;
> +		}
>  	}
>  
>  	set_irq_chip_and_handler_name(*irq, &xen_pirq_chip,
 
What if the caller didn't pass XEN_ALLOC_IRQ?
*irq could contain garbage here.
Considering that later on you are removing XEN_ALLOC_IRQ altogether,
maybe you just need to move this patch afterwards.

Re: [PATCH 07/12] xen: events: separate MSI PIRQ allocation from PIRQ binding to IRQ

[Stefano Stabellini]

On Fri, 18 Feb 2011, Ian Campbell wrote:
> Split the binding aspect of xen_allocate_pirq_msi out into a new
> xen_bind_pirq_to_irq function.
> 
> In xen_hvm_setup_msi_irq when allocating a pirq write the MSI message
> to signal the PIRQ as soon as the pirq is obtained. There is no way to
> free the pirq back so if the subsequent binding to an IRQ fails we
> want to ensure that we will reuse the PIRQ next time rather than leak
> it.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/x86/pci/xen.c   |   68 +++++++++++++++++++------------------------------
>  drivers/xen/events.c |   30 ++++++++++-----------
>  include/xen/events.h |    4 ++-
>  3 files changed, 43 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index b734358..2b915c1 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -86,7 +86,7 @@ static void xen_msi_compose_msg(struct pci_dev *pdev, unsigned int pirq,
>  
>  static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
> -	int irq, pirq, ret = 0;
> +	int irq, pirq;
>  	struct msi_desc *msidesc;
>  	struct msi_msg msg;
>  
> @@ -94,39 +94,32 @@ static int xen_hvm_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  		__read_msi_msg(msidesc, &msg);
>  		pirq = MSI_ADDR_EXT_DEST_ID(msg.address_hi) |
>  			((msg.address_lo >> MSI_ADDR_DEST_ID_SHIFT) & 0xff);
> -		if (xen_irq_from_pirq(pirq) >= 0 && msg.data == XEN_PIRQ_MSI_DATA) {
> -			irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
> -						    "msi-x" : "msi", &pirq, 0);
> -			if (irq < 0)
> +		if (msg.data != XEN_PIRQ_MSI_DATA ||
> +		    xen_irq_from_pirq(pirq) < 0) {
> +			pirq = xen_allocate_pirq_msi(dev, msidesc);
> +			if (pirq < 0)
>  				goto error;
> -			ret = set_irq_msi(irq, msidesc);
> -			if (ret < 0)
> -				goto error_while;
> -			printk(KERN_DEBUG "xen: msi already setup: msi --> irq=%d"
> -					" pirq=%d\n", irq, pirq);
> -			return 0;
> +			xen_msi_compose_msg(dev, pirq, &msg);
> +			__write_msi_msg(msidesc, &msg);
> +			dev_dbg(&dev->dev, "xen: msi bound to pirq=%d\n", pirq);
> +		} else {
> +			dev_dbg(&dev->dev,
> +				"xen: msi already bound to pirq=%d\n", pirq);
>  		}
> -		irq = xen_allocate_pirq_msi((type == PCI_CAP_ID_MSIX) ?
> -					    "msi-x" : "msi", &pirq, 1);
> -		if (irq < 0 || pirq < 0)
> +		irq = xen_bind_pirq_msi_to_irq(dev, msidesc, pirq,
> +					       (type == PCI_CAP_ID_MSIX) ?
> +					       "msi-x" : "msi");
> +		if (irq < 0)
>  			goto error;
> -		printk(KERN_DEBUG "xen: msi --> irq=%d, pirq=%d\n", irq, pirq);
> -		xen_msi_compose_msg(dev, pirq, &msg);
> -		ret = set_irq_msi(irq, msidesc);
> -		if (ret < 0)
> -			goto error_while;
> -		write_msi_msg(irq, &msg);
> +		dev_dbg(&dev->dev,
> +			"xen: msi --> pirq=%d --> irq=%d\n", pirq, irq);
>  	}
>  	return 0;

my only concern is about the order of the calls: on native and on xen
before this patch the order is

msi_compose_msg
set_irq_msi
write_msi_msg

while after this patch the order is:

msi_compose_msg
write_msi_msg
set_irq_msi

however I don't think it makes a difference because msi (and msix)
are not enabled yet anyway on the pci device in question (they are
enabled by msi(x)_capability_init right after calling
arch_setup_msi_irqs).

Re: [PATCH 07/12] xen: events: separate MSI PIRQ allocation from PIRQ binding to IRQ

[Ian Campbell]

On Mon, 2011-02-21 at 12:50 +0000, Stefano Stabellini wrote:
> On Fri, 18 Feb 2011, Ian Campbell wrote:

> my only concern is about the order of the calls: on native and on xen
> before this patch the order is
> 
> msi_compose_msg
> set_irq_msi
> write_msi_msg
> 
> while after this patch the order is:
> 
> msi_compose_msg
> write_msi_msg
> set_irq_msi
> 
> however I don't think it makes a difference because msi (and msix)
> are not enabled yet anyway on the pci device in question (they are
> enabled by msi(x)_capability_init right after calling
> arch_setup_msi_irqs).

My take is that write_msi_msg is literally get_irq_msi (to fetch the
corresponding struct msi_desc) followed by __write_msi_msg. The
set_irq_msi does nothing other than associate an msi_desc with an IRQ
number in order that the get_irq_msi can return it.

But since we have the right msi_desc in our hand already using
__write_msi_msg directly is OK and so this ordering is fine.

Ian.

Re: [PATCH 02/12] xen: events: do not leak IRQ from xen_allocate_pirq_msi when no pirq available.

[Ian Campbell]

On Mon, 2011-02-21 at 12:50 +0000, Stefano Stabellini wrote:
> On Fri, 18 Feb 2011, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: xen-devel@lists.xensource.com
> > ---
> >  drivers/xen/events.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index c536157..41a8a65 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -666,8 +666,11 @@ void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
> >  
> >  	if (alloc & XEN_ALLOC_PIRQ) {
> >  		*pirq = find_unbound_pirq(MAP_PIRQ_TYPE_MSI);
> > -		if (*pirq == -1)
> > +		if (*pirq == -1) {
> > +			xen_free_irq(*irq);
> > +			*irq = -1;
> >  			goto out;
> > +		}
> >  	}
> >  
> >  	set_irq_chip_and_handler_name(*irq, &xen_pirq_chip,
>  
> What if the caller didn't pass XEN_ALLOC_IRQ?
> *irq could contain garbage here.
> Considering that later on you are removing XEN_ALLOC_IRQ altogether,
> maybe you just need to move this patch afterwards.

good idea.

Re: [PATCH 09/12] xen: events: update pirq_to_irq in xen_create_msi_irq

[Konrad Rzeszutek Wilk]

On Fri, Feb 18, 2011 at 04:43:34PM +0000, Ian Campbell wrote:
> I don't think this was a deliberate ommision.
> 
> Makes the tail end of this function look even more like
> xen_bind_pirq_msi_to_irq.

I sense a trend here .. :-)

Re: [PATCH 09/12] xen: events: update pirq_to_irq in xen_create_msi_irq

[Ian Campbell]

On Tue, 2011-02-22 at 20:35 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 18, 2011 at 04:43:34PM +0000, Ian Campbell wrote:
> > I don't think this was a deliberate ommision.
> > 
> > Makes the tail end of this function look even more like
> > xen_bind_pirq_msi_to_irq.
> 
> I sense a trend here .. :-)

It's almost like I had an agenda :-)

Re: Re: [PATCH 09/12] xen: events: update pirq_to_irq in xen_create_msi_irq

[Konrad Rzeszutek Wilk]

On Wed, Feb 23, 2011 at 01:20:28PM +0000, Ian Campbell wrote:
> On Tue, 2011-02-22 at 20:35 +0000, Konrad Rzeszutek Wilk wrote:
> > On Fri, Feb 18, 2011 at 04:43:34PM +0000, Ian Campbell wrote:
> > > I don't think this was a deliberate ommision.
> > > 
> > > Makes the tail end of this function look even more like
> > > xen_bind_pirq_msi_to_irq.
> > 
> > I sense a trend here .. :-)
> 
> It's almost like I had an agenda :-)

The force is strong with your agenda... so I put your patches in
#stable/irq.cleanup and merged it in #linux-next.

Was the question that Stefano had answered?

Also tested on dom0, PV, PV+PCI, PVonHVM, PVonHVM+PCI with a 82576 and 82574 Intel NIC.

Re: Re: [PATCH 09/12] xen: events: update pirq_to_irq in xen_create_msi_irq

[Ian Campbell]

On Wed, 2011-02-23 at 15:08 +0000, Konrad Rzeszutek Wilk wrote:
> On Wed, Feb 23, 2011 at 01:20:28PM +0000, Ian Campbell wrote:
> > On Tue, 2011-02-22 at 20:35 +0000, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Feb 18, 2011 at 04:43:34PM +0000, Ian Campbell wrote:
> > > > I don't think this was a deliberate ommision.
> > > > 
> > > > Makes the tail end of this function look even more like
> > > > xen_bind_pirq_msi_to_irq.
> > > 
> > > I sense a trend here .. :-)
> > 
> > It's almost like I had an agenda :-)
> 
> The force is strong with your agenda... so I put your patches in
> #stable/irq.cleanup and merged it in #linux-next.

Thanks.

> Was the question that Stefano had answered?

> Also tested on dom0, PV, PV+PCI, PVonHVM, PVonHVM+PCI with a 82576 and 82574 Intel NIC.

Nice one, cheers!

Ian.