[patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Suresh Siddha]

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

After talking to some more folks inside intel (Peter Anvin, Asit Mallick),
the safest option (for future compatibility etc) seen was to use vector 0x20
for IRQ_MOVE_CLEANUP_VECTOR instead of using vector 0x1f (which is documented as
reserved vector in the Intel IA32 manuals).

Also we don't need to reserve the entire privilege level (all 16 vectors in
the priority bucket that IRQ_MOVE_CLEANUP_VECTOR falls into), as the
x86 architecture (section 10.9.3 in SDM Vol3a) specifies that with in the
priority level, the higher the vector number the higher the priority.
And hence we don't need to reserve the complete priority level 0x20-0x2f for
the IRQ migration cleanup logic.

So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
also can be migrated in the context of IOAPIC and hence need to be at a higher
priority level than IRQ_MOVE_CLEANUP_VECTOR).

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index 585a428..708322a 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -28,19 +28,15 @@
 #define MCE_VECTOR			0x12
 
 /*
- * IDT vectors usable for external interrupt sources start
- * at 0x20:
- * hpa said we can start from 0x1f.
- *   0x1f is documented as reserved.  However, the ability for the APIC
- *   to generate vectors starting at 0x10 is documented, as is the
- *   ability for the CPU to receive any vector number as an interrupt.
- *   0x1f is used for IRQ_MOVE_CLEANUP_VECTOR since that vector needs
- *   an entire privilege level (16 vectors) all by itself at a higher
- *   priority than any actual device vector.  Thus, by placing it in the
- *   otherwise-unusable 0x10 privilege level, we avoid wasting a full
- *   16-vector block.
+ * IDT vectors usable for external interrupt sources start at 0x20.
  */
-#define FIRST_EXTERNAL_VECTOR		0x1f
+#define FIRST_EXTERNAL_VECTOR		0x20
+
+/*
+ * Reserve the lowest usable vector (and hence lowest priority)  0x20 for
+ * triggering cleanup after irq migration.
+ */
+#define IRQ_MOVE_CLEANUP_VECTOR		FIRST_EXTERNAL_VECTOR
 
 #define IA32_SYSCALL_VECTOR		0x80
 #ifdef CONFIG_X86_32
@@ -48,17 +44,7 @@
 #endif
 
 /*
- * Reserve the lowest usable priority level 0x10 - 0x1f for triggering
- * cleanup after irq migration.
- * this overlaps with the reserved range for cpu exceptions so this
- * will need to be changed to 0x20 - 0x2f if the last cpu exception is
- * ever allocated.
- */
-
-#define IRQ_MOVE_CLEANUP_VECTOR		FIRST_EXTERNAL_VECTOR
-
-/*
- * Vectors 0x20-0x2f are used for ISA interrupts.
+ * Vectors 0x30-0x3f are used for ISA interrupts.
  *   round up to the next 16-vector boundary
  */
 #define IRQ0_VECTOR			((FIRST_EXTERNAL_VECTOR + 16) & ~15)
@@ -80,6 +66,12 @@
 #define IRQ15_VECTOR			(IRQ0_VECTOR + 15)
 
 /*
+ * First APIC vector available to drivers: (vectors 0x21-0xee).
+ * (0x80 is the syscall vector, 0x30-0x3f are for ISA)
+ */
+#define FIRST_DEVICE_VECTOR		(FIRST_EXTERNAL_VECTOR + 1)
+
+/*
  * Special IRQ vectors used by the SMP architecture, 0xf0-0xff
  *
  *  some of the following vectors are 'rare', they are merged
@@ -132,14 +124,6 @@
  */
 #define MCE_SELF_VECTOR			0xeb
 
-/*
- * First APIC vector available to drivers: (vectors 0x30-0xee).  We
- * start allocating at 0x31 to spread out vectors evenly between
- * priority levels. (0x80 is the syscall vector)
- */
-#define FIRST_DEVICE_VECTOR		(IRQ15_VECTOR + 1)
-#define VECTOR_OFFSET_START		1
-
 #define NR_VECTORS			 256
 
 #define FPU_IRQ				  13
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index d5bfa29..5c090a1 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
 	 * Also, we've got to be careful not to trash gate
 	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
 	 */
-	static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
-	static int current_offset = VECTOR_OFFSET_START % 8;
+	static int current_vector = FIRST_DEVICE_VECTOR;
+	static int current_offset = 0;
 	unsigned int old_vector;
 	int cpu, err;
 	cpumask_var_t tmp_mask;

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/08/2010 06:09 PM, Suresh Siddha wrote:
> 
> So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
> for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
> also can be migrated in the context of IOAPIC and hence need to be at a higher
> priority level than IRQ_MOVE_CLEANUP_VECTOR).
> 

You're referring to when they're accessed as IOAPIC interrupts as
opposed to ExtInt interrupts?

