Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376409 > unrolled thread

Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before  allocating descs for legacy IRQs Stefano Stabellini <sstabellini@kernel.org> - 2016-04-12 04:10 +0200
    Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-04-12 10:40 +0200
    Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-12 15:30 +0200
      Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Stefano Stabellini <sstabellini@kernel.org> - 2016-04-12 20:10 +0200
        Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-12 21:10 +0200
          Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Stefano Stabellini <sstabellini@kernel.org> - 2016-04-12 23:20 +0200
            Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-12 23:40 +0200
              Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-13 00:40 +0200
                Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Stefano Stabellini <sstabellini@kernel.org> - 2016-04-13 01:20 +0200
                  Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-13 03:30 +0200
                    Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-13 19:40 +0200
                      Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-13 21:20 +0200
                        Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-04-14 00:30 +0200
                        Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Stefano Stabellini <sstabellini@kernel.org> - 2016-04-14 00:40 +0200
                          Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-14 14:50 +0200
        Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs David Vrabel <david.vrabel@citrix.com> - 2016-04-14 15:50 +0200
          Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence  before allocating descs for legacy IRQs Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-14 16:50 +0200

#1376409 — Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

Hi all,

Unfortunately this patch (now commit
8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
when running on top of QEMU: the number of PIT irqs get set to 0 by
probe_8259A but actually there are 16.

Any suggestions on how to fix this?

1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
2) we could introduce an 'if (!xen_domain())' in probe_8259A
3) suggestions welcome


