Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758114AbcDLVgw (ORCPT ); Tue, 12 Apr 2016 17:36:52 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:40193 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758085AbcDLVgt (ORCPT ); Tue, 12 Apr 2016 17:36:49 -0400 Subject: Re: Xen regression, Was: [PATCH] x86/irq: Probe for PIC presence before allocating descs for legacy IRQs To: Stefano Stabellini References: <1446470676-1877-1-git-send-email-vkuznets@redhat.com> <570CF69A.1020701@oracle.com> <570D4645.20004@oracle.com> Cc: Vitaly Kuznetsov , x86@kernel.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Jiang Liu , "K. Y. Srinivasan" , linux-kernel@vger.kernel.org, konrad.wilk@oracle.com, david.vrabel@citrix.com, jgross@suse.com From: Boris Ostrovsky Message-ID: <570D69FE.6090200@oracle.com> Date: Tue, 12 Apr 2016 17:34:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3145 Lines: 85 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 >>> >>> 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