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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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