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 <[email protected]>
---
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
Vitaly!
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.
Nice catch!
> Fixes: d32932d02e18 ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> ---
> 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();
We can simplify this by letting probe() return the number of legacy
interrupts, hmm?
Thanks,
tglx
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Stefano Stabellini <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
--
Vitaly
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
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 <[email protected]>
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)
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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.
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 <[email protected]>
>>>
>>> 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
On Tue, 12 Apr 2016, Boris Ostrovsky wrote:
> 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?
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:
> > > > 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 <[email protected]>
> > > >
> > > > 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?
Right, possibly. I guess there is no harm in doing that, given that if
xen_bind_pirq_gsi_to_irq has already been called for the irq, then it
simply returns.
---
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 <[email protected]>
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index beac4df..5668aca 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)
@@ -503,7 +503,7 @@ int __init pci_xen_initial_domain(void)
true /* Map GSI to PIRQ */);
}
if (0 == nr_ioapics) {
- for (irq = 0; irq < nr_legacy_irqs(); irq++)
+ for (irq = 0; irq < NR_IRQS_LEGACY; irq++)
xen_bind_pirq_gsi_to_irq(irq, irq, 0, "xt-pic");
}
return 0;
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
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).
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
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
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
||
On Wed, Apr 13, 2016 at 12:10 PM, Boris Ostrovsky
<[email protected]> 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
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.
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
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 <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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