* [PATCH] x86/IO-APIC: fix guest RTE write corner cases
@ 2013-05-08 12:51 Jan Beulich
2013-05-14 14:52 ` Ping: " Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2013-05-08 12:51 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, xiantao.zhang
[-- Attachment #1: Type: text/plain, Size: 3420 bytes --]
This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
For one, IRQs that had their vector set up by Xen internally without a
handler ever having got set (e.g. via "com<n>=..." without a matching
consumer option like "console=com<n>") would wrongly call
add_pin_to_irq() here, triggering the BUG_ON() in that function.
Second, when assign_irq_vector() fails this addition to irq_2_pin[]
needs to be undone.
In the context of this I'm also surprised that the irq_2_pin[]
manipulations here occur without any lock, i.e. rely on Dom0 to do
some sort of serialization.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
share_vector_maps(irq_2_pin[irq].apic, apic);
}
+static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
+{
+ struct irq_pin_list *entry, *prev;
+
+ for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
+ if ((entry->apic == apic) && (entry->pin == pin))
+ break;
+ BUG_ON(!entry->next);
+ }
+
+ entry->pin = entry->apic = -1;
+
+ if (entry != &irq_2_pin[irq]) {
+ /* Removed entry is not at head of list. */
+ prev = &irq_2_pin[irq];
+ while (&irq_2_pin[prev->next] != entry)
+ prev = &irq_2_pin[prev->next];
+ prev->next = entry->next;
+ } else if (entry->next) {
+ /* Removed entry is at head of multi-item list. */
+ prev = entry;
+ entry = &irq_2_pin[entry->next];
+ *prev = *entry;
+ entry->pin = entry->apic = -1;
+ } else
+ return;
+
+ entry->next = irq_2_pin_free_entry;
+ irq_2_pin_free_entry = entry - irq_2_pin;
+}
+
/*
* Reroute an IRQ to a different pin.
*/
@@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
{
- int apic, pin, irq, ret, vector, pirq;
+ int apic, pin, irq, ret, pirq;
struct IO_APIC_route_entry rte = { 0 };
unsigned long flags;
struct irq_desc *desc;
@@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
return 0;
}
- if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) {
- add_pin_to_irq(irq, apic, pin);
- vector = assign_irq_vector(irq, NULL);
- if ( vector < 0 )
- return vector;
+ if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
+ {
+ int vector = desc->arch.vector;
+
+ if ( vector < FIRST_HIPRIORITY_VECTOR )
+ add_pin_to_irq(irq, apic, pin);
+ else
+ desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
+ ret = assign_irq_vector(irq, NULL);
+ if ( ret < 0 )
+ {
+ if ( vector < FIRST_HIPRIORITY_VECTOR )
+ remove_pin_from_irq(irq, apic, pin);
+ else
+ desc->arch.vector = vector;
+ return ret;
+ }
- printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, irq);
+ printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
}
spin_lock(&dom0->event_lock);
ret = map_domain_pirq(dom0, pirq, irq,
[-- Attachment #2: x86-ioapic-unused-hiprio-irq.patch --]
[-- Type: text/plain, Size: 3463 bytes --]
x86/IO-APIC: fix guest RTE write corner cases
This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
For one, IRQs that had their vector set up by Xen internally without a
handler ever having got set (e.g. via "com<n>=..." without a matching
consumer option like "console=com<n>") would wrongly call
add_pin_to_irq() here, triggering the BUG_ON() in that function.
Second, when assign_irq_vector() fails this addition to irq_2_pin[]
needs to be undone.
In the context of this I'm also surprised that the irq_2_pin[]
manipulations here occur without any lock, i.e. rely on Dom0 to do
some sort of serialization.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
share_vector_maps(irq_2_pin[irq].apic, apic);
}
+static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
+{
+ struct irq_pin_list *entry, *prev;
+
+ for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
+ if ((entry->apic == apic) && (entry->pin == pin))
+ break;
+ BUG_ON(!entry->next);
+ }
+
+ entry->pin = entry->apic = -1;
+
+ if (entry != &irq_2_pin[irq]) {
+ /* Removed entry is not at head of list. */
+ prev = &irq_2_pin[irq];
+ while (&irq_2_pin[prev->next] != entry)
+ prev = &irq_2_pin[prev->next];
+ prev->next = entry->next;
+ } else if (entry->next) {
+ /* Removed entry is at head of multi-item list. */
+ prev = entry;
+ entry = &irq_2_pin[entry->next];
+ *prev = *entry;
+ entry->pin = entry->apic = -1;
+ } else
+ return;
+
+ entry->next = irq_2_pin_free_entry;
+ irq_2_pin_free_entry = entry - irq_2_pin;
+}
+
/*
* Reroute an IRQ to a different pin.
*/
@@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
{
- int apic, pin, irq, ret, vector, pirq;
+ int apic, pin, irq, ret, pirq;
struct IO_APIC_route_entry rte = { 0 };
unsigned long flags;
struct irq_desc *desc;
@@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
return 0;
}
- if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) {
- add_pin_to_irq(irq, apic, pin);
- vector = assign_irq_vector(irq, NULL);
- if ( vector < 0 )
- return vector;
+ if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
+ {
+ int vector = desc->arch.vector;
+
+ if ( vector < FIRST_HIPRIORITY_VECTOR )
+ add_pin_to_irq(irq, apic, pin);
+ else
+ desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
+ ret = assign_irq_vector(irq, NULL);
+ if ( ret < 0 )
+ {
+ if ( vector < FIRST_HIPRIORITY_VECTOR )
+ remove_pin_from_irq(irq, apic, pin);
+ else
+ desc->arch.vector = vector;
+ return ret;
+ }
- printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, irq);
+ printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
}
spin_lock(&dom0->event_lock);
ret = map_domain_pirq(dom0, pirq, irq,
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases
2013-05-08 12:51 [PATCH] x86/IO-APIC: fix guest RTE write corner cases Jan Beulich
@ 2013-05-14 14:52 ` Jan Beulich
2013-05-14 15:17 ` Keir Fraser
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Jan Beulich @ 2013-05-14 14:52 UTC (permalink / raw)
To: Andrew Cooper, xiantao.zhang, Keir Fraser; +Cc: xen-devel
>>> On 08.05.13 at 14:51, "Jan Beulich" <JBeulich@suse.com> wrote:
> This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
> hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
>
> For one, IRQs that had their vector set up by Xen internally without a
> handler ever having got set (e.g. via "com<n>=..." without a matching
> consumer option like "console=com<n>") would wrongly call
> add_pin_to_irq() here, triggering the BUG_ON() in that function.
>
> Second, when assign_irq_vector() fails this addition to irq_2_pin[]
> needs to be undone.
>
> In the context of this I'm also surprised that the irq_2_pin[]
> manipulations here occur without any lock, i.e. rely on Dom0 to do
> some sort of serialization.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
No-one having any opinion? I'm hesitant to commit changes like
this without anyone having said any word...
Jan
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
> share_vector_maps(irq_2_pin[irq].apic, apic);
> }
>
> +static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
> +{
> + struct irq_pin_list *entry, *prev;
> +
> + for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
> + if ((entry->apic == apic) && (entry->pin == pin))
> + break;
> + BUG_ON(!entry->next);
> + }
> +
> + entry->pin = entry->apic = -1;
> +
> + if (entry != &irq_2_pin[irq]) {
> + /* Removed entry is not at head of list. */
> + prev = &irq_2_pin[irq];
> + while (&irq_2_pin[prev->next] != entry)
> + prev = &irq_2_pin[prev->next];
> + prev->next = entry->next;
> + } else if (entry->next) {
> + /* Removed entry is at head of multi-item list. */
> + prev = entry;
> + entry = &irq_2_pin[entry->next];
> + *prev = *entry;
> + entry->pin = entry->apic = -1;
> + } else
> + return;
> +
> + entry->next = irq_2_pin_free_entry;
> + irq_2_pin_free_entry = entry - irq_2_pin;
> +}
> +
> /*
> * Reroute an IRQ to a different pin.
> */
> @@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
>
> int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
> {
> - int apic, pin, irq, ret, vector, pirq;
> + int apic, pin, irq, ret, pirq;
> struct IO_APIC_route_entry rte = { 0 };
> unsigned long flags;
> struct irq_desc *desc;
> @@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
> return 0;
> }
>
> - if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) {
> - add_pin_to_irq(irq, apic, pin);
> - vector = assign_irq_vector(irq, NULL);
> - if ( vector < 0 )
> - return vector;
> + if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
> + {
> + int vector = desc->arch.vector;
> +
> + if ( vector < FIRST_HIPRIORITY_VECTOR )
> + add_pin_to_irq(irq, apic, pin);
> + else
> + desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
> + ret = assign_irq_vector(irq, NULL);
> + if ( ret < 0 )
> + {
> + if ( vector < FIRST_HIPRIORITY_VECTOR )
> + remove_pin_from_irq(irq, apic, pin);
> + else
> + desc->arch.vector = vector;
> + return ret;
> + }
>
> - printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, irq);
> + printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
> }
> spin_lock(&dom0->event_lock);
> ret = map_domain_pirq(dom0, pirq, irq,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases
2013-05-14 14:52 ` Ping: " Jan Beulich
@ 2013-05-14 15:17 ` Keir Fraser
2013-05-15 10:57 ` Andrew Cooper
2013-05-16 12:47 ` Zhang, Xiantao
2 siblings, 0 replies; 5+ messages in thread
From: Keir Fraser @ 2013-05-14 15:17 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, xiantao.zhang; +Cc: xen-devel
On 14/05/2013 15:52, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 08.05.13 at 14:51, "Jan Beulich" <JBeulich@suse.com> wrote:
>> This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
>> hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
>>
>> For one, IRQs that had their vector set up by Xen internally without a
>> handler ever having got set (e.g. via "com<n>=..." without a matching
>> consumer option like "console=com<n>") would wrongly call
>> add_pin_to_irq() here, triggering the BUG_ON() in that function.
>>
>> Second, when assign_irq_vector() fails this addition to irq_2_pin[]
>> needs to be undone.
>>
>> In the context of this I'm also surprised that the irq_2_pin[]
>> manipulations here occur without any lock, i.e. rely on Dom0 to do
>> some sort of serialization.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> No-one having any opinion? I'm hesitant to commit changes like
> this without anyone having said any word...
Yuk, well, the motivation is good and the code looks sane, so...
Acked-by: Keir Fraser <keir@xen.org>
> Jan
>
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
>> share_vector_maps(irq_2_pin[irq].apic, apic);
>> }
>>
>> +static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
>> +{
>> + struct irq_pin_list *entry, *prev;
>> +
>> + for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
>> + if ((entry->apic == apic) && (entry->pin == pin))
>> + break;
>> + BUG_ON(!entry->next);
>> + }
>> +
>> + entry->pin = entry->apic = -1;
>> +
>> + if (entry != &irq_2_pin[irq]) {
>> + /* Removed entry is not at head of list. */
>> + prev = &irq_2_pin[irq];
>> + while (&irq_2_pin[prev->next] != entry)
>> + prev = &irq_2_pin[prev->next];
>> + prev->next = entry->next;
>> + } else if (entry->next) {
>> + /* Removed entry is at head of multi-item list. */
>> + prev = entry;
>> + entry = &irq_2_pin[entry->next];
>> + *prev = *entry;
>> + entry->pin = entry->apic = -1;
>> + } else
>> + return;
>> +
>> + entry->next = irq_2_pin_free_entry;
>> + irq_2_pin_free_entry = entry - irq_2_pin;
>> +}
>> +
>> /*
>> * Reroute an IRQ to a different pin.
>> */
>> @@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
>>
>> int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
>> {
>> - int apic, pin, irq, ret, vector, pirq;
>> + int apic, pin, irq, ret, pirq;
>> struct IO_APIC_route_entry rte = { 0 };
>> unsigned long flags;
>> struct irq_desc *desc;
>> @@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
>> return 0;
>> }
>>
>> - if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
>> {
>> - add_pin_to_irq(irq, apic, pin);
>> - vector = assign_irq_vector(irq, NULL);
>> - if ( vector < 0 )
>> - return vector;
>> + if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
>> + {
>> + int vector = desc->arch.vector;
>> +
>> + if ( vector < FIRST_HIPRIORITY_VECTOR )
>> + add_pin_to_irq(irq, apic, pin);
>> + else
>> + desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
>> + ret = assign_irq_vector(irq, NULL);
>> + if ( ret < 0 )
>> + {
>> + if ( vector < FIRST_HIPRIORITY_VECTOR )
>> + remove_pin_from_irq(irq, apic, pin);
>> + else
>> + desc->arch.vector = vector;
>> + return ret;
>> + }
>>
>> - printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector,
>> irq);
>> + printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
>> }
>> spin_lock(&dom0->event_lock);
>> ret = map_domain_pirq(dom0, pirq, irq,
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases
2013-05-14 14:52 ` Ping: " Jan Beulich
2013-05-14 15:17 ` Keir Fraser
@ 2013-05-15 10:57 ` Andrew Cooper
2013-05-16 12:47 ` Zhang, Xiantao
2 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2013-05-15 10:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir (Xen.org), xiantao.zhang@intel.com, xen-devel
On 14/05/13 15:52, Jan Beulich wrote:
>>>> On 08.05.13 at 14:51, "Jan Beulich" <JBeulich@suse.com> wrote:
>> This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
>> hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
>>
>> For one, IRQs that had their vector set up by Xen internally without a
>> handler ever having got set (e.g. via "com<n>=..." without a matching
>> consumer option like "console=com<n>") would wrongly call
>> add_pin_to_irq() here, triggering the BUG_ON() in that function.
>>
>> Second, when assign_irq_vector() fails this addition to irq_2_pin[]
>> needs to be undone.
>>
>> In the context of this I'm also surprised that the irq_2_pin[]
>> manipulations here occur without any lock, i.e. rely on Dom0 to do
>> some sort of serialization.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> No-one having any opinion? I'm hesitant to commit changes like
> this without anyone having said any word...
>
> Jan
Sorry - this was on my list to look at but I have been very busy recently.
It tentatively looks good to me.
~Andrew
>
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
>> share_vector_maps(irq_2_pin[irq].apic, apic);
>> }
>>
>> +static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
>> +{
>> + struct irq_pin_list *entry, *prev;
>> +
>> + for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
>> + if ((entry->apic == apic) && (entry->pin == pin))
>> + break;
>> + BUG_ON(!entry->next);
>> + }
>> +
>> + entry->pin = entry->apic = -1;
>> +
>> + if (entry != &irq_2_pin[irq]) {
>> + /* Removed entry is not at head of list. */
>> + prev = &irq_2_pin[irq];
>> + while (&irq_2_pin[prev->next] != entry)
>> + prev = &irq_2_pin[prev->next];
>> + prev->next = entry->next;
>> + } else if (entry->next) {
>> + /* Removed entry is at head of multi-item list. */
>> + prev = entry;
>> + entry = &irq_2_pin[entry->next];
>> + *prev = *entry;
>> + entry->pin = entry->apic = -1;
>> + } else
>> + return;
>> +
>> + entry->next = irq_2_pin_free_entry;
>> + irq_2_pin_free_entry = entry - irq_2_pin;
>> +}
>> +
>> /*
>> * Reroute an IRQ to a different pin.
>> */
>> @@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
>>
>> int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
>> {
>> - int apic, pin, irq, ret, vector, pirq;
>> + int apic, pin, irq, ret, pirq;
>> struct IO_APIC_route_entry rte = { 0 };
>> unsigned long flags;
>> struct irq_desc *desc;
>> @@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
>> return 0;
>> }
>>
>> - if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) {
>> - add_pin_to_irq(irq, apic, pin);
>> - vector = assign_irq_vector(irq, NULL);
>> - if ( vector < 0 )
>> - return vector;
>> + if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR )
>> + {
>> + int vector = desc->arch.vector;
>> +
>> + if ( vector < FIRST_HIPRIORITY_VECTOR )
>> + add_pin_to_irq(irq, apic, pin);
>> + else
>> + desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
>> + ret = assign_irq_vector(irq, NULL);
>> + if ( ret < 0 )
>> + {
>> + if ( vector < FIRST_HIPRIORITY_VECTOR )
>> + remove_pin_from_irq(irq, apic, pin);
>> + else
>> + desc->arch.vector = vector;
>> + return ret;
>> + }
>>
>> - printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, irq);
>> + printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
>> }
>> spin_lock(&dom0->event_lock);
>> ret = map_domain_pirq(dom0, pirq, irq,
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases
2013-05-14 14:52 ` Ping: " Jan Beulich
2013-05-14 15:17 ` Keir Fraser
2013-05-15 10:57 ` Andrew Cooper
@ 2013-05-16 12:47 ` Zhang, Xiantao
2 siblings, 0 replies; 5+ messages in thread
From: Zhang, Xiantao @ 2013-05-16 12:47 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Keir Fraser; +Cc: Zhang, Xiantao, xen-devel
Looks fine to me. Acked, Thanks!
Xiantao
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, May 14, 2013 10:52 PM
> To: Andrew Cooper; Zhang, Xiantao; Keir Fraser
> Cc: xen-devel
> Subject: Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases
>
> >>> On 08.05.13 at 14:51, "Jan Beulich" <JBeulich@suse.com> wrote:
> > This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the
> > hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"):
> >
> > For one, IRQs that had their vector set up by Xen internally without a
> > handler ever having got set (e.g. via "com<n>=..." without a matching
> > consumer option like "console=com<n>") would wrongly call
> > add_pin_to_irq() here, triggering the BUG_ON() in that function.
> >
> > Second, when assign_irq_vector() fails this addition to irq_2_pin[]
> > needs to be undone.
> >
> > In the context of this I'm also surprised that the irq_2_pin[]
> > manipulations here occur without any lock, i.e. rely on Dom0 to do
> > some sort of serialization.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> No-one having any opinion? I'm hesitant to commit changes like
> this without anyone having said any word...
>
> Jan
>
> > --- a/xen/arch/x86/io_apic.c
> > +++ b/xen/arch/x86/io_apic.c
> > @@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int
> > share_vector_maps(irq_2_pin[irq].apic, apic);
> > }
> >
> > +static void remove_pin_from_irq(unsigned int irq, int apic, int pin)
> > +{
> > + struct irq_pin_list *entry, *prev;
> > +
> > + for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) {
> > + if ((entry->apic == apic) && (entry->pin == pin))
> > + break;
> > + BUG_ON(!entry->next);
> > + }
> > +
> > + entry->pin = entry->apic = -1;
> > +
> > + if (entry != &irq_2_pin[irq]) {
> > + /* Removed entry is not at head of list. */
> > + prev = &irq_2_pin[irq];
> > + while (&irq_2_pin[prev->next] != entry)
> > + prev = &irq_2_pin[prev->next];
> > + prev->next = entry->next;
> > + } else if (entry->next) {
> > + /* Removed entry is at head of multi-item list. */
> > + prev = entry;
> > + entry = &irq_2_pin[entry->next];
> > + *prev = *entry;
> > + entry->pin = entry->apic = -1;
> > + } else
> > + return;
> > +
> > + entry->next = irq_2_pin_free_entry;
> > + irq_2_pin_free_entry = entry - irq_2_pin;
> > +}
> > +
> > /*
> > * Reroute an IRQ to a different pin.
> > */
> > @@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys
> >
> > int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
> > {
> > - int apic, pin, irq, ret, vector, pirq;
> > + int apic, pin, irq, ret, pirq;
> > struct IO_APIC_route_entry rte = { 0 };
> > unsigned long flags;
> > struct irq_desc *desc;
> > @@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy
> > return 0;
> > }
> >
> > - if ( desc->arch.vector <= 0 || desc->arch.vector >
> LAST_DYNAMIC_VECTOR ) {
> > - add_pin_to_irq(irq, apic, pin);
> > - vector = assign_irq_vector(irq, NULL);
> > - if ( vector < 0 )
> > - return vector;
> > + if ( desc->arch.vector <= 0 || desc->arch.vector >
> LAST_DYNAMIC_VECTOR )
> > + {
> > + int vector = desc->arch.vector;
> > +
> > + if ( vector < FIRST_HIPRIORITY_VECTOR )
> > + add_pin_to_irq(irq, apic, pin);
> > + else
> > + desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
> > + ret = assign_irq_vector(irq, NULL);
> > + if ( ret < 0 )
> > + {
> > + if ( vector < FIRST_HIPRIORITY_VECTOR )
> > + remove_pin_from_irq(irq, apic, pin);
> > + else
> > + desc->arch.vector = vector;
> > + return ret;
> > + }
> >
> > - printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, irq);
> > + printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
> > }
> > spin_lock(&dom0->event_lock);
> > ret = map_domain_pirq(dom0, pirq, irq,
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-16 12:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-08 12:51 [PATCH] x86/IO-APIC: fix guest RTE write corner cases Jan Beulich
2013-05-14 14:52 ` Ping: " Jan Beulich
2013-05-14 15:17 ` Keir Fraser
2013-05-15 10:57 ` Andrew Cooper
2013-05-16 12:47 ` Zhang, Xiantao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.