diff mbox

xen/x86: actually allocate legacy interrupts on PV guests

Message ID alpine.DEB.2.10.1604201413070.23110@sstabellini-ThinkPad-X260 (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini April 20, 2016, 1:15 p.m. UTC 
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>

Comments

Juergen Gross April 21, 2016, 9:08 a.m. UTC | #1 
On 20/04/16 15:15, Stefano Stabellini wrote:
> 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.

Would you mind describing the resulting problem? With this commit
message I'm absolutely not capable to decide whether e.g. the other
use of nr_legacy_irqs() in pci_xen_initial_domain() is correct or
not.


Juergen

> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index beac4df..349b8ce 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
>  #endif
>  	__acpi_register_gsi = acpi_register_gsi_xen;
>  	__acpi_unregister_gsi = NULL;
> -	/* Pre-allocate legacy irqs */
> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> +	/*
> +	 * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
> +	 * because we don't have a PIC and thus nr_legacy_irqs() is zero.
> +	 */
> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>  		int trigger, polarity;
>  
>  		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
>
Stefano Stabellini April 21, 2016, 9:30 a.m. UTC | #2 
On Thu, 21 Apr 2016, Juergen Gross wrote:
> On 20/04/16 15:15, Stefano Stabellini wrote:
> > 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.
> 
> Would you mind describing the resulting problem?

This is a good question. The symptom is:

ata_piix: probe of 0000:00:01.1 failed with error -22


> With this commit message I'm absolutely not capable to decide whether
> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
> correct or not.

I looked at it but I couldn't really test that code because if I try to
change the number of ioapics in the system using the "noapic" command
line option (which actually changes the number if ioapics, not lapics),
I get an error from Linux saying that noapic is not supported when
running on Xen.

In my opinion having nr_legacy_irqs() calls in Xen code, which returns
0, is like playing with fire. I think it would be safer/saner to replace
them all with NR_IRQS_LEGACY, simply because reading the code one would
not expect that all those loops don't actually have any iterations.

However I didn't make the change because I couldn't test it properly.


> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index beac4df..349b8ce 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
> >  #endif
> >  	__acpi_register_gsi = acpi_register_gsi_xen;
> >  	__acpi_unregister_gsi = NULL;
> > -	/* Pre-allocate legacy irqs */
> > -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
> > +	/*
> > +	 * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
> > +	 * because we don't have a PIC and thus nr_legacy_irqs() is zero.
> > +	 */
> > +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
> >  		int trigger, polarity;
> >  
> >  		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
> > 
>
Olaf Hering April 21, 2016, 11:02 a.m. UTC | #3 
On Wed, Apr 20, Stefano Stabellini wrote:

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

Tested-by: Olaf Hering <olaf@aepfle.de>

Fixes my M9400 laptop (and should go to stable as well):
http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg02624.html

Olaf
Juergen Gross April 27, 2016, 5:02 a.m. UTC | #4 
On 21/04/16 11:30, Stefano Stabellini wrote:
> On Thu, 21 Apr 2016, Juergen Gross wrote:
>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>> 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.
>>
>> Would you mind describing the resulting problem?
> 
> This is a good question. The symptom is:
> 
> ata_piix: probe of 0000:00:01.1 failed with error -22
> 
> 
>> With this commit message I'm absolutely not capable to decide whether
>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>> correct or not.
> 
> I looked at it but I couldn't really test that code because if I try to
> change the number of ioapics in the system using the "noapic" command
> line option (which actually changes the number if ioapics, not lapics),
> I get an error from Linux saying that noapic is not supported when
> running on Xen.
> 
> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
> 0, is like playing with fire. I think it would be safer/saner to replace
> them all with NR_IRQS_LEGACY, simply because reading the code one would
> not expect that all those loops don't actually have any iterations.

I'm quite sure you should change both uses of nr_legacy_irqs() in
pci_xen_initial_domain().

Looking at xen_pcifront_enable_irq() I'm not really sure what is the
correct thing to do.

Adding Konrad as he might have a better insight.


Juergen

> 
> However I didn't make the change because I couldn't test it properly.
> 
> 
>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>>> index beac4df..349b8ce 100644
>>> --- a/arch/x86/pci/xen.c
>>> +++ b/arch/x86/pci/xen.c
>>> @@ -491,8 +491,11 @@ int __init pci_xen_initial_domain(void)
>>>  #endif
>>>  	__acpi_register_gsi = acpi_register_gsi_xen;
>>>  	__acpi_unregister_gsi = NULL;
>>> -	/* Pre-allocate legacy irqs */
>>> -	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
>>> +	/*
>>> +	 * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
>>> +	 * because we don't have a PIC and thus nr_legacy_irqs() is zero.
>>> +	 */
>>> +	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
>>>  		int trigger, polarity;
>>>  
>>>  		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)
>>>
>>
> 
>
David Vrabel April 27, 2016, 9:35 a.m. UTC | #5 
On 27/04/16 06:02, Juergen Gross wrote:
> On 21/04/16 11:30, Stefano Stabellini wrote:
>> On Thu, 21 Apr 2016, Juergen Gross wrote:
>>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>>> 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.
>>>
>>> Would you mind describing the resulting problem?
>>
>> This is a good question. The symptom is:
>>
>> ata_piix: probe of 0000:00:01.1 failed with error -22
>>
>>
>>> With this commit message I'm absolutely not capable to decide whether
>>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>>> correct or not.
>>
>> I looked at it but I couldn't really test that code because if I try to
>> change the number of ioapics in the system using the "noapic" command
>> line option (which actually changes the number if ioapics, not lapics),
>> I get an error from Linux saying that noapic is not supported when
>> running on Xen.
>>
>> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
>> 0, is like playing with fire. I think it would be safer/saner to replace
>> them all with NR_IRQS_LEGACY, simply because reading the code one would
>> not expect that all those loops don't actually have any iterations.
> 
> I'm quite sure you should change both uses of nr_legacy_irqs() in
> pci_xen_initial_domain().
> 
> Looking at xen_pcifront_enable_irq() I'm not really sure what is the
> correct thing to do.
> 
> Adding Konrad as he might have a better insight.