>  
> -/*
> - * First APIC vector available to drivers: (vectors 0x30-0xee).  We
> - * start allocating at 0x31 to spread out vectors evenly between
> - * priority levels. (0x80 is the syscall vector)
> - */
> -#define FIRST_DEVICE_VECTOR		(IRQ15_VECTOR + 1)
> -#define VECTOR_OFFSET_START		1
> -
>  #define NR_VECTORS			 256
>  
>  #define FPU_IRQ				  13
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index d5bfa29..5c090a1 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
>  	 * Also, we've got to be careful not to trash gate
>  	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
>  	 */
> -	static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
> -	static int current_offset = VECTOR_OFFSET_START % 8;
> +	static int current_vector = FIRST_DEVICE_VECTOR;
> +	static int current_offset = 0;
>  	unsigned int old_vector;
>  	int cpu, err;
>  	cpumask_var_t tmp_mask;
> 

I'm not entirely sure I like losing this bit, even though it isn't
really necessary with your changes (VECTOR_OFFSET_START would be 0).
I'm afraid we might end up with the same buglet being "reinvented" later.

However, my most serious concern with this patch is that there is a
fairly significant change due to this patch, which is that the legacy
IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
a bad thing -- in fact, it is fundamentally the right thing to do
especially once we consider platforms which *don't* have the legacy IRQs
-- but it makes me scared of unexpected behavior changes as a result.
If you feel confident that that is not the case, could you outline why
it shouldn't be a problem?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Yinghai Lu]

On Fri, Jan 8, 2010 at 6:19 PM, H. Peter Anvin <hpa@zytor.com> wrote:

> However, my most serious concern with this patch is that there is a
> fairly significant change due to this patch, which is that the legacy
> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
> a bad thing -- in fact, it is fundamentally the right thing to do
> especially once we consider platforms which *don't* have the legacy IRQs
> -- but it makes me scared of unexpected behavior changes as a result.
> If you feel confident that that is not the case, could you outline why
> it shouldn't be a problem?
>

if it is not used by ioapic instead of legacy,
do we need to re assign irq0 to irq15 domain from all cpus to
apic->vector_allocation_domain()?

YH

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Yinghai Lu]

On Fri, Jan 8, 2010 at 6:09 PM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f
>
> After talking to some more folks inside intel (Peter Anvin, Asit Mallick),
> the safest option (for future compatibility etc) seen was to use vector 0x20
> for IRQ_MOVE_CLEANUP_VECTOR instead of using vector 0x1f (which is documented as
> reserved vector in the Intel IA32 manuals).
>
> Also we don't need to reserve the entire privilege level (all 16 vectors in
> the priority bucket that IRQ_MOVE_CLEANUP_VECTOR falls into), as the
> x86 architecture (section 10.9.3 in SDM Vol3a) specifies that with in the
> priority level, the higher the vector number the higher the priority.
> And hence we don't need to reserve the complete priority level 0x20-0x2f for
> the IRQ migration cleanup logic.
>
> So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
> for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
> also can be migrated in the context of IOAPIC and hence need to be at a higher
> priority level than IRQ_MOVE_CLEANUP_VECTOR).
>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
>
> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
> index 585a428..708322a 100644
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> @@ -28,19 +28,15 @@
>  #define MCE_VECTOR                     0x12
>
>  /*
> - * IDT vectors usable for external interrupt sources start
> - * at 0x20:
> - * hpa said we can start from 0x1f.
> - *   0x1f is documented as reserved.  However, the ability for the APIC
> - *   to generate vectors starting at 0x10 is documented, as is the
> - *   ability for the CPU to receive any vector number as an interrupt.
> - *   0x1f is used for IRQ_MOVE_CLEANUP_VECTOR since that vector needs
> - *   an entire privilege level (16 vectors) all by itself at a higher
> - *   priority than any actual device vector.  Thus, by placing it in the
> - *   otherwise-unusable 0x10 privilege level, we avoid wasting a full
> - *   16-vector block.
> + * IDT vectors usable for external interrupt sources start at 0x20.
>  */
> -#define FIRST_EXTERNAL_VECTOR          0x1f
> +#define FIRST_EXTERNAL_VECTOR          0x20
> +
> +/*
> + * Reserve the lowest usable vector (and hence lowest priority)  0x20 for
> + * triggering cleanup after irq migration.
> + */
> +#define IRQ_MOVE_CLEANUP_VECTOR                FIRST_EXTERNAL_VECTOR
>
>  #define IA32_SYSCALL_VECTOR            0x80
>  #ifdef CONFIG_X86_32
> @@ -48,17 +44,7 @@
>  #endif
>
>  /*
> - * Reserve the lowest usable priority level 0x10 - 0x1f for triggering
> - * cleanup after irq migration.
> - * this overlaps with the reserved range for cpu exceptions so this
> - * will need to be changed to 0x20 - 0x2f if the last cpu exception is
> - * ever allocated.
> - */
> -
> -#define IRQ_MOVE_CLEANUP_VECTOR                FIRST_EXTERNAL_VECTOR
> -
> -/*
> - * Vectors 0x20-0x2f are used for ISA interrupts.
> + * Vectors 0x30-0x3f are used for ISA interrupts.
>  *   round up to the next 16-vector boundary
>  */
>  #define IRQ0_VECTOR                    ((FIRST_EXTERNAL_VECTOR + 16) & ~15)
> @@ -80,6 +66,12 @@
>  #define IRQ15_VECTOR                   (IRQ0_VECTOR + 15)
>
>  /*
> + * First APIC vector available to drivers: (vectors 0x21-0xee).
> + * (0x80 is the syscall vector, 0x30-0x3f are for ISA)
> + */
> +#define FIRST_DEVICE_VECTOR            (FIRST_EXTERNAL_VECTOR + 1)
> +
> +/*
>  * Special IRQ vectors used by the SMP architecture, 0xf0-0xff
>  *
>  *  some of the following vectors are 'rare', they are merged
> @@ -132,14 +124,6 @@
>  */
>  #define MCE_SELF_VECTOR                        0xeb
>
> -/*
> - * First APIC vector available to drivers: (vectors 0x30-0xee).  We
> - * start allocating at 0x31 to spread out vectors evenly between
> - * priority levels. (0x80 is the syscall vector)
> - */
> -#define FIRST_DEVICE_VECTOR            (IRQ15_VECTOR + 1)
> -#define VECTOR_OFFSET_START            1
> -
>  #define NR_VECTORS                      256
>
>  #define FPU_IRQ                                  13
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index d5bfa29..5c090a1 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
>         * Also, we've got to be careful not to trash gate
>         * 0x80, because int 0x80 is hm, kind of importantish. ;)
>         */
> -       static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
> -       static int current_offset = VECTOR_OFFSET_START % 8;
> +       static int current_vector = FIRST_DEVICE_VECTOR;
> +       static int current_offset = 0;
>        unsigned int old_vector;
>        int cpu, err;
>        cpumask_var_t tmp_mask;
>

