[PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[Ian Campbell]

The following series changes the Xen event channel infrastructure to
make better use of the core IRQ allocation interfaces and simplifies
our attempts to avoid clashes between GSIs and event channels.

The two most interesting changes are:

      * PV domU IRQ numbers are now completely dynamically allocated,
        including those associated with PCI passthrough devices. Since
        the guest sees no equivalent to GSI space there is no need to do
        anything clever in this case like maintaining a 1-1 mapping
        between GSI and IRQ. (this is impossible anyway since a PV guest
        has no idea what the largest possible GSI is).

      * PV dom0 and HVM guests now completely segregate GSIs from
        dynamically allocated IRQs. In this case IRQs for GSIs are
        allocated 1-1 beneath nr_irq_gsi (similar to native) and dynamic
        IRQs (event channels, MSIs etc) are allocated above. This
        simplifies the IRQ allocator code enormously.

The series has been tested as:
      * PV guest with a PCI passthrough device.
      * PV domain 0.
      * HVM guest with and without XENFEAT_hvm_pirqs.

It appears to do the correct thing in each case.

I also tried HVM with PCI passthrough, which wasn't too successful due
to my hardware not having an IOMMU, but it did appear to setup the IRQ
mapping correctly at least.

I don't intend this for 2.6.38 at this stage since it looks like:
        67b0ea2bdcd7 xen/irq: Don't fall over when nr_irqs_gsi > nr_irqs.
        d1b758ebc2a8 xen/irq: Cleanup the find_unbound_irq
are sufficient (BTW this series follows those).

Ian.

[PATCH 1/4] xen: handled remapped IRQs when enabling a pcifront PCI device.

[Ian Campbell]

This happens to not be an issue currently because we take pains to try
to ensure that the GSI-IRQ mapping is 1-1 in a PV guest and that
regular event channels do not clash. However a subsequent patch is
going to break this 1-1 mapping.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 arch/x86/pci/xen.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 25cd4a0..2a12f3d 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -226,21 +226,27 @@ static int xen_pcifront_enable_irq(struct pci_dev *dev)
 {
 	int rc;
 	int share = 1;
+	u8 gsi;
 
-	dev_info(&dev->dev, "Xen PCI enabling IRQ: %d\n", dev->irq);
-
-	if (dev->irq < 0)
-		return -EINVAL;
+	rc = pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &gsi);
+	if (rc < 0) {
+		dev_warn(&dev->dev, "Xen PCI: failed to read interrupt line: %d\n",
+			 rc);
+		return rc;
+	}
 
-	if (dev->irq < NR_IRQS_LEGACY)
+	if (gsi < NR_IRQS_LEGACY)
 		share = 0;
 
-	rc = xen_allocate_pirq(dev->irq, share, "pcifront");
+	rc = xen_allocate_pirq(gsi, share, "pcifront");
 	if (rc < 0) {
-		dev_warn(&dev->dev, "Xen PCI IRQ: %d, failed to register:%d\n",
-			 dev->irq, rc);
+		dev_warn(&dev->dev, "Xen PCI: failed to register GSI%d: %d\n",
+			 gsi, rc);
 		return rc;
 	}
+
+	dev->irq = rc;
+	dev_info(&dev->dev, "Xen PCI mapped GSI%d to IRQ%d\n", gsi, dev->irq);
 	return 0;
 }
 
-- 
1.5.6.5

[PATCH 2/4] xen:events: move find_unbound_irq inside CONFIG_PCI_MSI

[Ian Campbell]

The only caller is xen_allocate_pirq_msi which is also under this
ifdef so this fixes:
    drivers/xen/events.c:377: warning: 'find_unbound_pirq' defined but not used
when CONFIG_PCI_MSI=n

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/xen/events.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 98b7220..ae8d45d 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -384,23 +384,6 @@ static int get_nr_hw_irqs(void)
 	return ret;
 }
 