I wonder if it would be helpful to have a xen-specific #define like
XEN_NR_LEGACY_PIRQS or something, and document carefully what this means
and why it is != nr_legacy_irqs().

David
Boris Ostrovsky April 27, 2016, 1:38 p.m. UTC | #6 
On 04/27/2016 05:35 AM, David Vrabel wrote:
> On 27/04/16 06:02, Juergen Gross wrote:
>> On 21/04/16 11:30, Stefano Stabellini wrote:
>>> On Thu, 21 Apr 2016, Juergen Gross wrote:
>>>> On 20/04/16 15:15, Stefano Stabellini wrote:
>>>>> 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.
>>>> Would you mind describing the resulting problem?
>>> This is a good question. The symptom is:
>>>
>>> ata_piix: probe of 0000:00:01.1 failed with error -22
>>>
>>>
>>>> With this commit message I'm absolutely not capable to decide whether
>>>> e.g. the other use of nr_legacy_irqs() in pci_xen_initial_domain() is
>>>> correct or not.
>>> I looked at it but I couldn't really test that code because if I try to
>>> change the number of ioapics in the system using the "noapic" command
>>> line option (which actually changes the number if ioapics, not lapics),
>>> I get an error from Linux saying that noapic is not supported when
>>> running on Xen.
>>>
>>> In my opinion having nr_legacy_irqs() calls in Xen code, which returns
>>> 0, is like playing with fire. I think it would be safer/saner to replace
>>> them all with NR_IRQS_LEGACY, simply because reading the code one would
>>> not expect that all those loops don't actually have any iterations.
>> I'm quite sure you should change both uses of nr_legacy_irqs() in
>> pci_xen_initial_domain().
>>
>> Looking at xen_pcifront_enable_irq() I'm not really sure what is the
>> correct thing to do.
>>
>> Adding Konrad as he might have a better insight.
> I wonder if it would be helpful to have a xen-specific #define like
> XEN_NR_LEGACY_PIRQS or something, and document carefully what this means
> and why it is != nr_legacy_irqs().


int xen_nr_legacy_irqs()
{
     if (xen_hvm_domain())
         return nr_legacy_irqs();
     if (xen_initial_domain())
         return NR_IRQS_LEGACY;
     return 0;
}

?

-boris
David Vrabel April 27, 2016, 1:40 p.m. UTC | #7 
On 27/04/16 14:38, Boris Ostrovsky wrote:
> 
> int xen_nr_legacy_irqs()
> {
>     if (xen_hvm_domain())
>         return nr_legacy_irqs();
>     if (xen_initial_domain())
>         return NR_IRQS_LEGACY;
>     return 0;
> }

Yeah, if that does the right thing...

David
Boris Ostrovsky April 27, 2016, 2:03 p.m. UTC | #8 
On 04/27/2016 09:40 AM, David Vrabel wrote:
> On 27/04/16 14:38, Boris Ostrovsky wrote:
>> int xen_nr_legacy_irqs()
>> {
>>      if (xen_hvm_domain())
>>          return nr_legacy_irqs();
>>      if (xen_initial_domain())
>>          return NR_IRQS_LEGACY;
>>      return 0;
>> }
> Yeah, if that does the right thing...

I think it will break xen_allocate_irq_gsi() again, unless we check for 
HVM domain explicitly. Which would be ugly.