looks like it returned to first version i suggested?

http://lkml.org/lkml/2010/1/4/26

maybe we can just kill FIRST_DEVICE_VECTOR...

YH

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/08/2010 07:07 PM, Yinghai Lu wrote:
>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> index d5bfa29..5c090a1 100644
>> --- a/arch/x86/kernel/apic/io_apic.c
>> +++ b/arch/x86/kernel/apic/io_apic.c
>> @@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
>>         * Also, we've got to be careful not to trash gate
>>         * 0x80, because int 0x80 is hm, kind of importantish. ;)
>>         */
>> -       static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
>> -       static int current_offset = VECTOR_OFFSET_START % 8;
>> +       static int current_vector = FIRST_DEVICE_VECTOR;
>> +       static int current_offset = 0;
>>        unsigned int old_vector;
>>        int cpu, err;
>>        cpumask_var_t tmp_mask;
>>
> 
> looks like it returned to first version i suggested?
> 
> http://lkml.org/lkml/2010/1/4/26
> 
> maybe we can just kill FIRST_DEVICE_VECTOR...
> 

As I already said, I really don't like reverting this bit.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/08/2010 06:09 PM, Suresh Siddha wrote:
> From: Suresh Siddha <suresh.b.siddha@intel.com>
> Subject: x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f
> 
> After talking to some more folks inside intel (Peter Anvin, Asit Mallick),
> the safest option (for future compatibility etc) seen was to use vector 0x20
> for IRQ_MOVE_CLEANUP_VECTOR instead of using vector 0x1f (which is documented as
> reserved vector in the Intel IA32 manuals).
> 
> Also we don't need to reserve the entire privilege level (all 16 vectors in
> the priority bucket that IRQ_MOVE_CLEANUP_VECTOR falls into), as the
> x86 architecture (section 10.9.3 in SDM Vol3a) specifies that with in the
> priority level, the higher the vector number the higher the priority.
> And hence we don't need to reserve the complete priority level 0x20-0x2f for
> the IRQ migration cleanup logic.
> 
> So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
> for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
> also can be migrated in the context of IOAPIC and hence need to be at a higher
> priority level than IRQ_MOVE_CLEANUP_VECTOR).
> 

I guess another thing is whether or not we can allocate 0x20 as a normal
vector if we don't need IRQ_MOVE_CLEANUP_VECTORS, and rely on the
used_vectors for that one as well.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Suresh Siddha]

On Fri, 2010-01-08 at 18:19 -0800, H. Peter Anvin wrote:
> On 01/08/2010 06:09 PM, Suresh Siddha wrote:
> > 
> > So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
> > for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
> > also can be migrated in the context of IOAPIC and hence need to be at a higher
> > priority level than IRQ_MOVE_CLEANUP_VECTOR).
> > 
> 
> You're referring to when they're accessed as IOAPIC interrupts as
> opposed to ExtInt interrupts?

