Xen devel - Jun 2009 - [PATCH] Fix legacy irq allocation issue

When try to assign vector for irq, we should not assign vector for legacy irq,
which has fixed mapped irq<->vector relationship.

Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>


diff -r 2acd065485e8 xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c    Sun Jun 14 23:12:24 2009 +0800
+++ b/xen/arch/x86/irq.c    Wed Jun 17 08:51:43 2009 +0800
@@ -83,6 +83,12 @@ int assign_irq_vector(int irq)
     BUG_ON(irq >= nr_irqs && irq != AUTO_ASSIGN_IRQ);

     spin_lock(&vector_lock);
+
+    if ((irq != AUTO_ASSIGN_IRQ) && !IO_APIC_IRQ(irq))
+    {
+        spin_unlock(&vector_lock);
+        return LEGACY_VECTOR(irq);
+    }

     if ((irq != AUTO_ASSIGN_IRQ) && (IO_APIC_VECTOR(irq) > 0)) {
         spin_unlock(&vector_lock);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 19.06.09
04:03 >>>
>When try to assign vector for irq, we should not assign vector for legacy
irq, which has fixed mapped irq<->vector relationship.
What code path does get us there?
>--- a/xen/arch/x86/irq.c    Sun Jun 14 23:12:24 2009 +0800
>+++ b/xen/arch/x86/irq.c    Wed Jun 17 08:51:43 2009 +0800
>@@ -83,6 +83,12 @@ int assign_irq_vector(int irq)
>     BUG_ON(irq >= nr_irqs && irq != AUTO_ASSIGN_IRQ);
>
>     spin_lock(&vector_lock);
>+
>+    if ((irq != AUTO_ASSIGN_IRQ) && !IO_APIC_IRQ(irq))
>+    {
>+        spin_unlock(&vector_lock);
>+        return LEGACY_VECTOR(irq);
>+    }
If this is really needed, then why not simply exchange the following uses
of IO_APIC_VECTOR() to irq_to_vector(), which already properly
considers legacy vectors?
>
>     if ((irq != AUTO_ASSIGN_IRQ) && (IO_APIC_VECTOR(irq) > 0)) {
>         spin_unlock(&vector_lock);
Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
>>>> "Jiang, Yunhong" <yunhong.jiang@intel.com>
19.06.09 04:03 >>>
>> When try to assign vector for irq, we should not assign
> vector for legacy irq, which has fixed mapped irq<->vector
> relationship. 
> 
> What code path does get us there?
> 
>> --- a/xen/arch/x86/irq.c    Sun Jun 14 23:12:24 2009 +0800
>> +++ b/xen/arch/x86/irq.c    Wed Jun 17 08:51:43 2009 +0800
>> @@ -83,6 +83,12 @@ int assign_irq_vector(int irq)
>>     BUG_ON(irq >= nr_irqs && irq != AUTO_ASSIGN_IRQ);
>> 
>>     spin_lock(&vector_lock);
>> +
>> +    if ((irq != AUTO_ASSIGN_IRQ) && !IO_APIC_IRQ(irq)) +    {
>> +        spin_unlock(&vector_lock);
>> +        return LEGACY_VECTOR(irq);
>> +    }
> 
> If this is really needed, then why not simply exchange the following
> uses of IO_APIC_VECTOR() to irq_to_vector(), which already properly
> considers legacy vectors? 
Aha, yes. I should use that.

This is needed if dom0 try to assign a vector to legacy irq. I find this when
help debugging one system. I don''t have the broken system, so I have no
idea why dom0 will try to assign vector to legacy irq. But from API point of
view, I think this check is needed.
 
--jyh
> 
>> 
>>     if ((irq != AUTO_ASSIGN_IRQ) && (IO_APIC_VECTOR(irq) >
0)) {
>>         spin_unlock(&vector_lock);
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19/06/2009 09:13, "Jiang, Yunhong" <yunhong.jiang@intel.com>
wrote:
>> If this is really needed, then why not simply exchange the following
>> uses of IO_APIC_VECTOR() to irq_to_vector(), which already properly
>> considers legacy vectors?
> 
> Aha, yes. I should use that.
A patch to apply on top of the old patch then, please.

 K.
> This is needed if dom0 try to assign a vector to legacy irq. I find this
when
> help debugging one system. I don''t have the broken system, so I
have no idea
> why dom0 will try to assign vector to legacy irq. But from API point of
view,
> I think this check is needed.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The followed is based on old patch. Jan, is this ok?
Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>

BTW, when I working on this, I''m abit confused of the irq. I''m
not sure if I can assume irq is mainly for IOAPIC/PIC (i.e. something like gsi
and is global), while pirq is just physical irq (i.e. including both gsi/MSI
irq)?

 If yes, what''s the irq in PHYSDEVOP_alloc_irq_vector()? It is in fact
dom0''s irq, however, in assign_irq_vector(), seems it is treated same
as Xen''s irq. I remember I understood that part when I begin working on
MSI, but seems I fogot the answer now :$

 (seems this is also discussed in mailint list when talking about pv_ops dom0).

Thanks
Yunhong Jiang


diff -r 880c27f9e2db xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c        Fri Jun 19 02:56:20 2009 +0800
+++ b/xen/arch/x86/irq.c        Fri Jun 19 02:56:38 2009 +0800
@@ -84,15 +84,9 @@ int assign_irq_vector(int irq)

     spin_lock(&vector_lock);

-    if ((irq != AUTO_ASSIGN_IRQ) && !IO_APIC_IRQ(irq))
-    {
+    if ((irq != AUTO_ASSIGN_IRQ) && (irq_to_vector(irq) > 0)) {
         spin_unlock(&vector_lock);
-        return LEGACY_VECTOR(irq);
-    }
-
-    if ((irq != AUTO_ASSIGN_IRQ) && (IO_APIC_VECTOR(irq) > 0)) {
-        spin_unlock(&vector_lock);
-        return IO_APIC_VECTOR(irq);
+        return irq_to_vector(irq);
     }

     vector = current_vector;



Keir Fraser wrote:
> On 19/06/2009 09:13, "Jiang, Yunhong"
<yunhong.jiang@intel.com> wrote:
> 
>>> If this is really needed, then why not simply exchange the
following
>>> uses of IO_APIC_VECTOR() to irq_to_vector(), which already properly
>>> considers legacy vectors?
>> 
>> Aha, yes. I should use that.
> 
> A patch to apply on top of the old patch then, please.
> 
> K.
> 
>> This is needed if dom0 try to assign a vector to legacy irq. I find
>> this when help debugging one system. I don''t have the broken
system,
>> so I have no idea why dom0 will try to assign vector to legacy irq.
>> But from API point of view, I think this check is needed. 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 19.06.09
11:03 >>>
>The followed is based on old patch. Jan, is this ok?
>Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
Yes, this is how I expected it to be.
>BTW, when I working on this, I''m abit confused of the irq.
I''m not sure if I can assume irq is mainly for IOAPIC/PIC (i.e.
something
>like gsi and is global), while pirq is just physical irq (i.e. including
both gsi/MSI irq)?
"irq" should no longer refer to anything MSI related (MSI just
requires a vector, but not an irq). "pirq" is generally meant to be
the guest representation (even for MSI, the guest needs a pirq assigned because
the event channel interface requires one to be passed in).
> If yes, what''s the irq in PHYSDEVOP_alloc_irq_vector()? It is in
fact dom0''s irq, however, in assign_irq_vector(), seems it is treated
>same as Xen''s irq. I remember I understood that part when I begin
working on MSI, but seems I fogot the answer now :$
Correct, because for IO-APIC irqs a 1:1 mapping is being assumed between (dom0)
pirq and (xen) irq. I think there''s currently no real reason to break
this assumption, even though it seems not fully correct (because not properly
abstracted).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
>>>> "Jiang, Yunhong" <yunhong.jiang@intel.com>
19.06.09 11:03 >>>
>> The followed is based on old patch. Jan, is this ok?
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
> 
> Yes, this is how I expected it to be.
> 
>> BTW, when I working on this, I''m abit confused of the irq.
> I''m not sure if I can assume irq is mainly for IOAPIC/PIC
> (i.e. something
>> like gsi and is global), while pirq is just physical irq
> (i.e. including both gsi/MSI irq)?
> 
> "irq" should no longer refer to anything MSI related (MSI just
> requires a vector, but not an irq). "pirq" is generally meant
> to be the guest representation (even for MSI, the guest needs
> a pirq assigned because the event channel interface requires one to
> be passed in). 
So in fact, we can explain pirq to be per-domain irq, instead of physical irq
anymore (I assume this is achieved mainly in 19650 changeset).

In Keir/Jeremy''s new design interface for PV_ops dom0, will
IOAPIC''s interface be pirq (i.e. perdomain irq) also?

Current arch/x86/irq.c is not so clear in using irq/pirq, like all parameter is
irq. (It is in fact my fault, I think :$ )
> 
>> If yes, what''s the irq in PHYSDEVOP_alloc_irq_vector()? It
> is in fact dom0''s irq, however, in assign_irq_vector(), seems
> it is treated
>> same as Xen''s irq. I remember I understood that part when I
> begin working on MSI, but seems I fogot the answer now :$
> 
> Correct, because for IO-APIC irqs a 1:1 mapping is being
> assumed between (dom0) pirq and (xen) irq. I think there''s
> currently no real reason to break this assumption, even though
> it seems not fully correct (because not properly abstracted).
Do you think Xen hypervisor itself need anything like "irq", if not
considering back-compatibility with old dom0? For xen, it only cares physical
IOAPIC (i.e. gsi) and vector. The maint task is to translate a vector to a pirq
and then inject it to corresponding guest. I think Xen itself is not using irq
anymore, like request_irq_vector()).
But not sure what will happen when we adding per-cpu vector, since at that time,
vector will not be a global namespace anymore, we may need a name for the
irq_desc[]. After all, usually irq can be explained as index for irq_desc[]
while it is not in Xen''s context.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel