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

[ebiederm@xmission.com (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