yes.

> > -/*
> > - * First APIC vector available to drivers: (vectors 0x30-0xee).  We
> > - * start allocating at 0x31 to spread out vectors evenly between
> > - * priority levels. (0x80 is the syscall vector)
> > - */
> > -#define FIRST_DEVICE_VECTOR		(IRQ15_VECTOR + 1)
> > -#define VECTOR_OFFSET_START		1
> > -
> >  #define NR_VECTORS			 256
> >  
> >  #define FPU_IRQ				  13
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index d5bfa29..5c090a1 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
> >  	 * Also, we've got to be careful not to trash gate
> >  	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
> >  	 */
> > -	static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
> > -	static int current_offset = VECTOR_OFFSET_START % 8;
> > +	static int current_vector = FIRST_DEVICE_VECTOR;
> > +	static int current_offset = 0;
> >  	unsigned int old_vector;
> >  	int cpu, err;
> >  	cpumask_var_t tmp_mask;
> > 
> 
> I'm not entirely sure I like losing this bit, even though it isn't
> really necessary with your changes (VECTOR_OFFSET_START would be 0).
> I'm afraid we might end up with the same buglet being "reinvented" later.

Ok. We should be able to retain that bit. I will send another version
shortly.

> However, my most serious concern with this patch is that there is a
> fairly significant change due to this patch, which is that the legacy
> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
> a bad thing -- in fact, it is fundamentally the right thing to do
> especially once we consider platforms which *don't* have the legacy IRQs
> -- but it makes me scared of unexpected behavior changes as a result.
> If you feel confident that that is not the case, could you outline why
> it shouldn't be a problem?

In irqinit.c, we statically pre-assign the per-cpu vector to irq
mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
is statically initialized for legacy IRQ's in io_apic.c. So we won't be
able to use this space for anything else.

thanks,
suresh

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 02:53 PM, Suresh Siddha wrote:
> 
>> However, my most serious concern with this patch is that there is a
>> fairly significant change due to this patch, which is that the legacy
>> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
>> a bad thing -- in fact, it is fundamentally the right thing to do
>> especially once we consider platforms which *don't* have the legacy IRQs
>> -- but it makes me scared of unexpected behavior changes as a result.
>> If you feel confident that that is not the case, could you outline why
>> it shouldn't be a problem?
> 
> In irqinit.c, we statically pre-assign the per-cpu vector to irq
> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
> able to use this space for anything else.
> 

What enforces that, though?  The used_vector bitmap?  In the past it was
enforced simply by being < FIRST_DEVICE_VECTOR.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

Suresh Siddha <suresh.b.siddha@intel.com> writes:

> On Fri, 2010-01-08 at 18:19 -0800, H. Peter Anvin wrote:
>> On 01/08/2010 06:09 PM, Suresh Siddha wrote:
>> > 
>> > So change the IRQ_MOVE_CLEANUP_VECTOR to 0x20 and  allow 0x21-0x2f to be used
>> > for device interrupts. 0x30-0x3f will be used for ISA interrupts (these
>> > also can be migrated in the context of IOAPIC and hence need to be at a higher
>> > priority level than IRQ_MOVE_CLEANUP_VECTOR).
>> > 
>> 
>> You're referring to when they're accessed as IOAPIC interrupts as
>> opposed to ExtInt interrupts?
>
> yes.
>
>> > -/*
>> > - * First APIC vector available to drivers: (vectors 0x30-0xee).  We
>> > - * start allocating at 0x31 to spread out vectors evenly between
>> > - * priority levels. (0x80 is the syscall vector)
>> > - */
>> > -#define FIRST_DEVICE_VECTOR		(IRQ15_VECTOR + 1)
>> > -#define VECTOR_OFFSET_START		1
>> > -
>> >  #define NR_VECTORS			 256
>> >  
>> >  #define FPU_IRQ				  13
>> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>> > index d5bfa29..5c090a1 100644
>> > --- a/arch/x86/kernel/apic/io_apic.c
>> > +++ b/arch/x86/kernel/apic/io_apic.c
>> > @@ -1162,8 +1162,8 @@ __assign_irq_vector(int irq, struct irq_cfg *cfg, const struct cpumask *mask)
>> >  	 * Also, we've got to be careful not to trash gate
>> >  	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
>> >  	 */
>> > -	static int current_vector = FIRST_DEVICE_VECTOR + VECTOR_OFFSET_START;
>> > -	static int current_offset = VECTOR_OFFSET_START % 8;
>> > +	static int current_vector = FIRST_DEVICE_VECTOR;
>> > +	static int current_offset = 0;
>> >  	unsigned int old_vector;
>> >  	int cpu, err;
>> >  	cpumask_var_t tmp_mask;
>> > 
>> 
>> I'm not entirely sure I like losing this bit, even though it isn't
>> really necessary with your changes (VECTOR_OFFSET_START would be 0).
>> I'm afraid we might end up with the same buglet being "reinvented" later.
>
> Ok. We should be able to retain that bit. I will send another version
> shortly.
>
>> However, my most serious concern with this patch is that there is a
>> fairly significant change due to this patch, which is that the legacy
>> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
>> a bad thing -- in fact, it is fundamentally the right thing to do
>> especially once we consider platforms which *don't* have the legacy IRQs
>> -- but it makes me scared of unexpected behavior changes as a result.
>> If you feel confident that that is not the case, could you outline why
>> it shouldn't be a problem?
>
> In irqinit.c, we statically pre-assign the per-cpu vector to irq
> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
> able to use this space for anything else.

