Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1383369 > unrolled thread
| Started by | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| First post | 2016-04-20 15:20 +0200 |
| Last post | 2016-04-21 13:10 +0200 |
| Articles | 9 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] xen/x86: actually allocate legacy interrupts on PV guests Stefano Stabellini <sstabellini@kernel.org> - 2016-04-20 15:20 +0200
Re: [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Juergen Gross <jgross@suse.com> - 2016-04-21 11:10 +0200
Re: [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Stefano Stabellini <sstabellini@kernel.org> - 2016-04-21 11:40 +0200
Re: [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Juergen Gross <jgross@suse.com> - 2016-04-27 07:10 +0200
Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests David Vrabel <david.vrabel@citrix.com> - 2016-04-27 11:40 +0200
Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-27 15:40 +0200
Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests David Vrabel <david.vrabel@citrix.com> - 2016-04-27 15:50 +0200
Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-04-27 16:10 +0200
Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests Olaf Hering <olaf@aepfle.de> - 2016-04-21 13:10 +0200
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2016-04-20 15:20 +0200 |
| Subject | [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rq0cV-8rY-15@gated-at.bofh.it> |
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..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)
[toc] | [next] | [standalone]
| From | Juergen Gross <jgross@suse.com> |
|---|---|
| Date | 2016-04-21 11:10 +0200 |
| Message-ID | <rqiMy-6KC-1@gated-at.bofh.it> |
| In reply to | #1383369 |
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)
>
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2016-04-21 11:40 +0200 |
| Subject | Re: [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rqjfA-70c-29@gated-at.bofh.it> |
| In reply to | #1383964 |
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)
> >
>
[toc] | [prev] | [next] | [standalone]
| From | Juergen Gross <jgross@suse.com> |
|---|---|
| Date | 2016-04-27 07:10 +0200 |
| Message-ID | <rspTz-3Q0-5@gated-at.bofh.it> |
| In reply to | #1384014 |
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)
>>>
>>
>
>
[toc] | [prev] | [next] | [standalone]
| From | David Vrabel <david.vrabel@citrix.com> |
|---|---|
| Date | 2016-04-27 11:40 +0200 |
| Subject | Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rsu6S-7lj-19@gated-at.bofh.it> |
| In reply to | #1388666 |
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
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-04-27 15:40 +0200 |
| Subject | Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rsxR7-1XC-5@gated-at.bofh.it> |
| In reply to | #1388837 |
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
[toc] | [prev] | [next] | [standalone]
| From | David Vrabel <david.vrabel@citrix.com> |
|---|---|
| Date | 2016-04-27 15:50 +0200 |
| Subject | Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rsy0O-22y-9@gated-at.bofh.it> |
| In reply to | #1389048 |
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
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2016-04-27 16:10 +0200 |
| Subject | Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rsykb-2uQ-39@gated-at.bofh.it> |
| In reply to | #1389059 |
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
[toc] | [prev] | [next] | [standalone]
| From | Olaf Hering <olaf@aepfle.de> |
|---|---|
| Date | 2016-04-21 13:10 +0200 |
| Subject | Re: [Xen-devel] [PATCH] xen/x86: actually allocate legacy interrupts on PV guests |
| Message-ID | <rqkEG-8dU-19@gated-at.bofh.it> |
| In reply to | #1383369 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web