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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Apr 2016 17:34:54 -0400
From:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:	Stefano Stabellini <sstabellini@...nel.org>
Cc:	Vitaly Kuznetsov <vkuznets@...hat.com>, x86@...nel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	linux-kernel@...r.kernel.org, konrad.wilk@...cle.com,
	david.vrabel@...rix.com, jgross@...e.com
Subject: 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@...nel.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