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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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