-boris
Stefano Stabellini May 16, 2016, 11:23 a.m. UTC | #9 
On Wed, 27 Apr 2016, Boris Ostrovsky wrote:
> On 04/27/2016 09:40 AM, David Vrabel wrote:
> > On 27/04/16 14:38, Boris Ostrovsky wrote:
> > > int xen_nr_legacy_irqs()
> > > {
> > >      if (xen_hvm_domain())
> > >          return nr_legacy_irqs();
> > >      if (xen_initial_domain())
> > >          return NR_IRQS_LEGACY;
> > >      return 0;
> > > }
> > Yeah, if that does the right thing...
> 
> I think it will break xen_allocate_irq_gsi() again, unless we check for HVM
> domain explicitly. Which would be ugly.

I guess we all forgot about this patch, in the meantime the merge window
has opened.

Should we go ahead with:

http://marc.info/?l=linux-kernel&m=146115812124261&w=2

?
It might not be complete, but it is certainly an improvement.

Otherwise, please submit proper patches ASAP. I don't think we want to
delay this fix until 4.8.
Boris Ostrovsky May 16, 2016, 1:57 p.m. UTC | #10 
On 05/16/2016 07:23 AM, Stefano Stabellini wrote:
> On Wed, 27 Apr 2016, Boris Ostrovsky wrote:
>> On 04/27/2016 09:40 AM, David Vrabel wrote:
>>> On 27/04/16 14:38, Boris Ostrovsky wrote:
>>>> int xen_nr_legacy_irqs()
>>>> {
>>>>      if (xen_hvm_domain())
>>>>          return nr_legacy_irqs();
>>>>      if (xen_initial_domain())
>>>>          return NR_IRQS_LEGACY;
>>>>      return 0;
>>>> }
>>> Yeah, if that does the right thing...
>> I think it will break xen_allocate_irq_gsi() again, unless we check for HVM
>> domain explicitly. Which would be ugly.
> I guess we all forgot about this patch, in the meantime the merge window
> has opened.
>
> Should we go ahead with:
>
> http://marc.info/?l=linux-kernel&m=146115812124261&w=2
>
> ?
> It might not be complete, but it is certainly an improvement.

Yes, I think what you have there is the best option. What I suggested
above won't work and adding another check for HVM domain in 
xen_allocate_irq_gsi() won't look good (although it does work).

It will need to go to stable as well (4.4+ ?)

-boris

>
> Otherwise, please submit proper patches ASAP. I don't think we want to
> delay this fix until 4.8.
Stefano Stabellini May 16, 2016, 3:21 p.m. UTC | #11 
On Mon, 16 May 2016, Boris Ostrovsky wrote:
> On 05/16/2016 07:23 AM, Stefano Stabellini wrote:
> > On Wed, 27 Apr 2016, Boris Ostrovsky wrote:
> >> On 04/27/2016 09:40 AM, David Vrabel wrote:
> >>> On 27/04/16 14:38, Boris Ostrovsky wrote:
> >>>> int xen_nr_legacy_irqs()
> >>>> {
> >>>>      if (xen_hvm_domain())
> >>>>          return nr_legacy_irqs();
> >>>>      if (xen_initial_domain())
> >>>>          return NR_IRQS_LEGACY;
> >>>>      return 0;
> >>>> }
> >>> Yeah, if that does the right thing...
> >> I think it will break xen_allocate_irq_gsi() again, unless we check for HVM
> >> domain explicitly. Which would be ugly.
> > I guess we all forgot about this patch, in the meantime the merge window
> > has opened.
> >
> > Should we go ahead with:
> >
> > http://marc.info/?l=linux-kernel&m=146115812124261&w=2
> >
> > ?
> > It might not be complete, but it is certainly an improvement.
> 
> Yes, I think what you have there is the best option. What I suggested
> above won't work and adding another check for HVM domain in 
> xen_allocate_irq_gsi() won't look good (although it does work).
> 
> It will need to go to stable as well (4.4+ ?)

All right, I'll commit it to for-linus-4.7 with
 Cc: stable@vger.kernel.org
diff mbox

Patch

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index beac4df..349b8ce 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -491,8 +491,11 @@  int __init pci_xen_initial_domain(void)
 #endif
 	__acpi_register_gsi = acpi_register_gsi_xen;
 	__acpi_unregister_gsi = NULL;
-	/* Pre-allocate legacy irqs */
-	for (irq = 0; irq < nr_legacy_irqs(); irq++) {
+	/*
+	 * Pre-allocate the legacy IRQs.  Use NR_LEGACY_IRQS here
+	 * because we don't have a PIC and thus nr_legacy_irqs() is zero.
+	 */
+	for (irq = 0; irq < NR_IRQS_LEGACY; irq++) {
 		int trigger, polarity;
 
 		if (acpi_get_override_irq(irq, &trigger, &polarity) == -1)