On Mon, 2 Nov 2015, Vitaly Kuznetsov wrote:
> Commit d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain
> interfaces") brought a regression for Hyper-V Gen2 instances. These
> instances don't have i8259 legacy PIC but they use legacy IRQs for serial
> port, rtc, and acpi. With this commit included we end up with these IRQs
> not initialized. Earlier, there was a special workaround for legacy IRQs
> in mp_map_pin_to_irq() doing mp_irqdomain_map() without looking at
> nr_legacy_irqs() and now we fail in __irq_domain_alloc_irqs() when
> irq_domain_alloc_descs() returns -EEXIST.
> 
> The essence of the issue seems to be that early_irq_init() calls
> arch_probe_nr_irqs() to figure out the number of legacy IRQs before
> we probe for i8259 and gets 16. Later when init_8259A() is called we switch
> to NULL legacy PIC and nr_legacy_irqs() starts to return 0 but we already
> have 16 descs allocated.
> 
> Solve the issue by separating i8259 probe from init and calling it in
> arch_probe_nr_irqs() before we actually use nr_legacy_irqs() information.
> 
> Fixes: d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/i8259.h  |  1 +
>  arch/x86/kernel/apic/vector.c |  3 +++
>  arch/x86/kernel/i8259.c       | 24 ++++++++++++++++--------
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/i8259.h b/arch/x86/include/asm/i8259.h
> index ccffa53..bd55a77 100644
> --- a/arch/x86/include/asm/i8259.h
> +++ b/arch/x86/include/asm/i8259.h
> @@ -60,6 +60,7 @@ struct legacy_pic {
>  	void (*mask_all)(void);
>  	void (*restore_mask)(void);
>  	void (*init)(int auto_eoi);
> +	void (*probe)(void);
>  	int (*irq_pending)(unsigned int irq);
>  	void (*make_irq)(unsigned int irq);
>  };
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 836d11b..aadd7ae 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -361,6 +361,9 @@ int __init arch_probe_nr_irqs(void)
>  	if (nr < nr_irqs)
>  		nr_irqs = nr;
>  
> +	/* nr_legecy_irqs() depends on the PIC presence */
> +	legacy_pic->probe();
> +
>  	return nr_legacy_irqs();
>  }
>  
> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
> index 16cb827..96f1562 100644
> --- a/arch/x86/kernel/i8259.c
> +++ b/arch/x86/kernel/i8259.c
> @@ -295,16 +295,11 @@ static void unmask_8259A(void)
>  	raw_spin_unlock_irqrestore(&i8259A_lock, flags);
>  }
>  
> -static void init_8259A(int auto_eoi)
> +static void probe_8259A(void)
>  {
>  	unsigned long flags;
>  	unsigned char probe_val = ~(1 << PIC_CASCADE_IR);
>  	unsigned char new_val;
> -
> -	i8259A_auto_eoi = auto_eoi;
> -
> -	raw_spin_lock_irqsave(&i8259A_lock, flags);
> -
>  	/*
>  	 * Check to see if we have a PIC.
>  	 * Mask all except the cascade and read
> @@ -312,16 +307,27 @@ static void init_8259A(int auto_eoi)
>  	 * have a PIC, we will read 0xff as opposed to the
>  	 * value we wrote.
>  	 */
> +	raw_spin_lock_irqsave(&i8259A_lock, flags);
> +
>  	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
>  	outb(probe_val, PIC_MASTER_IMR);
>  	new_val = inb(PIC_MASTER_IMR);
>  	if (new_val != probe_val) {
>  		printk(KERN_INFO "Using NULL legacy PIC\n");
>  		legacy_pic = &null_legacy_pic;
> -		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> -		return;
>  	}
>  
> +	raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> +}
> +
> +static void init_8259A(int auto_eoi)
> +{
> +	unsigned long flags;
> +
> +	i8259A_auto_eoi = auto_eoi;
> +
> +	raw_spin_lock_irqsave(&i8259A_lock, flags);
> +
>  	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
>  
>  	/*
> @@ -388,6 +394,7 @@ struct legacy_pic null_legacy_pic = {
>  	.mask_all = legacy_pic_noop,
>  	.restore_mask = legacy_pic_noop,
>  	.init = legacy_pic_int_noop,
> +	.probe = legacy_pic_noop,
>  	.irq_pending = legacy_pic_irq_pending_noop,
>  	.make_irq = legacy_pic_uint_noop,
>  };
> @@ -400,6 +407,7 @@ struct legacy_pic default_legacy_pic = {
>  	.mask_all = mask_8259A,
>  	.restore_mask = unmask_8259A,
>  	.init = init_8259A,
> +	.probe = probe_8259A,
>  	.irq_pending = i8259A_irq_pending,
>  	.make_irq = make_8259A_irq,
>  };
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[toc] | [next] | [standalone]

#1376565 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

Stefano Stabellini <sstabellini@kernel.org> writes:

> Hi all,
>
> Unfortunately this patch (now commit
> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
> when running on top of QEMU: the number of PIT irqs get set to 0 by
> probe_8259A but actually there are 16.
>

How would one see the regression?
8c058b0b9c34d8c8d7912880956543769323e2d8 is present since 4.4 and HVM
guests seem to work.

Other than that, why does probe_8259A() lie?

> Any suggestions on how to fix this?
>
> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8

This would re-introduce the original issue I was fixing.

> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
> 3) suggestions welcome

I'd suggest we make probe_8259A() work. It can only return 0 if PIC
probe by outb()/inb() fails. Why does it fail on QEMU?

>
> On Mon, 2 Nov 2015, Vitaly Kuznetsov wrote:
>> Commit d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain
>> interfaces") brought a regression for Hyper-V Gen2 instances. These
>> instances don't have i8259 legacy PIC but they use legacy IRQs for serial
>> port, rtc, and acpi. With this commit included we end up with these IRQs
>> not initialized. Earlier, there was a special workaround for legacy IRQs
>> in mp_map_pin_to_irq() doing mp_irqdomain_map() without looking at
>> nr_legacy_irqs() and now we fail in __irq_domain_alloc_irqs() when
>> irq_domain_alloc_descs() returns -EEXIST.
>> 
>> The essence of the issue seems to be that early_irq_init() calls
>> arch_probe_nr_irqs() to figure out the number of legacy IRQs before
>> we probe for i8259 and gets 16. Later when init_8259A() is called we switch
>> to NULL legacy PIC and nr_legacy_irqs() starts to return 0 but we already
>> have 16 descs allocated.
>> 
>> Solve the issue by separating i8259 probe from init and calling it in
>> arch_probe_nr_irqs() before we actually use nr_legacy_irqs() information.
>> 
>> Fixes: d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/include/asm/i8259.h  |  1 +
>>  arch/x86/kernel/apic/vector.c |  3 +++
>>  arch/x86/kernel/i8259.c       | 24 ++++++++++++++++--------
>>  3 files changed, 20 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/i8259.h b/arch/x86/include/asm/i8259.h
>> index ccffa53..bd55a77 100644
>> --- a/arch/x86/include/asm/i8259.h
>> +++ b/arch/x86/include/asm/i8259.h
>> @@ -60,6 +60,7 @@ struct legacy_pic {
>>  	void (*mask_all)(void);
>>  	void (*restore_mask)(void);
>>  	void (*init)(int auto_eoi);
>> +	void (*probe)(void);
>>  	int (*irq_pending)(unsigned int irq);
>>  	void (*make_irq)(unsigned int irq);
>>  };
>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index 836d11b..aadd7ae 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -361,6 +361,9 @@ int __init arch_probe_nr_irqs(void)
>>  	if (nr < nr_irqs)
>>  		nr_irqs = nr;
>>  
>> +	/* nr_legecy_irqs() depends on the PIC presence */
>> +	legacy_pic->probe();
>> +
>>  	return nr_legacy_irqs();
>>  }
>>  
>> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
>> index 16cb827..96f1562 100644
>> --- a/arch/x86/kernel/i8259.c
>> +++ b/arch/x86/kernel/i8259.c
>> @@ -295,16 +295,11 @@ static void unmask_8259A(void)
>>  	raw_spin_unlock_irqrestore(&i8259A_lock, flags);
>>  }
>>  
>> -static void init_8259A(int auto_eoi)
>> +static void probe_8259A(void)
>>  {
>>  	unsigned long flags;
>>  	unsigned char probe_val = ~(1 << PIC_CASCADE_IR);
>>  	unsigned char new_val;
>> -
>> -	i8259A_auto_eoi = auto_eoi;
>> -
>> -	raw_spin_lock_irqsave(&i8259A_lock, flags);
>> -
>>  	/*
>>  	 * Check to see if we have a PIC.
>>  	 * Mask all except the cascade and read
>> @@ -312,16 +307,27 @@ static void init_8259A(int auto_eoi)
>>  	 * have a PIC, we will read 0xff as opposed to the
>>  	 * value we wrote.
>>  	 */
>> +	raw_spin_lock_irqsave(&i8259A_lock, flags);
>> +
>>  	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
>>  	outb(probe_val, PIC_MASTER_IMR);
>>  	new_val = inb(PIC_MASTER_IMR);
>>  	if (new_val != probe_val) {
>>  		printk(KERN_INFO "Using NULL legacy PIC\n");
>>  		legacy_pic = &null_legacy_pic;
>> -		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
>> -		return;
>>  	}
>>  
>> +	raw_spin_unlock_irqrestore(&i8259A_lock, flags);
>> +}
>> +
>> +static void init_8259A(int auto_eoi)
>> +{
>> +	unsigned long flags;
>> +
>> +	i8259A_auto_eoi = auto_eoi;
>> +
>> +	raw_spin_lock_irqsave(&i8259A_lock, flags);
>> +
>>  	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
>>  
>>  	/*
>> @@ -388,6 +394,7 @@ struct legacy_pic null_legacy_pic = {
>>  	.mask_all = legacy_pic_noop,
>>  	.restore_mask = legacy_pic_noop,
>>  	.init = legacy_pic_int_noop,
>> +	.probe = legacy_pic_noop,
>>  	.irq_pending = legacy_pic_irq_pending_noop,
>>  	.make_irq = legacy_pic_uint_noop,
>>  };
>> @@ -400,6 +407,7 @@ struct legacy_pic default_legacy_pic = {
>>  	.mask_all = mask_8259A,
>>  	.restore_mask = unmask_8259A,
>>  	.init = init_8259A,
>> +	.probe = probe_8259A,
>>  	.irq_pending = i8259A_irq_pending,
>>  	.make_irq = make_8259A_irq,
>>  };
>> -- 
>> 2.4.3
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>> 

-- 
  Vitaly

[toc] | [prev] | [next] | [standalone]

#1376796 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
> Hi all,
>
> Unfortunately this patch (now commit
> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
> when running on top of QEMU: the number of PIT irqs get set to 0 by
> probe_8259A but actually there are 16.
>
> Any suggestions on how to fix this?
>
> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
> 3) suggestions welcome

Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?

It was supposed to fix this problem for Xen. However, I just noticed 
that arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 
(unlike arch/arm/include/asm/irq.h). Could that be the problem?


-boris

[toc] | [prev] | [next] | [standalone]

#1377067 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
> > Hi all,
> > 
> > Unfortunately this patch (now commit
> > 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
> > when running on top of QEMU: the number of PIT irqs get set to 0 by
> > probe_8259A but actually there are 16.
> > 
> > Any suggestions on how to fix this?
> > 
> > 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
> > 2) we could introduce an 'if (!xen_domain())' in probe_8259A
> > 3) suggestions welcome
> 
> Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
> 
> It was supposed to fix this problem for Xen. However, I just noticed that
> arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
> arch/arm/include/asm/irq.h). Could that be the problem?

I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
issue for me.

Is the idea of your patch that xen_allocate_irq_gsi will allocate the
descriptor dynamically instead?  If so, it doesn't work because it
doesn't get called for irq 14:

piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
-EVAIL

If you look at pci_xen_initial_domain, the loop:

for (irq = 0; irq < nr_legacy_irqs(); irq++) {

won't work anymore because by the time is called, nr_legacy_irqs()
already returns 0, because it has been changed by probe_8259A().

We also need the following patch:

---

xen/x86: actually allocate legacy interrupts on PV guests

b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
of legacy interrupts when actually nr_legacy_irqs() returns 0 after
probe_8259A(). Use NR_IRQS_LEGACY instead.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index beac4df..6db0060 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
 	__acpi_register_gsi = acpi_register_gsi_xen;
 	__acpi_unregister_gsi = NULL;
 	/* Pre-allocate legacy irqs */
-	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
+	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
 		int trigger, polarity;
 
 		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)