Last I looked when we switched legacy irqs irqs from pic mode to io_apic mode
we continue to use the same vector.

Which means we should not be wasting those vectors and that it is dangerous
to play lowest priority irq games in that range.

Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 03:00 PM, Eric W. Biederman wrote:
>>
>> In irqinit.c, we statically pre-assign the per-cpu vector to irq
>> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
>> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
>> able to use this space for anything else.
> 
> Last I looked when we switched legacy irqs irqs from pic mode to io_apic mode
> we continue to use the same vector.
> 
> Which means we should not be wasting those vectors and that it is dangerous
> to play lowest priority irq games in that range.
> 

This statement made no sense to me?

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 01/11/2010 02:53 PM, Suresh Siddha wrote:
>> 
>>> However, my most serious concern with this patch is that there is a
>>> fairly significant change due to this patch, which is that the legacy
>>> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
>>> a bad thing -- in fact, it is fundamentally the right thing to do
>>> especially once we consider platforms which *don't* have the legacy IRQs
>>> -- but it makes me scared of unexpected behavior changes as a result.
>>> If you feel confident that that is not the case, could you outline why
>>> it shouldn't be a problem?
>> 
>> In irqinit.c, we statically pre-assign the per-cpu vector to irq
>> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
>> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
>> able to use this space for anything else.
>> 
>
> What enforces that, though?  The used_vector bitmap?  In the past it was
> enforced simply by being < FIRST_DEVICE_VECTOR.

I believe historically it was simply that we did not loop over that set of vectors,
in assign_irq_vector.

Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 03:10 PM, Eric W. Biederman wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> On 01/11/2010 02:53 PM, Suresh Siddha wrote:
>>>
>>>> However, my most serious concern with this patch is that there is a
>>>> fairly significant change due to this patch, which is that the legacy
>>>> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
>>>> a bad thing -- in fact, it is fundamentally the right thing to do
>>>> especially once we consider platforms which *don't* have the legacy IRQs
>>>> -- but it makes me scared of unexpected behavior changes as a result.
>>>> If you feel confident that that is not the case, could you outline why
>>>> it shouldn't be a problem?
>>>
>>> In irqinit.c, we statically pre-assign the per-cpu vector to irq
>>> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
>>> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
>>> able to use this space for anything else.
>>>
>>
>> What enforces that, though?  The used_vector bitmap?  In the past it was
>> enforced simply by being < FIRST_DEVICE_VECTOR.
> 
> I believe historically it was simply that we did not loop over that set of vectors,
> in assign_irq_vector.
> 

Yes, that's what I said.  My question was to Suresh what enforces that
in the case of his patch, which moves the legacy range into the middle
of the device vectors.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Suresh Siddha]

On Mon, 2010-01-11 at 15:13 -0800, H. Peter Anvin wrote:
> On 01/11/2010 03:10 PM, Eric W. Biederman wrote:
> > "H. Peter Anvin" <hpa@zytor.com> writes:
> > 
> >> On 01/11/2010 02:53 PM, Suresh Siddha wrote:
> >>>
> >>>> However, my most serious concern with this patch is that there is a
> >>>> fairly significant change due to this patch, which is that the legacy
> >>>> IRQ vectors now fall *inside* the FIRST_DEVICE_VECTOR range.  This isn't
> >>>> a bad thing -- in fact, it is fundamentally the right thing to do
> >>>> especially once we consider platforms which *don't* have the legacy IRQs
> >>>> -- but it makes me scared of unexpected behavior changes as a result.
> >>>> If you feel confident that that is not the case, could you outline why
> >>>> it shouldn't be a problem?
> >>>
> >>> In irqinit.c, we statically pre-assign the per-cpu vector to irq
> >>> mappings (vector_irq) for all the legacy IRQ vectors. Similarly irq_cfg
> >>> is statically initialized for legacy IRQ's in io_apic.c. So we won't be
> >>> able to use this space for anything else.
> >>>
> >>
> >> What enforces that, though?  The used_vector bitmap?  In the past it was
> >> enforced simply by being < FIRST_DEVICE_VECTOR.
> > 
> > I believe historically it was simply that we did not loop over that set of vectors,
> > in assign_irq_vector.
> > 
> 
> Yes, that's what I said.  My question was to Suresh what enforces that
> in the case of his patch, which moves the legacy range into the middle
> of the device vectors.

It's not the used_vector bitmap. That range will appear as used on all
the cpu's and hence we won't be allocating it for anything else.

Now the question is: for non-legacy (io-apic) case, instead of reserving
this range for all the cpu's, does it make sense to generalize like any
other vector?

thanks,
suresh

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 04:06 PM, Suresh Siddha wrote:
>>
>> Yes, that's what I said.  My question was to Suresh what enforces that
>> in the case of his patch, which moves the legacy range into the middle
>> of the device vectors.
> 
> It's not the used_vector bitmap. That range will appear as used on all
> the cpu's and hence we won't be allocating it for anything else.
> 

OK, fair enough.

> Now the question is: for non-legacy (io-apic) case, instead of reserving
> this range for all the cpu's, does it make sense to generalize like any
> other vector?

It sounds like something that we could experiment with -- after
switching an IRQ to ioapic mode, make it a movable interrupt.  It
*seems* it should work, but it's scary stuff to muck with.

Eric, do you see any reason why it wouldn't work?  I truly couldn't
understand your previous remark, especially the bit about "it is
dangerous to play lowest priority irq games in that range".

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 01/11/2010 04:06 PM, Suresh Siddha wrote:
>>>
>>> Yes, that's what I said.  My question was to Suresh what enforces that
>>> in the case of his patch, which moves the legacy range into the middle
>>> of the device vectors.
>> 
>> It's not the used_vector bitmap. That range will appear as used on all
>> the cpu's and hence we won't be allocating it for anything else.
>> 
>
> OK, fair enough.
>
>> Now the question is: for non-legacy (io-apic) case, instead of reserving
>> this range for all the cpu's, does it make sense to generalize like any
>> other vector?
>
> It sounds like something that we could experiment with -- after
> switching an IRQ to ioapic mode, make it a movable interrupt.  It
> *seems* it should work, but it's scary stuff to muck with.
>
> Eric, do you see any reason why it wouldn't work?  I truly couldn't
> understand your previous remark, especially the bit about "it is
> dangerous to play lowest priority irq games in that range".

Sorry.  I suck at multitasking.

Without changes assign_irq_vector will reuse vectors in the range
IRQ0_VECTOR to IRQ15_VECTOR in the code as it we currently ship it,
when we switch irq0-15 into ioapic mode.

Switching the loop to cover IRQ0_VECTOR to IRQ15_VECTOR is not a
problem.  I don't think it will find anything free as we assign those
vectors on all cpus, but the data structures are fine.

I am uncomfortable with the suggestion of sharing the priority of the
IRQ_MOVE_CLEANUP_VECTOR with other interrupts.  I know if it had be
clear from the documentation that it was safe to share the irq level
with other interrupts I would not have reserved the entire interrupt
level for the IRQ_MOVE_CLEANUP_VECTOR.

Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 04:28 PM, Eric W. Biederman wrote:
> 
> Sorry.  I suck at multitasking.
> 
> Without changes assign_irq_vector will reuse vectors in the range
> IRQ0_VECTOR to IRQ15_VECTOR in the code as it we currently ship it,
> when we switch irq0-15 into ioapic mode.
> 
> Switching the loop to cover IRQ0_VECTOR to IRQ15_VECTOR is not a
> problem.  I don't think it will find anything free as we assign those
> vectors on all cpus, but the data structures are fine.
> 
> I am uncomfortable with the suggestion of sharing the priority of the
> IRQ_MOVE_CLEANUP_VECTOR with other interrupts.  I know if it had be
> clear from the documentation that it was safe to share the irq level
> with other interrupts I would not have reserved the entire interrupt
> level for the IRQ_MOVE_CLEANUP_VECTOR.
> 

What are the properties that you're looking for, and in what
documentation?  We reviewed the Software Development Manual here at
Intel, and it rather explicitly states:

"Each interrupt priority level (sometimes interpreted by software as an
interrupt priority class) encompasses 16 vectors. Prioritizing
interrupts within a priority level is determined by the vector number.
The higher the vector number, the higher the priority within that
priority level.  In determining the priority of a vector and ranking
of vectors within a priority group, the vector number is often divided
into two parts, with the high 4 bits of the vector indicating its
priority and the low 4 bit indicating its ranking within the priority
group."

[Intel® 64 and IA-32 Architectures Software Developer’s Manual
Volume 3A: System Programming Guide, Part 1; September 2009, Order
Number 253668-032US; section 10.9.3, page 10-57f.]

So 0x20 is the lowest-priority vector within priority group 0x2.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 04:28 PM, Eric W. Biederman wrote:
> 
> Without changes assign_irq_vector will reuse vectors in the range
> IRQ0_VECTOR to IRQ15_VECTOR in the code as it we currently ship it,
> when we switch irq0-15 into ioapic mode.
> 
> Switching the loop to cover IRQ0_VECTOR to IRQ15_VECTOR is not a
> problem.  I don't think it will find anything free as we assign those
> vectors on all cpus, but the data structures are fine.
> 

The question there is if we can treat the resulting ioapic IRQs as
normal movable IRQs, and just let the target-moving mechanism take care
of it.  After all, there is a discrete event at which we decide that any
particular interrupt is an IOAPIC interrupt instead of XT-PIC.
Obviously, the vectors that remain XT-PIC vectors have to remain
allocated on all vectors for all time.

Another question is why we reserve the legacy IRQ 2 vector at all --
except when ACPI is present!  I don't think it could ever be tickled,
and it sort of felt as a "just in case" thing that could be removed.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 01/11/2010 04:28 PM, Eric W. Biederman wrote:
>> 
>> Sorry.  I suck at multitasking.
>> 
>> Without changes assign_irq_vector will reuse vectors in the range
>> IRQ0_VECTOR to IRQ15_VECTOR in the code as it we currently ship it,
>> when we switch irq0-15 into ioapic mode.
>> 
>> Switching the loop to cover IRQ0_VECTOR to IRQ15_VECTOR is not a
>> problem.  I don't think it will find anything free as we assign those
>> vectors on all cpus, but the data structures are fine.
>> 
>> I am uncomfortable with the suggestion of sharing the priority of the
>> IRQ_MOVE_CLEANUP_VECTOR with other interrupts.  I know if it had be
>> clear from the documentation that it was safe to share the irq level
>> with other interrupts I would not have reserved the entire interrupt
>> level for the IRQ_MOVE_CLEANUP_VECTOR.
>> 
>
> What are the properties that you're looking for, and in what
> documentation?  We reviewed the Software Development Manual here at
> Intel, and it rather explicitly states:
>
> "Each interrupt priority level (sometimes interpreted by software as an
> interrupt priority class) encompasses 16 vectors. Prioritizing
> interrupts within a priority level is determined by the vector number.
> The higher the vector number, the higher the priority within that
> priority level.  In determining the priority of a vector and ranking
> of vectors within a priority group, the vector number is often divided
> into two parts, with the high 4 bits of the vector indicating its
> priority and the low 4 bit indicating its ranking within the priority
> group."
>
> [Intel® 64 and IA-32 Architectures Software Developer’s Manual
> Volume 3A: System Programming Guide, Part 1; September 2009, Order
> Number 253668-032US; section 10.9.3, page 10-57f.]
>
> So 0x20 is the lowest-priority vector within priority group 0x2.

After having the documentation quoted at me.  I am having a distinct
memory of one piece of documentation saying:
"interrupts within a priority level can be delivered in any order"

So I am guessing there is not any ordering of interrupts in the same
priority level until they get to the local apic.

What guarantee we need is the interesting question.

The cleanup ipi is sent when we have seen an interrupt arrive at it's
newly configured location.  It is possible that there is still an
interrupt in flight to the old configured location (think NUMA where
the interrupt has been migrated from off node to on node).  We want
the guarantee that the ipi arrives after the inflight irq.  Which
means on the wire ordering as well as in the local apic ordering is
interesting.

I am slammed with other stuff right now so I don't think I will have
time to find the old documentation I was looking at for a couple of
more days.

Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
> 
> After having the documentation quoted at me.  I am having a distinct
> memory of one piece of documentation saying:
> "interrupts within a priority level can be delivered in any order"
> 
> So I am guessing there is not any ordering of interrupts in the same
> priority level until they get to the local apic.
> 

There is no ordering of interrupts before they hit the local APIC, since
the local APIC is what would serialize them...

> What guarantee we need is the interesting question.
> 
> The cleanup ipi is sent when we have seen an interrupt arrive at it's
> newly configured location.  It is possible that there is still an
> interrupt in flight to the old configured location (think NUMA where
> the interrupt has been migrated from off node to on node).  We want
> the guarantee that the ipi arrives after the inflight irq.  Which
> means on the wire ordering as well as in the local apic ordering is
> interesting.

I don't think there is any such guarantee possible, but that that has
nothing to do with the interrupt priority.  Suresh tells me that that is
handled by detecting and re-posting the migration IRQ.

> I am slammed with other stuff right now so I don't think I will have
> time to find the old documentation I was looking at for a couple of
> more days.

I'm wondering if what you're thinking of are the really old LAPICs which
could only remember two pending interrupts per priority level?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
>> 
>> After having the documentation quoted at me.  I am having a distinct
>> memory of one piece of documentation saying:
>> "interrupts within a priority level can be delivered in any order"
>> 
>> So I am guessing there is not any ordering of interrupts in the same
>> priority level until they get to the local apic.
>> 
>
> There is no ordering of interrupts before they hit the local APIC, since
> the local APIC is what would serialize them...

