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

From: Boris Ostrovsky
Date: Tue Apr 12 2016 - 17:36:54 EST


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@xxxxxxxxxx>

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