-static int find_unbound_pirq(int type)
-{
-	int rc, i;
-	struct physdev_get_free_pirq op_get_free_pirq;
-	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;
-}
-
 static int find_unbound_irq(void)
 {
 	struct irq_data *data;
@@ -683,6 +666,23 @@ out:
 #include <linux/msi.h>
 #include "../pci/msi.h"
 
+static int find_unbound_pirq(int type)
+{
+	int rc, i;
+	struct physdev_get_free_pirq op_get_free_pirq;
+	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;
+}
+
 void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
 {
 	spin_lock(&irq_mapping_update_lock);
-- 
1.5.6.5

[PATCH 3/4] xen: events: add xen_allocate_irq_{dynamic, gsi} and xen_free_irq

[Ian Campbell]

This is neater than open-coded calls to irq_alloc_desc_at and
irq_free_desc.

No intended behavioural change.

Note that we previously were not checking the return value of
irq_alloc_desc_at which would be failing for GSI<NR_IRQS_LEGACY
because the core architecture code has already allocated those for
us. Hence the additional check against NR_IRQS_LEGACY in
xen_allocate_irq_gsi.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/xen/events.c |   53 +++++++++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index ae8d45d..74fb216 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -384,7 +384,7 @@ static int get_nr_hw_irqs(void)
 	return ret;
 }
 
-static int find_unbound_irq(void)
+static int xen_allocate_irq_dynamic(void)
 {
 	struct irq_data *data;
 	int irq, res;
@@ -442,6 +442,30 @@ static bool identity_mapped_irq(unsigned irq)
 	return irq < get_nr_hw_irqs();
 }
 
+static int xen_allocate_irq_gsi(unsigned gsi)
+{
+	int irq;
+
+	if (!identity_mapped_irq(gsi) &&
+	    (xen_initial_domain() || !xen_pv_domain()))
+		return xen_allocate_irq_dynamic();
+
+	/* Legacy IRQ descriptors are already allocated by the arch. */
+	if (gsi < NR_IRQS_LEGACY)
+		return gsi;
+
+	irq = irq_alloc_desc_at(gsi, -1);
+	if (irq < 0)
+		panic("Unable to allocate to IRQ%d (%d)\n", gsi, irq);
+
+	return irq;
+}
+
+static void xen_free_irq(unsigned irq)
+{
+	irq_free_desc(irq);
+}
+
 static void pirq_unmask_notify(int irq)
 {
 	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
@@ -627,14 +651,7 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name)
 		goto out;	/* XXX need refcount? */
 	}
 
-	/* If we are a PV guest, we don't have GSIs (no ACPI passed). Therefore
-	 * we are using the !xen_initial_domain() to drop in the function.*/
-	if (identity_mapped_irq(gsi) || (!xen_initial_domain() &&
-				xen_pv_domain())) {
-		irq = gsi;
-		irq_alloc_desc_at(irq, -1);
-	} else
-		irq = find_unbound_irq();
+	irq = xen_allocate_irq_gsi(gsi);
 
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
 				      handle_level_irq, name);
@@ -647,7 +664,7 @@ int xen_map_pirq_gsi(unsigned pirq, unsigned gsi, int shareable, char *name)
 	 * this in the priv domain. */
 	if (xen_initial_domain() &&
 	    HYPERVISOR_physdev_op(PHYSDEVOP_alloc_irq_vector, &irq_op)) {
-		irq_free_desc(irq);
+		xen_free_irq(irq);
 		irq = -ENOSPC;
 		goto out;
 	}
@@ -688,7 +705,7 @@ void xen_allocate_pirq_msi(char *name, int *irq, int *pirq, int alloc)
 	spin_lock(&irq_mapping_update_lock);
 
 	if (alloc & XEN_ALLOC_IRQ) {
-		*irq = find_unbound_irq();
+		*irq = xen_allocate_irq_dynamic();
 		if (*irq == -1)
 			goto out;
 	}
@@ -738,7 +755,7 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 
 	spin_lock(&irq_mapping_update_lock);
 
-	irq = find_unbound_irq();
+	irq = xen_allocate_irq_dynamic();
 
 	if (irq == -1)
 		goto out;
@@ -747,7 +764,7 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	if (rc) {
 		printk(KERN_WARNING "xen map irq failed %d\n", rc);
 
-		irq_free_desc(irq);
+		xen_free_irq(irq);
 
 		irq = -1;
 		goto out;
@@ -789,7 +806,7 @@ int xen_destroy_irq(int irq)
 	}
 	irq_info[irq] = mk_unbound_info();
 
-	irq_free_desc(irq);
+	xen_free_irq(irq);
 
 out:
 	spin_unlock(&irq_mapping_update_lock);
@@ -820,7 +837,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 	irq = evtchn_to_irq[evtchn];
 
 	if (irq == -1) {
-		irq = find_unbound_irq();
+		irq = xen_allocate_irq_dynamic();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
 					      handle_fasteoi_irq, "event");