The two wire bus that the all came across would serialize them.
I'm not certain how much of that was preserved with front-side
bus delivery.

>
>> What guarantee we need is the interesting question.
>> 
>> The cleanup ipi is sent when we have seen an interrupt arrive at it's
>> newly configured location.  It is possible that there is still an
>> interrupt in flight to the old configured location (think NUMA where
>> the interrupt has been migrated from off node to on node).  We want
>> the guarantee that the ipi arrives after the inflight irq.  Which
>> means on the wire ordering as well as in the local apic ordering is
>> interesting.
>
> I don't think there is any such guarantee possible, but that that has
> nothing to do with the interrupt priority.  Suresh tells me that that is
> handled by detecting and re-posting the migration IRQ.

The re-posting should only be for the case where we are migrating more
than one irq at a time.


>> I am slammed with other stuff right now so I don't think I will have
>> time to find the old documentation I was looking at for a couple of
>> more days.
>
> I'm wondering if what you're thinking of are the really old LAPICs which
> could only remember two pending interrupts per priority level?

Not precisely, but it could easily be something similar.


Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Alan Cox]

> I'm wondering if what you're thinking of are the really old LAPICs which
> could only remember two pending interrupts per priority level?

On the really old boards an IPI can arrive more than once which makes
synchronization questions all the more interesting !

Alan

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
>> 
>> After having the documentation quoted at me.  I am having a distinct
>> memory of one piece of documentation saying:
>> "interrupts within a priority level can be delivered in any order"
>> 
>> So I am guessing there is not any ordering of interrupts in the same
>> priority level until they get to the local apic.
>> 
>
> There is no ordering of interrupts before they hit the local APIC, since
> the local APIC is what would serialize them...

The io apic serializes them, and sends them over either the 2-wire
bus or the front side bus.   How much serialization and prioritization
happens at that point I am not certain, but some certainly happens
before you get to the local apic.

Eric

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/13/2010 12:36 PM, Eric W. Biederman wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
>>>
>>> After having the documentation quoted at me.  I am having a distinct
>>> memory of one piece of documentation saying:
>>> "interrupts within a priority level can be delivered in any order"
>>>
>>> So I am guessing there is not any ordering of interrupts in the same
>>> priority level until they get to the local apic.
>>>
>>
>> There is no ordering of interrupts before they hit the local APIC, since
>> the local APIC is what would serialize them...
> 
> The io apic serializes them, and sends them over either the 2-wire
> bus or the front side bus.   How much serialization and prioritization
> happens at that point I am not certain, but some certainly happens
> before you get to the local apic.
> 

If you *have* a front side bus, that is!  With QPI or HyperTransport,
you don't have a single serializing bus in the same way, but you have a
mesh network instead.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/13/2010 12:36 PM, Eric W. Biederman wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
>>>
>>> After having the documentation quoted at me.  I am having a distinct
>>> memory of one piece of documentation saying:
>>> "interrupts within a priority level can be delivered in any order"
>>>
>>> So I am guessing there is not any ordering of interrupts in the same
>>> priority level until they get to the local apic.
>>>
>>
>> There is no ordering of interrupts before they hit the local APIC, since
>> the local APIC is what would serialize them...
> 
> The io apic serializes them, and sends them over either the 2-wire
> bus or the front side bus.   How much serialization and prioritization
> happens at that point I am not certain, but some certainly happens
> before you get to the local apic.
> 

>From the looks of the original 82093AA spec, the 2-wire bus did
round-robin between different APICs (including IOAPICs), and the IOAPIC
had no ordering guarantees whatsoever.

	-hpa

Re: [patch] x86, apic: use 0x20 for the IRQ_MOVE_CLEANUP_VECTOR instead of 0x1f

[H. Peter Anvin]

On 01/13/2010 12:36 PM, Eric W. Biederman wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> On 01/11/2010 05:52 PM, Eric W. Biederman wrote:
>>>
>>> After having the documentation quoted at me.  I am having a distinct
>>> memory of one piece of documentation saying:
>>> "interrupts within a priority level can be delivered in any order"
>>>
>>> So I am guessing there is not any ordering of interrupts in the same
>>> priority level until they get to the local apic.
>>>
>>
>> There is no ordering of interrupts before they hit the local APIC, since
>> the local APIC is what would serialize them...
> 
> The io apic serializes them, and sends them over either the 2-wire
> bus or the front side bus.   How much serialization and prioritization
> happens at that point I am not certain, but some certainly happens
> before you get to the local apic.
> 

Specifically, from the 82093AA spec:

"The interrupt number or the vector does not imply a particular priority
for being sent.  The IOAPIC continually polls the 24 interrupts in a
rotating fashion, one at a time.  The pending interrupt polled first is
the one sent."

In other words, the prioritization is all done at the LAPIC.

	-hpa