[toc] | [prev] | [next] | [standalone]

#1377123 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/12/2016 02:06 PM, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>> On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
>>> Hi all,
>>>
>>> Unfortunately this patch (now commit
>>> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
>>> when running on top of QEMU: the number of PIT irqs get set to 0 by
>>> probe_8259A but actually there are 16.
>>>
>>> Any suggestions on how to fix this?
>>>
>>> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
>>> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
>>> 3) suggestions welcome
>> Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
>>
>> It was supposed to fix this problem for Xen. However, I just noticed that
>> arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
>> arch/arm/include/asm/irq.h). Could that be the problem?
> I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
> issue for me.
>
> Is the idea of your patch that xen_allocate_irq_gsi will allocate the
> descriptor dynamically instead?

Right.

> If so, it doesn't work because it
> doesn't get called for irq 14:

So how has it worked until now then?

>
> piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
> devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
> -EVAIL
>
> If you look at pci_xen_initial_domain, the loop:
>
> for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>
> won't work anymore because by the time is called, nr_legacy_irqs()
> already returns 0, because it has been changed by probe_8259A().
>
> We also need the following patch:
>
> ---
>
> xen/x86: actually allocate legacy interrupts on PV guests
>
> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> probe_8259A(). Use NR_IRQS_LEGACY instead.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index beac4df..6db0060 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
>   	__acpi_register_gsi = acpi_register_gsi_xen;
>   	__acpi_unregister_gsi = NULL;
>   	/* Pre-allocate legacy irqs */
> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>   		int trigger, polarity;
>   
>   		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)