@@ -845,7 +862,7 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 	irq = per_cpu(ipi_to_irq, cpu)[ipi];
 
 	if (irq == -1) {
-		irq = find_unbound_irq();
+		irq = xen_allocate_irq_dynamic();
 		if (irq < 0)
 			goto out;
 
@@ -881,7 +898,7 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
 	irq = per_cpu(virq_to_irq, cpu)[virq];
 
 	if (irq == -1) {
-		irq = find_unbound_irq();
+		irq = xen_allocate_irq_dynamic();
 
 		set_irq_chip_and_handler_name(irq, &xen_percpu_chip,
 					      handle_percpu_irq, "virq");
@@ -940,7 +957,7 @@ static void unbind_from_irq(unsigned int irq)
 	if (irq_info[irq].type != IRQT_UNBOUND) {
 		irq_info[irq] = mk_unbound_info();
 
-		irq_free_desc(irq);
+		xen_free_irq(irq);
 	}
 
 	spin_unlock(&irq_mapping_update_lock);
-- 
1.5.6.5

[PATCH 4/4] xen: events: allocate GSIs and dynamic IRQs from separate IRQ ranges.

[Ian Campbell]

There are three cases which we need to care about, PV guest, PV domain
0 and HVM guest.

The PV guest case is simple since it has no access to ACPI or real
APICs and therefore has no GSIs therefore we simply dynamically
allocate all IRQs. The potentially interesting case here is PIRQ type
event channels associated with passed through PCI devices. However
even in this case the guest has no direct interaction with the
physical GSI since that happens in the PCI backend.

The PV domain 0 and HVM guest cases are actually the same. In domain 0
case the kernel sees the host ACPI and GSIs (although it only sees the
APIC indirectly via the hypervisor) and in the HVM guest case it sees
the virtualised ACPI and emulated APICs. In these cases we start
allocating dynamic IRQs at nr_irqs_gsi so that they cannot clash with
any GSI.

Currently xen_allocate_irq_dynamic starts at nr_irqs and works
backwards looking for a free IRQ in order to (try and) avoid clashing
with GSIs used in domain 0 and in HVM guests. This change avoids that
although we retain the behaviour of allowing dynamic IRQs to encroach
on the GSI range if no suitable IRQs are available since a future IRQ
clash is deemed preferable to failure right now.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 drivers/xen/events.c |   84 +++++++++++++++----------------------------------
 1 files changed, 26 insertions(+), 58 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 74fb216..a7b60f6 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -373,81 +373,49 @@ static void unmask_evtchn(int port)
 	put_cpu();
 }
 
-static int get_nr_hw_irqs(void)
+static int xen_allocate_irq_dynamic(void)
 {
-	int ret = 1;
+	int first = 0;
+	int irq;
 
 #ifdef CONFIG_X86_IO_APIC
-	ret = get_nr_irqs_gsi();
+	/*
+	 * For an HVM guest or domain 0 which see "real" (emulated or
+	 * actual repectively) GSIs we allocate dynamic IRQs
+	 * e.g. those corresponding to event channels or MSIs
+	 * etc. from the range above those "real" GSIs to avoid
+	 * collisions.
+	 */
+	if (xen_initial_domain() || xen_hvm_domain())
+		first = get_nr_irqs_gsi();
 #endif
 
-	return ret;
-}
-
-static int xen_allocate_irq_dynamic(void)
-{
-	struct irq_data *data;
-	int irq, res;
-	int bottom = get_nr_hw_irqs();
-	int top = nr_irqs-1;
-
-	if (bottom == nr_irqs)
-		goto no_irqs;
-
 retry:
-	/* This loop starts from the top of IRQ space and goes down.
-	 * We need this b/c if we have a PCI device in a Xen PV guest
-	 * we do not have an IO-APIC (though the backend might have them)
-	 * mapped in. To not have a collision of physical IRQs with the Xen
-	 * event channels start at the top of the IRQ space for virtual IRQs.
-	 */
-	for (irq = top; irq > bottom; irq--) {
-		data = irq_get_irq_data(irq);
-		/* only 15->0 have init'd desc; handle irq > 16 */
-		if (!data)
-			break;
-		if (data->chip == &no_irq_chip)
-			break;
-		if (data->chip != &xen_dynamic_chip)
-			continue;
-		if (irq_info[irq].type == IRQT_UNBOUND)
-			return irq;
-	}
+	irq = irq_alloc_desc_from(first, -1);
 
-	if (irq == bottom)
-		goto no_irqs;
-
-	res = irq_alloc_desc_at(irq, -1);
-	if (res == -EEXIST) {
-		top--;
-		if (bottom > top)
-			printk(KERN_ERR "Eating in GSI/MSI space (%d)!" \
-				" Your PCI device might not work!\n", top);
-		if (top > NR_IRQS_LEGACY)
-			goto retry;
+	if (irq == -ENOMEM && first > NR_IRQS_LEGACY) {
+		printk(KERN_ERR "Out of dynamic IRQ space and eating into GSI space. You should increase nr_irqs\n");
+		first = max(NR_IRQS_LEGACY, first - NR_IRQS_LEGACY);
+		goto retry;
 	}
 
-	if (WARN_ON(res != irq))
-		return -1;
+	if (irq < 0)
+		panic("No available IRQ to bind to: increase nr_irqs!\n");
 
 	return irq;
-
-no_irqs:
-	panic("No available IRQ to bind to: increase nr_irqs!\n");
-}
-
-static bool identity_mapped_irq(unsigned irq)
-{
-	/* identity map all the hardware irqs */
-	return irq < get_nr_hw_irqs();
 }
 
 static int xen_allocate_irq_gsi(unsigned gsi)
 {
 	int irq;
 
-	if (!identity_mapped_irq(gsi) &&
-	    (xen_initial_domain() || !xen_pv_domain()))
+	/*
+	 * A PV guest has no concept of a GSI (since it has no ACPI
+	 * nor access to/knowledge of the physical APICs). Therefore
+	 * all IRQs are dynamically allocated from the entire IRQ
+	 * space.
+	 */
+	if (xen_pv_domain() && !xen_initial_domain())
 		return xen_allocate_irq_dynamic();
 
 	/* Legacy IRQ descriptors are already allocated by the arch. */
-- 
1.5.6.5

Re: [PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[Konrad Rzeszutek Wilk]

On Tue, Jan 11, 2011 at 05:19:49PM +0000, Ian Campbell wrote:
> The following series changes the Xen event channel infrastructure to
> make better use of the core IRQ allocation interfaces and simplifies
> our attempts to avoid clashes between GSIs and event channels.
> 
> The two most interesting changes are:
> 
>       * PV domU IRQ numbers are now completely dynamically allocated,
>         including those associated with PCI passthrough devices. Since
>         the guest sees no equivalent to GSI space there is no need to do
>         anything clever in this case like maintaining a 1-1 mapping
>         between GSI and IRQ. (this is impossible anyway since a PV guest
>         has no idea what the largest possible GSI is).
> 
>       * PV dom0 and HVM guests now completely segregate GSIs from
>         dynamically allocated IRQs. In this case IRQs for GSIs are
>         allocated 1-1 beneath nr_irq_gsi (similar to native) and dynamic
>         IRQs (event channels, MSIs etc) are allocated above. This
>         simplifies the IRQ allocator code enormously.
> 
> The series has been tested as:
>       * PV guest with a PCI passthrough device.

Which type of domain0? A 2.6.37 + Xen PCI backend + your patches?
Or the older 2.6.32?
>       * PV domain 0.
>       * HVM guest with and without XENFEAT_hvm_pirqs.
> 
> It appears to do the correct thing in each case.

Nice..
> 
> I also tried HVM with PCI passthrough, which wasn't too successful due
> to my hardware not having an IOMMU, but it did appear to setup the IRQ
> mapping correctly at least.
> 
> I don't intend this for 2.6.38 at this stage since it looks like:
>         67b0ea2bdcd7 xen/irq: Don't fall over when nr_irqs_gsi > nr_irqs.
>         d1b758ebc2a8 xen/irq: Cleanup the find_unbound_irq
> are sufficient (BTW this series follows those).
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 3/4] xen: events: add xen_allocate_irq_{dynamic, gsi} and xen_free_irq

[Konrad Rzeszutek Wilk]

On Tue, Jan 11, 2011 at 05:20:15PM +0000, Ian Campbell wrote:
> This is neater than open-coded calls to irq_alloc_desc_at and
> irq_free_desc.
> 
> No intended behavioural change.
> 
> Note that we previously were not checking the return value of
> irq_alloc_desc_at which would be failing for GSI<NR_IRQS_LEGACY
> because the core architecture code has already allocated those for
> us. Hence the additional check against NR_IRQS_LEGACY in
> xen_allocate_irq_gsi.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
>  drivers/xen/events.c |   53 +++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index ae8d45d..74fb216 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -384,7 +384,7 @@ static int get_nr_hw_irqs(void)
>  	return ret;
>  }
>  
> -static int find_unbound_irq(void)
> +static int xen_allocate_irq_dynamic(void)
>  {
>  	struct irq_data *data;
>  	int irq, res;
> @@ -442,6 +442,30 @@ static bool identity_mapped_irq(unsigned irq)
>  	return irq < get_nr_hw_irqs();
>  }
>  
> +static int xen_allocate_irq_gsi(unsigned gsi)
> +{
> +	int irq;
> +
> +	if (!identity_mapped_irq(gsi) &&
> +	    (xen_initial_domain() || !xen_pv_domain()))

Perhaps 'xen_hvm_domain()' would sound better? That way there
are less _not_ expressions to think through?

> +		return xen_allocate_irq_dynamic();

Ok, so this ends up allocating an IRQ for all non-physical
IRQs, such as the spinlock, call IPI, and so on, correct?

> +
> +	/* Legacy IRQ descriptors are already allocated by the arch. */
> +	if (gsi < NR_IRQS_LEGACY)
> +		return gsi;
> +
> +	irq = irq_alloc_desc_at(gsi, -1);
> +	if (irq < 0)
> +		panic("Unable to allocate to IRQ%d (%d)\n", gsi, irq);
> +
> +	return irq;
> +}
> +
> +static void xen_free_irq(unsigned irq)
> +{
> +	irq_free_desc(irq);
This is still OK even if the IRQ is < NR_IRQS_LEGACY? You mention
"Legacy IRQ descriptors are already allocated by the arch" so I would
think that the arch would take care of de-allocating those?

Re: [PATCH 4/4] xen: events: allocate GSIs and dynamic IRQs from separate IRQ ranges.

[Konrad Rzeszutek Wilk]

On Tue, Jan 11, 2011 at 05:20:16PM +0000, Ian Campbell wrote:
> There are three cases which we need to care about, PV guest, PV domain
> 0 and HVM guest.
> 
> The PV guest case is simple since it has no access to ACPI or real
> APICs and therefore has no GSIs therefore we simply dynamically
> allocate all IRQs. The potentially interesting case here is PIRQ type
> event channels associated with passed through PCI devices. However
> even in this case the guest has no direct interaction with the
> physical GSI since that happens in the PCI backend.
> 
> The PV domain 0 and HVM guest cases are actually the same. In domain 0
> case the kernel sees the host ACPI and GSIs (although it only sees the
> APIC indirectly via the hypervisor) and in the HVM guest case it sees
> the virtualised ACPI and emulated APICs. In these cases we start
> allocating dynamic IRQs at nr_irqs_gsi so that they cannot clash with
> any GSI.
> 
> Currently xen_allocate_irq_dynamic starts at nr_irqs and works
> backwards looking for a free IRQ in order to (try and) avoid clashing
> with GSIs used in domain 0 and in HVM guests. This change avoids that
> although we retain the behaviour of allowing dynamic IRQs to encroach
> on the GSI range if no suitable IRQs are available since a future IRQ
> clash is deemed preferable to failure right now.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
>  drivers/xen/events.c |   84 +++++++++++++++----------------------------------
>  1 files changed, 26 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 74fb216..a7b60f6 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -373,81 +373,49 @@ static void unmask_evtchn(int port)
>  	put_cpu();
>  }
>  
> -static int get_nr_hw_irqs(void)
> +static int xen_allocate_irq_dynamic(void)
>  {
> -	int ret = 1;
> +	int first = 0;
> +	int irq;
>  
>  #ifdef CONFIG_X86_IO_APIC
> -	ret = get_nr_irqs_gsi();
> +	/*
> +	 * For an HVM guest or domain 0 which see "real" (emulated or
> +	 * actual repectively) GSIs we allocate dynamic IRQs
> +	 * e.g. those corresponding to event channels or MSIs
> +	 * etc. from the range above those "real" GSIs to avoid
> +	 * collisions.
> +	 */
> +	if (xen_initial_domain() || xen_hvm_domain())
> +		first = get_nr_irqs_gsi();
>  #endif
>  
> -	return ret;
> -}
> -
> -static int xen_allocate_irq_dynamic(void)
> -{
> -	struct irq_data *data;
> -	int irq, res;
> -	int bottom = get_nr_hw_irqs();
> -	int top = nr_irqs-1;
> -
> -	if (bottom == nr_irqs)
> -		goto no_irqs;
> -
>  retry:
> -	/* This loop starts from the top of IRQ space and goes down.
> -	 * We need this b/c if we have a PCI device in a Xen PV guest
> -	 * we do not have an IO-APIC (though the backend might have them)
> -	 * mapped in. To not have a collision of physical IRQs with the Xen
> -	 * event channels start at the top of the IRQ space for virtual IRQs.
> -	 */
> -	for (irq = top; irq > bottom; irq--) {
> -		data = irq_get_irq_data(irq);
> -		/* only 15->0 have init'd desc; handle irq > 16 */
> -		if (!data)
> -			break;
> -		if (data->chip == &no_irq_chip)
> -			break;
> -		if (data->chip != &xen_dynamic_chip)
> -			continue;
> -		if (irq_info[irq].type == IRQT_UNBOUND)
> -			return irq;
> -	}
> +	irq = irq_alloc_desc_from(first, -1);
>  
> -	if (irq == bottom)
> -		goto no_irqs;
> -
> -	res = irq_alloc_desc_at(irq, -1);
> -	if (res == -EEXIST) {
> -		top--;
> -		if (bottom > top)
> -			printk(KERN_ERR "Eating in GSI/MSI space (%d)!" \
> -				" Your PCI device might not work!\n", top);
> -		if (top > NR_IRQS_LEGACY)
> -			goto retry;
> +	if (irq == -ENOMEM && first > NR_IRQS_LEGACY) {
> +		printk(KERN_ERR "Out of dynamic IRQ space and eating into GSI space. You should increase nr_irqs\n");
> +		first = max(NR_IRQS_LEGACY, first - NR_IRQS_LEGACY);
> +		goto retry;

You don't check for irq == -EEXIST. So if specific IRQ (first) is already
occupied you panic. Would it be better to check for that too in this got
and retry with that value?

>  	}
>  
> -	if (WARN_ON(res != irq))
> -		return -1;
> +	if (irq < 0)
> +		panic("No available IRQ to bind to: increase nr_irqs!\n");
>  
>  	return irq;
> -
> -no_irqs:
> -	panic("No available IRQ to bind to: increase nr_irqs!\n");
> -}
> -
> -static bool identity_mapped_irq(unsigned irq)
> -{
> -	/* identity map all the hardware irqs */
> -	return irq < get_nr_hw_irqs();
>  }
>  
>  static int xen_allocate_irq_gsi(unsigned gsi)
>  {
>  	int irq;
>  
> -	if (!identity_mapped_irq(gsi) &&
> -	    (xen_initial_domain() || !xen_pv_domain()))
> +	/*
> +	 * A PV guest has no concept of a GSI (since it has no ACPI
> +	 * nor access to/knowledge of the physical APICs). Therefore
> +	 * all IRQs are dynamically allocated from the entire IRQ
> +	 * space.
> +	 */
> +	if (xen_pv_domain() && !xen_initial_domain())
>  		return xen_allocate_irq_dynamic();

OK. So with a IDE disk at IRQ 14 it can end up with irq = 5 (say the first five
IRQs are used by xen-spinlock, xen-timer, xen-debug, xen-console, and something else).
The gsi that gets stuck via the mk_pirq_info[5] ends up being 14, and pirq = 14.
When you do EVTCHNOP_bind_pirq you end up passing in pirq=14 and bind that to the
Linux irq five, right?


>  
>  	/* Legacy IRQ descriptors are already allocated by the arch. */
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[Ian Campbell]

On Tue, 2011-01-11 at 18:34 +0000, Konrad Rzeszutek Wilk wrote:
> 
> > The series has been tested as:
> >       * PV guest with a PCI passthrough device.
> 
> Which type of domain0? A 2.6.37 + Xen PCI backend + your patches?
> Or the older 2.6.32?

The 2.6.32 variant. Same for the HVM test. Dom 0 was 2.6.37 + patches.

> >       * PV domain 0.
> >       * HVM guest with and without XENFEAT_hvm_pirqs. 

Re: [PATCH 3/4] xen: events: add xen_allocate_irq_{dynamic, gsi} and xen_free_irq

[Ian Campbell]

On Tue, 2011-01-11 at 18:46 +0000, Konrad Rzeszutek Wilk wrote:
> > +     if (!identity_mapped_irq(gsi) &&
> > +         (xen_initial_domain() || !xen_pv_domain()))
> 
> Perhaps 'xen_hvm_domain()' would sound better? That way there
> are less _not_ expressions to think through?

This is deliberately just the inverse of the test I removed from the
callsite in xen_map_pirq_gsi, modulo an application or two of De
Morgan's:

-       /* If we are a PV guest, we don't have GSIs (no ACPI passed). Therefore
-        * we are using the !xen_initial_domain() to drop in the function.*/
-       if (identity_mapped_irq(gsi) || (!xen_initial_domain() &&
-                               xen_pv_domain())) {
-               irq = gsi;
-               irq_alloc_desc_at(irq, -1);
-       } else
-               irq = find_unbound_irq();
+       irq = xen_allocate_irq_gsi(gsi);

This patch is just the refactoring step before the meat of the change in
the following patch where this complex expression goes away. 

> > +             return xen_allocate_irq_dynamic();
> 
> Ok, so this ends up allocating an IRQ for all non-physical
> IRQs, such as the spinlock, call IPI, and so on, correct? 

The overall effect should be identical to before this patch. 

> > +static void xen_free_irq(unsigned irq)
> > +{
> > +     irq_free_desc(irq);
> This is still OK even if the IRQ is < NR_IRQS_LEGACY? You mention
> "Legacy IRQ descriptors are already allocated by the arch" so I would
> think that the arch would take care of de-allocating those?

Hmm. Interesting question. I suspect you are right but I can't think
howto convince the system to deallocate such an interrupt anyway. I'll
dig around the code a little and convince myself as best I can that
adding a check+return there is correct.

Ian.

Re: [PATCH 4/4] xen: events: allocate GSIs and dynamic IRQs from separate IRQ ranges.

[Ian Campbell]

On Tue, 2011-01-11 at 19:14 +0000, Konrad Rzeszutek Wilk wrote: 
> On Tue, Jan 11, 2011 at 05:20:16PM +0000, Ian Campbell wrote:

> >  retry:
> > -	/* This loop starts from the top of IRQ space and goes down.
> > -	 * We need this b/c if we have a PCI device in a Xen PV guest
> > -	 * we do not have an IO-APIC (though the backend might have them)
> > -	 * mapped in. To not have a collision of physical IRQs with the Xen
> > -	 * event channels start at the top of the IRQ space for virtual IRQs.
> > -	 */
> > -	for (irq = top; irq > bottom; irq--) {
> > -		data = irq_get_irq_data(irq);
> > -		/* only 15->0 have init'd desc; handle irq > 16 */
> > -		if (!data)
> > -			break;
> > -		if (data->chip == &no_irq_chip)
> > -			break;
> > -		if (data->chip != &xen_dynamic_chip)
> > -			continue;
> > -		if (irq_info[irq].type == IRQT_UNBOUND)
> > -			return irq;
> > -	}
> > +	irq = irq_alloc_desc_from(first, -1);
> >  
> > -	if (irq == bottom)
> > -		goto no_irqs;
> > -
> > -	res = irq_alloc_desc_at(irq, -1);
> > -	if (res == -EEXIST) {
> > -		top--;
> > -		if (bottom > top)
> > -			printk(KERN_ERR "Eating in GSI/MSI space (%d)!" \
> > -				" Your PCI device might not work!\n", top);
> > -		if (top > NR_IRQS_LEGACY)
> > -			goto retry;
> > +	if (irq == -ENOMEM && first > NR_IRQS_LEGACY) {
> > +		printk(KERN_ERR "Out of dynamic IRQ space and eating into GSI space. You should increase nr_irqs\n");
> > +		first = max(NR_IRQS_LEGACY, first - NR_IRQS_LEGACY);
> > +		goto retry;
> 
> You don't check for irq == -EEXIST. So if specific IRQ (first) is already
> occupied you panic. Would it be better to check for that too in this got
> and retry with that value?

It's not obvious due to the way diff has chosen to represent this change
but this is checking the return value of irq_alloc_desc_from and not
irq_alloc_desc_at. The former cannot return EEXIST.

> >  	}
> >  
> > -	if (WARN_ON(res != irq))
> > -		return -1;
> > +	if (irq < 0)
> > +		panic("No available IRQ to bind to: increase nr_irqs!\n");
> >  
> >  	return irq;
> > -
> > -no_irqs:
> > -	panic("No available IRQ to bind to: increase nr_irqs!\n");
> > -}
> > -
> > -static bool identity_mapped_irq(unsigned irq)
> > -{
> > -	/* identity map all the hardware irqs */
> > -	return irq < get_nr_hw_irqs();
> >  }
> >  
> >  static int xen_allocate_irq_gsi(unsigned gsi)
> >  {
> >  	int irq;
> >  
> > -	if (!identity_mapped_irq(gsi) &&
> > -	    (xen_initial_domain() || !xen_pv_domain()))
> > +	/*
> > +	 * A PV guest has no concept of a GSI (since it has no ACPI
> > +	 * nor access to/knowledge of the physical APICs). Therefore
> > +	 * all IRQs are dynamically allocated from the entire IRQ
> > +	 * space.
> > +	 */
> > +	if (xen_pv_domain() && !xen_initial_domain())
> >  		return xen_allocate_irq_dynamic();
> 
> OK. So with a IDE disk at IRQ 14 it can end up with irq = 5 (say the first five
> IRQs are used by xen-spinlock, xen-timer, xen-debug, xen-console, and something else).
> The gsi that gets stuck via the mk_pirq_info[5] ends up being 14, and pirq = 14.
> When you do EVTCHNOP_bind_pirq you end up passing in pirq=14 and bind that to the
> Linux irq five, right?

Actually because the core registers the first NR_IRQS_LEGACY interrupts
the PV interrupts actually start at 16 so 5 is a bad example but the
gist is otherwise correct, yes.

Ian.

Re: [PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[Konrad Rzeszutek Wilk]

On Tue, Jan 11, 2011 at 07:25:18PM +0000, Ian Campbell wrote:
> On Tue, 2011-01-11 at 18:34 +0000, Konrad Rzeszutek Wilk wrote:
> > 
> > > The series has been tested as:
> > >       * PV guest with a PCI passthrough device.
> > 
> > Which type of domain0? A 2.6.37 + Xen PCI backend + your patches?
> > Or the older 2.6.32?
> 
> The 2.6.32 variant. Same for the HVM test. Dom 0 was 2.6.37 + patches.

Ok. Could you test the PV guest with a PCI passthrough device
on a PV domain 0 that is 2.6.37 based? It _should_ work
but it never hurts to try.

For your convience I've put up a merge tree with stable/* patches,
devel/irq.rework (has mine and these patches you have posted),
devel/xen-pciback, devel/gntdev (Stefano's last posting)

It is devel/next-2.6.37

Re: [PATCH 0/4] xen: events: improve event channel IRQ allocation strategy.

[Ian Campbell]

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

On Tue, 2011-01-11 at 20:40 +0000, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 11, 2011 at 07:25:18PM +0000, Ian Campbell wrote:
> > On Tue, 2011-01-11 at 18:34 +0000, Konrad Rzeszutek Wilk wrote:
> > > 
> > > > The series has been tested as:
> > > >       * PV guest with a PCI passthrough device.
> > > 
> > > Which type of domain0? A 2.6.37 + Xen PCI backend + your patches?
> > > Or the older 2.6.32?
> > 
> > The 2.6.32 variant. Same for the HVM test. Dom 0 was 2.6.37 + patches.
> 
> Ok. Could you test the PV guest with a PCI passthrough device
> on a PV domain 0 that is 2.6.37 based? It _should_ work
> but it never hurts to try.
> 
> For your convience I've put up a merge tree with stable/* patches,
> devel/irq.rework (has mine and these patches you have posted),
> devel/xen-pciback, devel/gntdev (Stefano's last posting)
> 
> It is devel/next-2.6.37

I tested using this kernel as dom0 and my previous 2.6.37+patches as PV
domU and PCI passthrough seemed to work ok. In particular:
[    0.380253] uhci_hcd 0000:00:00.0: enabling device (0000 -> 0001)
[    0.380606] uhci_hcd 0000:00:00.0: Xen PCI mapped GSI20 to IRQ44
[    0.380669] uhci_hcd 0000:00:00.0: enabling bus mastering
[    0.380794] uhci_hcd 0000:00:00.0: setting latency timer to 64
[    0.380844] uhci_hcd 0000:00:00.0: UHCI Host Controller
[    0.381369] uhci_hcd 0000:00:00.0: new USB bus registered, assigned bus number 1

It didn't stop me testing but I got a bunch of spew in the form of ten
score of these earlish in the boot of devel/next-2.6.37 as dom0:

WARNING: at /local/scratch/ianc/devel/kernels/linux-2.6/arch/x86/xen/multicalls.c:182 xen_mc_flush+0x293/0x2a0()
Hardware name: PowerEdge 860
Modules linked in:
Pid: 0, comm: swapper Tainted: G        W   2.6.37-x86_32p-xen0-00105-g4abcf5c #99
Call Trace:
 [<c1003c33>] ? xen_mc_flush+0x293/0x2a0
 [<c1003c33>] ? xen_mc_flush+0x293/0x2a0
 [<c103ef7c>] warn_slowpath_common+0x6c/0xa0
 [<c1003c33>] ? xen_mc_flush+0x293/0x2a0
 [<c103efcd>] warn_slowpath_null+0x1d/0x20
 [<c1003c33>] xen_mc_flush+0x293/0x2a0
 [<c1006597>] ? xen_set_domain_pte+0x57/0x100
 [<c10065df>] xen_set_domain_pte+0x9f/0x100
 [<c1003e00>] ? __raw_callee_save_xen_pte_val+0x0/0x8
 [<c1006716>] xen_set_pte+0x86/0x90
 [<c1003e00>] ? __raw_callee_save_xen_pte_val+0x0/0x8
 [<c1562e8b>] xen_set_pte_init+0x8a/0x96
 [<c1571825>] kernel_physical_mapping_init+0x2d3/0x3de
 [<c138117e>] init_memory_mapping+0x27e/0x4f0
 [<c156461e>] setup_arch+0x74d/0xcc4
 [<c139fa7f>] ? _raw_spin_unlock_irqrestore+0x3f/0x70
 [<c103fd2d>] ? vprintk+0x2ad/0x470
 [<c1069c28>] ? trace_hardirqs_off_caller+0xa8/0x140
 [<c1006990>] ? __raw_callee_save_xen_save_fl+0x0/0x8
 [<c1006998>] ? __raw_callee_save_xen_restore_fl+0x0/0x8
 [<c1069c28>] ? trace_hardirqs_off_caller+0xa8/0x140
 [<c1175712>] ? __raw_spin_lock_init+0x32/0x60
 [<c1002640>] ? xen_cpuid+0x0/0xa0
 [<c1002640>] ? xen_cpuid+0x0/0xa0
 [<c155e72c>] start_kernel+0x8b/0x381
 [<c155e0b3>] i386_start_kernel+0xa2/0xde
 [<c156175a>] xen_start_kernel+0x5fa/0x6b0
---[ end trace 4eaa2a86a8e4bf7c ]---

Full bootlog is attached.


[-- Attachment #2: bootlog.gz --]
[-- Type: application/x-gzip, Size: 17931 bytes --]

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

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

[PATCH] xen: events: do not free legacy IRQs

[Ian Campbell]

c514d00c8057 "xen: events: add xen_allocate_irq_{dynamic, gsi} and
xen_free_irq" correctly avoids reallocating legacy IRQs (which are
managed by the arch core) but erroneously did not prevent them being
freed.

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

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index ff044e0..eb7d47f 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -434,6 +434,10 @@ static int xen_allocate_irq_gsi(unsigned gsi)
 
 static void xen_free_irq(unsigned irq)
 {
+	/* Legacy IRQ descriptors are managed by the arch. */
+	if (irq < NR_IRQS_LEGACY)
+		return irq;
+
 	irq_free_desc(irq);
 }
 
-- 
1.5.6.5

Re: [PATCH] xen: events: do not free legacy IRQs

[Ian Campbell]

On Thu, 2011-02-03 at 08:30 +0000, Ian Campbell wrote:
> c514d00c8057 "xen: events: add xen_allocate_irq_{dynamic, gsi} and
> xen_free_irq" correctly avoids reallocating legacy IRQs (which are
> managed by the arch core) but erroneously did not prevent them being
> freed.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/xen/events.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index ff044e0..eb7d47f 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -434,6 +434,10 @@ static int xen_allocate_irq_gsi(unsigned gsi)
>  
>  static void xen_free_irq(unsigned irq)
>  {
> +	/* Legacy IRQ descriptors are managed by the arch. */
> +	if (irq < NR_IRQS_LEGACY)
> +		return irq;

Hmm, didn't notice the warning when I compiled this... Should obviously
just be "return;"

Ian.

8<--------------------

>From 84cc1a94c7eec94b94f1973cd84c017ae36c3c5f Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Thu, 3 Feb 2011 08:23:10 +0000
Subject: [PATCH] xen: events: do not free legacy IRQs

c514d00c8057 "xen: events: add xen_allocate_irq_{dynamic, gsi} and
xen_free_irq" correctly avoids reallocating legacy IRQs (which are
managed by the arch core) but erroneously did not prevent them being
freed.

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

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index ff044e0..eb7d47f 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -434,6 +434,10 @@ static int xen_allocate_irq_gsi(unsigned gsi)
 
 static void xen_free_irq(unsigned irq)
 {
+	/* Legacy IRQ descriptors are managed by the arch. */
+	if (irq < NR_IRQS_LEGACY)
+		return;
+
 	irq_free_desc(irq);
 }
 
-- 
1.5.6.5