Won't we need the same change in the 'if (0 == nr_ioapics)' clause?

-boris

[toc] | [prev] | [next] | [standalone]

#1377269 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> On 04/12/2016 02:06 PM, Stefano Stabellini wrote:
> > On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> > > On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
> > > > Hi all,
> > > > 
> > > > Unfortunately this patch (now commit
> > > > 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
> > > > when running on top of QEMU: the number of PIT irqs get set to 0 by
> > > > probe_8259A but actually there are 16.
> > > > 
> > > > Any suggestions on how to fix this?
> > > > 
> > > > 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
> > > > 2) we could introduce an 'if (!xen_domain())' in probe_8259A
> > > > 3) suggestions welcome
> > > Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
> > > 
> > > It was supposed to fix this problem for Xen. However, I just noticed that
> > > arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
> > > arch/arm/include/asm/irq.h). Could that be the problem?
> > I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
> > issue for me.
> > 
> > Is the idea of your patch that xen_allocate_irq_gsi will allocate the
> > descriptor dynamically instead?
> 
> Right.
> 
> > If so, it doesn't work because it
> > doesn't get called for irq 14:
> 
> So how has it worked until now then?

It didn't for me :-)
Maybe it was working for DomUs, but not for Dom0?


> > piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
> > devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
> > -EVAIL
> > 
> > If you look at pci_xen_initial_domain, the loop:
> > 
> > for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> > 
> > won't work anymore because by the time is called, nr_legacy_irqs()
> > already returns 0, because it has been changed by probe_8259A().
> > 
> > We also need the following patch:
> > 
> > ---
> > 
> > xen/x86: actually allocate legacy interrupts on PV guests
> > 
> > b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> > of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> > probe_8259A(). Use NR_IRQS_LEGACY instead.
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index beac4df..6db0060 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
> >   	__acpi_register_gsi = acpi_register_gsi_xen;
> >   	__acpi_unregister_gsi = NULL;
> >   	/* Pre-allocate legacy irqs */
> > -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> > +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
> >   		int trigger, polarity;
> >     		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
> 
> 
> Won't we need the same change in the 'if (0 == nr_ioapics)' clause?

That's not a problem: there is 1 ioapic in my system and is detected
correctly.

[toc] | [prev] | [next] | [standalone]

#1377317 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/12/2016 05:14 PM, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>> On 04/12/2016 02:06 PM, Stefano Stabellini wrote:
>>> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>>>> On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
>>>>> Hi all,
>>>>>
>>>>> Unfortunately this patch (now commit
>>>>> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
>>>>> when running on top of QEMU: the number of PIT irqs get set to 0 by
>>>>> probe_8259A but actually there are 16.
>>>>>
>>>>> Any suggestions on how to fix this?
>>>>>
>>>>> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
>>>>> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
>>>>> 3) suggestions welcome
>>>> Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
>>>>
>>>> It was supposed to fix this problem for Xen. However, I just noticed that
>>>> arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
>>>> arch/arm/include/asm/irq.h). Could that be the problem?
>>> I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
>>> issue for me.
>>>
>>> Is the idea of your patch that xen_allocate_irq_gsi will allocate the
>>> descriptor dynamically instead?
>> Right.
>>
>>> If so, it doesn't work because it
>>> doesn't get called for irq 14:
>> So how has it worked until now then?
> It didn't for me :-)
> Maybe it was working for DomUs, but not for Dom0?

I most certainly run this as both dom0 and domU on variety of machines.

So perhaps there is some other problem?

>
>
>>> piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
>>> devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
>>> -EVAIL
>>>
>>> If you look at pci_xen_initial_domain, the loop:
>>>
>>> for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>>>
>>> won't work anymore because by the time is called, nr_legacy_irqs()
>>> already returns 0, because it has been changed by probe_8259A().
>>>
>>> We also need the following patch:
>>>
>>> ---
>>>
>>> xen/x86: actually allocate legacy interrupts on PV guests
>>>
>>> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
>>> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
>>> probe_8259A(). Use NR_IRQS_LEGACY instead.
>>>
>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>>> index beac4df..6db0060 100644
>>> --- a/arch/x86/pci/xen.c
>>> +++ b/arch/x86/pci/xen.c
>>> @@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
>>>    	__acpi_register_gsi = acpi_register_gsi_xen;
>>>    	__acpi_unregister_gsi = NULL;
>>>    	/* Pre-allocate legacy irqs */
>>> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>>> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>>>    		int trigger, polarity;
>>>      		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
>>
>> Won't we need the same change in the 'if (0 == nr_ioapics)' clause?
> That's not a problem: there is 1 ioapic in my system and is detected
> correctly.

True, but if a system has no ioapics then we will again fail to manage 
the pirq, no?

-boris

[toc] | [prev] | [next] | [standalone]

#1377367 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
>
>   
> I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
> But I am certain that 4.6-rc2, with the attached config, fails as Dom0
> on QEMU with the following sequence of calls:

I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config 
(4.6-rc3 doesn't build for me for some reason) and that booted dom0 as well.

BTW, what do you mean by "dom0 on QEMU"?

-boris

[toc] | [prev] | [next] | [standalone]

#1377404 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
> > 
> >   I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
> > But I am certain that 4.6-rc2, with the attached config, fails as Dom0
> > on QEMU with the following sequence of calls:
> 
> I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
> (4.6-rc3 doesn't build for me for some reason) and that booted dom0 as well.
> 
> BTW, what do you mean by "dom0 on QEMU"?
 
I am running Xen and Linux inside a QEMU x86_64 emulated machine (nested
virt).

[toc] | [prev] | [next] | [standalone]

#1377459 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/12/2016 07:15 PM, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>> On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
>>>    I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
>>> But I am certain that 4.6-rc2, with the attached config, fails as Dom0
>>> on QEMU with the following sequence of calls:
>> I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
>> (4.6-rc3 doesn't build for me for some reason) and that booted dom0 as well.
>>
>> BTW, what do you mean by "dom0 on QEMU"?
>   
> I am running Xen and Linux inside a QEMU x86_64 emulated machine (nested
> virt).

This I, of course, never tried.

But given that things work in a single-level virt, doesn't this imply 
that perhaps there is something in the emulation that's not quite right?

-boris

[toc] | [prev] | [next] | [standalone]

#1378132 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/12/2016 09:27 PM, Boris Ostrovsky wrote:
> On 04/12/2016 07:15 PM, Stefano Stabellini wrote:
>> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>>> On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
>>>>    I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
>>>> But I am certain that 4.6-rc2, with the attached config, fails as Dom0
>>>> on QEMU with the following sequence of calls:
>>> I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
>>> (4.6-rc3 doesn't build for me for some reason) and that booted dom0 
>>> as well.
>>>
>>> BTW, what do you mean by "dom0 on QEMU"?
>>   I am running Xen and Linux inside a QEMU x86_64 emulated machine 
>> (nested
>> virt).
>
> This I, of course, never tried.
>
> But given that things work in a single-level virt, doesn't this imply 
> that perhaps there is something in the emulation that's not quite right?

OK, so this *is* broken on single level virt as well. It's just that we 
always end up using AHCI so lack of irq 14 (and 15) does not affect the 
system. And I guess in QEMU case it's IDE only, right?

You patch does fix this but I wonder if we could change something in 
probe_8259A() so that we can continue using nr_legacy_irqs(). Using 
nr_legacy_irqs() and NR_IRQS_LEGACY at the same time is inconsistent and 
may cause us headaches in the future.


-boris

[toc] | [prev] | [next] | [standalone]

#1378189 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/13/2016 01:36 PM, Boris Ostrovsky wrote:
> On 04/12/2016 09:27 PM, Boris Ostrovsky wrote:
>> On 04/12/2016 07:15 PM, Stefano Stabellini wrote:
>>> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>>>> On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
>>>>>    I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
>>>>> But I am certain that 4.6-rc2, with the attached config, fails as 
>>>>> Dom0
>>>>> on QEMU with the following sequence of calls:
>>>> I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
>>>> (4.6-rc3 doesn't build for me for some reason) and that booted dom0 
>>>> as well.
>>>>
>>>> BTW, what do you mean by "dom0 on QEMU"?
>>>   I am running Xen and Linux inside a QEMU x86_64 emulated machine 
>>> (nested
>>> virt).
>>
>> This I, of course, never tried.
>>
>> But given that things work in a single-level virt, doesn't this imply 
>> that perhaps there is something in the emulation that's not quite right?
>
> OK, so this *is* broken on single level virt as well. It's just that 
> we always end up using AHCI so lack of irq 14 (and 15) does not affect 
> the system. And I guess in QEMU case it's IDE only, right?
>
> You patch does fix this but I wonder if we could change something in 
> probe_8259A() so that we can continue using nr_legacy_irqs(). Using 
> nr_legacy_irqs() and NR_IRQS_LEGACY at the same time is inconsistent 
> and may cause us headaches in the future.
>

I think we could use paravirt_has() feature that was added for similar 
reason when we had a problem with RTC (commit 
d8c98a1d1488747625ad6044d423406e17e99b7a). So we add paravirt_has(PIC) 
which will only be set by dom0 and then probe_8259A() will not set 
legacy_pic to null_legacy_pic when this flag is set.

Note that paravirt_has() is being removed by 
http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01415.html so 
presumably we'd use new struct x86_legacy_features instead (copying Luis 
so that if this is acceptable he could add it to his next spin).

-boris

||

[toc] | [prev] | [next] | [standalone]

#1378321 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On Wed, Apr 13, 2016 at 12:10 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> Note that paravirt_has() is being removed by
> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01415.html so
> presumably we'd use new struct x86_legacy_features instead (copying Luis so
> that if this is acceptable he could add it to his next spin).

That should be added as a separate patch then as its new feature not
replacing existing code.

 Luis

[toc] | [prev] | [next] | [standalone]

#1378326 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On Wed, 13 Apr 2016, Boris Ostrovsky wrote:

> On 04/13/2016 01:36 PM, Boris Ostrovsky wrote:
> > On 04/12/2016 09:27 PM, Boris Ostrovsky wrote:
> > > On 04/12/2016 07:15 PM, Stefano Stabellini wrote:
> > > > On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> > > > > On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
> > > > > >    I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
> > > > > > But I am certain that 4.6-rc2, with the attached config, fails as
> > > > > > Dom0
> > > > > > on QEMU with the following sequence of calls:
> > > > > I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
> > > > > (4.6-rc3 doesn't build for me for some reason) and that booted dom0 as
> > > > > well.
> > > > > 
> > > > > BTW, what do you mean by "dom0 on QEMU"?
> > > >   I am running Xen and Linux inside a QEMU x86_64 emulated machine
> > > > (nested
> > > > virt).
> > > 
> > > This I, of course, never tried.
> > > 
> > > But given that things work in a single-level virt, doesn't this imply that
> > > perhaps there is something in the emulation that's not quite right?
> > 
> > OK, so this *is* broken on single level virt as well. It's just that we
> > always end up using AHCI so lack of irq 14 (and 15) does not affect the
> > system. And I guess in QEMU case it's IDE only, right?
> > 
> > You patch does fix this but I wonder if we could change something in
> > probe_8259A() so that we can continue using nr_legacy_irqs(). Using
> > nr_legacy_irqs() and NR_IRQS_LEGACY at the same time is inconsistent and may
> > cause us headaches in the future.
> > 
> 
> I think we could use paravirt_has() feature that was added for similar reason
> when we had a problem with RTC (commit
> d8c98a1d1488747625ad6044d423406e17e99b7a). So we add paravirt_has(PIC) which
> will only be set by dom0 and then probe_8259A() will not set legacy_pic to
> null_legacy_pic when this flag is set.

Maybe we could introduce a legacy_pic_xen in arch/x86/kernel/i8259.c or
arch/x86/xen/enlighten.c? We could set legacy_pic = legacy_pic_xen from
start_kernel, so that we can skip probe_8259A completely.

 
> Note that paravirt_has() is being removed by
> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01415.html so
> presumably we'd use new struct x86_legacy_features instead (copying Luis so
> that if this is acceptable he could add it to his next spin).

I would prefer to come up with a fix that is backportable to 4.3, 4.4
and 4.5.

[toc] | [prev] | [next] | [standalone]

#1378803 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/13/2016 06:38 PM, Stefano Stabellini wrote:
>
> On Wed, 13 Apr 2016, Boris Ostrovsky wrote:
>
>> On 04/13/2016 01:36 PM, Boris Ostrovsky wrote:
>>> On 04/12/2016 09:27 PM, Boris Ostrovsky wrote:
>>>> On 04/12/2016 07:15 PM, Stefano Stabellini wrote:
>>>>> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>>>>>> On 04/12/2016 05:56 PM, Stefano Stabellini wrote:
>>>>>>>     I am not sure, maybe you didn't have CONFIG_SPARSE_IRQ?
>>>>>>> But I am certain that 4.6-rc2, with the attached config, fails as
>>>>>>> Dom0
>>>>>>> on QEMU with the following sequence of calls:
>>>>>> I did have CONFIG_SPARSE_IRQ and I just rebuilt 4.5.0 with your config
>>>>>> (4.6-rc3 doesn't build for me for some reason) and that booted dom0 as
>>>>>> well.
>>>>>>
>>>>>> BTW, what do you mean by "dom0 on QEMU"?
>>>>>    I am running Xen and Linux inside a QEMU x86_64 emulated machine
>>>>> (nested
>>>>> virt).
>>>> This I, of course, never tried.
>>>>
>>>> But given that things work in a single-level virt, doesn't this imply that
>>>> perhaps there is something in the emulation that's not quite right?
>>> OK, so this *is* broken on single level virt as well. It's just that we
>>> always end up using AHCI so lack of irq 14 (and 15) does not affect the
>>> system. And I guess in QEMU case it's IDE only, right?
>>>
>>> You patch does fix this but I wonder if we could change something in
>>> probe_8259A() so that we can continue using nr_legacy_irqs(). Using
>>> nr_legacy_irqs() and NR_IRQS_LEGACY at the same time is inconsistent and may
>>> cause us headaches in the future.
>>>
>> I think we could use paravirt_has() feature that was added for similar reason
>> when we had a problem with RTC (commit
>> d8c98a1d1488747625ad6044d423406e17e99b7a). So we add paravirt_has(PIC) which
>> will only be set by dom0 and then probe_8259A() will not set legacy_pic to
>> null_legacy_pic when this flag is set.
> Maybe we could introduce a legacy_pic_xen in arch/x86/kernel/i8259.c or
> arch/x86/xen/enlighten.c? We could set legacy_pic = legacy_pic_xen from
> start_kernel, so that we can skip probe_8259A completely.

Something like

legacy_pic_xen = null_legacy_pic;
legacy_pic_xen.nr_legacy_irqs = NR_IRQS_LEGACY;
legacy_pic = legacy_pic_xen;

?

>
>   
>> Note that paravirt_has() is being removed by
>> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01415.html so
>> presumably we'd use new struct x86_legacy_features instead (copying Luis so
>> that if this is acceptable he could add it to his next spin).
> I would prefer to come up with a fix that is backportable to 4.3, 4.4
> and 4.5.

I think once the series above makes it to mainline x86_legacy_features 
would be the way to go --- that's what it is created for. Given that we 
are almost at rc4 I assume that would be 4.7.

Using paravirt_has() now will create merge headaches so it's probably 
not a good idea. We can either use your original patch or do what you 
suggested with legacy_pic_xen (both of which are backportable) and then 
switch to x86_legacy_features once it shows up in the tree.

-boris

[toc] | [prev] | [next] | [standalone]

#1378868 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 12/04/16 19:06, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>> On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
>>> Hi all,
>>>
>>> Unfortunately this patch (now commit
>>> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
>>> when running on top of QEMU: the number of PIT irqs get set to 0 by
>>> probe_8259A but actually there are 16.
>>>
>>> Any suggestions on how to fix this?
>>>
>>> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
>>> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
>>> 3) suggestions welcome
>>
>> Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
>>
>> It was supposed to fix this problem for Xen. However, I just noticed that
>> arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
>> arch/arm/include/asm/irq.h). Could that be the problem?
> 
> I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
> issue for me.
> 
> Is the idea of your patch that xen_allocate_irq_gsi will allocate the
> descriptor dynamically instead?  If so, it doesn't work because it
> doesn't get called for irq 14:
> 
> piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
> devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
> -EVAIL
> 
> If you look at pci_xen_initial_domain, the loop:
> 
> for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> 
> won't work anymore because by the time is called, nr_legacy_irqs()
> already returns 0, because it has been changed by probe_8259A().
> 
> We also need the following patch:
> 
> ---
> 
> xen/x86: actually allocate legacy interrupts on PV guests
> 
> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
> probe_8259A(). Use NR_IRQS_LEGACY instead.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index beac4df..6db0060 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
>  	__acpi_register_gsi = acpi_register_gsi_xen;
>  	__acpi_unregister_gsi = NULL;
>  	/* Pre-allocate legacy irqs */
> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>  		int trigger, polarity;
>  
>  		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)

I think I prefer this fix because PV guests don't have a legacy PIC and
don't need any legacy irqs allocated so fiddling with nr_legacy_irqs()
seems wrong.

But we do need a comment here saying something like:

/*
 * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
 * because we don't have a PCI and thus nr_legacy_irqs() is zero.
 */

Does this make sense?

David

[toc] | [prev] | [next] | [standalone]

#1378971 — Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs

On 04/14/2016 09:45 AM, David Vrabel wrote:
> On 12/04/16 19:06, Stefano Stabellini wrote:
>> On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
>>> On 04/11/2016 10:08 PM, Stefano Stabellini wrote:
>>>> Hi all,
>>>>
>>>> Unfortunately this patch (now commit
>>>> 8c058b0b9c34d8c8d7912880956543769323e2d8) causes a regression on Xen
>>>> when running on top of QEMU: the number of PIT irqs get set to 0 by
>>>> probe_8259A but actually there are 16.
>>>>
>>>> Any suggestions on how to fix this?
>>>>
>>>> 1) we could revert 8c058b0b9c34d8c8d7912880956543769323e2d8
>>>> 2) we could introduce an 'if (!xen_domain())' in probe_8259A
>>>> 3) suggestions welcome
>>> Stefano, do you have b4ff8389ed14b849354b59ce9b360bdefcdbf99c ?
>>>
>>> It was supposed to fix this problem for Xen. However, I just noticed that
>>> arch/arm64/include/asm/irq.h makes nr_legacy_irqs() return 0 (unlike
>>> arch/arm/include/asm/irq.h). Could that be the problem?
>> I have b4ff8389ed14b849354b59ce9b360bdefcdbf99c but it doesn't fix the
>> issue for me.
>>
>> Is the idea of your patch that xen_allocate_irq_gsi will allocate the
>> descriptor dynamically instead?  If so, it doesn't work because it
>> doesn't get called for irq 14:
>>
>> piix_init_one -> ata_pci_sff_activate_host -> devm_request_irq ->
>> devm_request_threaded_irq-> request_threaded_irq -> irq_to_desc(14) ->
>> -EVAIL
>>
>> If you look at pci_xen_initial_domain, the loop:
>>
>> for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>>
>> won't work anymore because by the time is called, nr_legacy_irqs()
>> already returns 0, because it has been changed by probe_8259A().
>>
>> We also need the following patch:
>>
>> ---
>>
>> xen/x86: actually allocate legacy interrupts on PV guests
>>
>> b4ff8389ed14 is incomplete: relies on nr_legacy_irqs() to get the number
>> of legacy interrupts when actually nr_legacy_irqs() returns 0 after
>> probe_8259A(). Use NR_IRQS_LEGACY instead.
>>
>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>> index beac4df..6db0060 100644
>> --- a/arch/x86/pci/xen.c
>> +++ b/arch/x86/pci/xen.c
>> @@ -492,7 +492,7 @@ int __init pci_xen_initial_domain(void)
>>   	__acpi_register_gsi = acpi_register_gsi_xen;
>>   	__acpi_unregister_gsi = NULL;
>>   	/* Pre-allocate legacy irqs */
>> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>>   		int trigger, polarity;
>>   
>>   		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
> I think I prefer this fix because PV guests don't have a legacy PIC and
> don't need any legacy irqs allocated so fiddling with nr_legacy_irqs()
> seems wrong.
>
> But we do need a comment here saying something like:
>
> /*
>   * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
>   * because we don't have a PCI and thus nr_legacy_irqs() is zero.

s/PCI/PIC


>   */
>
> Does this make sense?
>
> David

OK. (I first thought that we also don't have RTC and yet we claim that 
dom0 has it. But then I realized that we do emulate it)

-boris

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web