Han, Weidong
2009-Jun-03 09:28 UTC
[xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Originally, it calls xmalloc to set index in ioapic_rte_to_remap_entry(). When make with debug=y, it may trigger spinlock BUG_ON because allocate memory with interrupt disabled. This patch doesn''t allocate list_head entry in ioapic_rte_to_remap_entry(), instead allocate the array in enable_intremap() to avoid allocating memory with interrupt disabled. Signed-off-by: Weidong Han <weidong.han@intel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Jun-03 12:02 UTC
Re: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Wasteful of memory, so I checked in a modified version as c/s 19707, which dynamically sizes the array. Please take a look and check it''s okay. It probably breaks ia64 build due to undefined nr_ioapics and nr_ioapic_registers[], but I think yours broke ia64 too so we''re even. :-) Isaku: can you suggest ia64 equivalents for nr_ioapics and nr_ioapic_registers[]? We can do some ifdef magic at the top of intremap.c, including defining a nr_ioapic_registers() macro, if that helps. Thanks, Keir On 03/06/2009 10:28, "Han, Weidong" <weidong.han@intel.com> wrote:> Originally, it calls xmalloc to set index in ioapic_rte_to_remap_entry(). When > make with debug=y, it may trigger spinlock BUG_ON because allocate memory with > interrupt disabled. > > This patch doesn''t allocate list_head entry in ioapic_rte_to_remap_entry(), > instead allocate the array in enable_intremap() to avoid allocating memory > with interrupt disabled. > > > Signed-off-by: Weidong Han <weidong.han@intel.com>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Han, Weidong
2009-Jun-04 01:16 UTC
RE: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Keir, Yes, your modified patch saves memory. I wanted to do like it. But Dexuan is working on a x2apic patch, which will move interrupt remapping enabling before IOAPIC setup. So I''m wondering ioapic stuffs (nr_ioapic_registers[], nr_ioapics, etc.) aren''t ready when enable interrupt remapping after that moving. Dexuan, can you have a look at it? Regards, Weidong Keir Fraser wrote:> Wasteful of memory, so I checked in a modified version as c/s 19707, > which dynamically sizes the array. Please take a look and check it''s > okay. > > It probably breaks ia64 build due to undefined nr_ioapics and > nr_ioapic_registers[], but I think yours broke ia64 too so we''re > even. :-) > > Isaku: can you suggest ia64 equivalents for nr_ioapics and > nr_ioapic_registers[]? We can do some ifdef magic at the top of > intremap.c, including defining a nr_ioapic_registers() macro, if that > helps. > > Thanks, > Keir > > On 03/06/2009 10:28, "Han, Weidong" <weidong.han@intel.com> wrote: > >> Originally, it calls xmalloc to set index in >> ioapic_rte_to_remap_entry(). When make with debug=y, it may trigger >> spinlock BUG_ON because allocate memory with interrupt disabled. >> >> This patch doesn''t allocate list_head entry in >> ioapic_rte_to_remap_entry(), instead allocate the array in >> enable_intremap() to avoid allocating memory with interrupt >> disabled. >> >> >> Signed-off-by: Weidong Han <weidong.han@intel.com>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2009-Jun-04 03:59 UTC
Re: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
On Wed, Jun 03, 2009 at 01:02:59PM +0100, Keir Fraser wrote:> Wasteful of memory, so I checked in a modified version as c/s 19707, which > dynamically sizes the array. Please take a look and check it''s okay. > > It probably breaks ia64 build due to undefined nr_ioapics and > nr_ioapic_registers[], but I think yours broke ia64 too so we''re even. :-) > > Isaku: can you suggest ia64 equivalents for nr_ioapics and > nr_ioapic_registers[]? We can do some ifdef magic at the top of intremap.c, > including defining a nr_ioapic_registers() macro, if that helps.Here is the patch. I''m going to test it, though. [] operator is an obstacle to use CPP magic. So I did just quick hack to send this patch as soon as possible. You may want to wrap it with a function. thanks, vtd: ia64 fix of intremap.c 19707:07cf79dfb59c caused compilation error on ia64. This patch fixes it. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> diff --git a/xen/arch/ia64/linux-xen/iosapic.c b/xen/arch/ia64/linux-xen/iosapic.c --- a/xen/arch/ia64/linux-xen/iosapic.c +++ b/xen/arch/ia64/linux-xen/iosapic.c @@ -1275,4 +1275,22 @@ int iosapic_guest_write(unsigned long ph spin_unlock_irqrestore(&irq_descp(vec)->lock, flags); return 0; } + +/* for vtd interrupt remapping. xen/drivers/vtd/intremap.c */ +int iosapic_get_nr_iosapics(void) +{ + int index; + + for (index = NR_IOSAPICS - 1; index >= 0; index--) { + if (iosapic_lists[index].addr) + break; + } + + return index + 1; +} + +int iosapic_get_nr_pins(int index) +{ + return iosapic_lists[index].num_rte; +} #endif /* XEN */ diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -33,6 +33,10 @@ #ifdef __ia64__ #define dest_SMI -1 +#define nr_ioapics iosapic_get_nr_iosapics() +#define nr_ioapic_registers(i) iosapic_get_nr_pins(i) +#else +#define nr_ioapic_registers(i) nr_ioapic_registers[i] #endif /* apic_pin_2_ir_idx[apicid][pin] = interrupt remapping table index */ @@ -45,7 +49,7 @@ static int init_apic_pin_2_ir_idx(void) nr_pins = 0; for ( i = 0; i < nr_ioapics; i++ ) - nr_pins += nr_ioapic_registers[i]; + nr_pins += nr_ioapic_registers(i); _apic_pin_2_ir_idx = xmalloc_array(unsigned int, nr_pins); apic_pin_2_ir_idx = xmalloc_array(unsigned int *, nr_ioapics); @@ -63,7 +67,7 @@ static int init_apic_pin_2_ir_idx(void) for ( i = 0; i < nr_ioapics; i++ ) { apic_pin_2_ir_idx[i] = &_apic_pin_2_ir_idx[nr_pins]; - nr_pins += nr_ioapic_registers[i]; + nr_pins += nr_ioapic_registers(i); } return 0; diff --git a/xen/include/asm-ia64/linux-xen/asm/iosapic.h b/xen/include/asm-ia64/linux-xen/asm/iosapic.h --- a/xen/include/asm-ia64/linux-xen/asm/iosapic.h +++ b/xen/include/asm-ia64/linux-xen/asm/iosapic.h @@ -186,6 +186,9 @@ struct rte_entry { #define IOSAPIC_RTEINDEX(reg) (((reg) - 0x10) >> 1) extern unsigned long ia64_vector_mask[]; extern unsigned long ia64_xen_vector[]; + +int iosapic_get_nr_iosapics(void); +int iosapic_get_nr_pins(int index); #endif /* XEN */ #define IO_APIC_BASE(idx) ((unsigned int *)iosapic_lists[idx].addr) -- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Cui, Dexuan
2009-Jun-04 04:18 UTC
RE: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Yes, I''m organizing the order. I can further change the code. :-) Thanks, -- Dexuan -----Original Message----- From: Han, Weidong Sent: 2009?6?4? 9:16 To: ''Keir Fraser'' Cc: ''xen-devel''; ''Jan Beulich''; ''Isaku Yamahata''; Cui, Dexuan Subject: RE: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index Keir, Yes, your modified patch saves memory. I wanted to do like it. But Dexuan is working on a x2apic patch, which will move interrupt remapping enabling before IOAPIC setup. So I''m wondering ioapic stuffs (nr_ioapic_registers[], nr_ioapics, etc.) aren''t ready when enable interrupt remapping after that moving. Dexuan, can you have a look at it? Regards, Weidong Keir Fraser wrote:> Wasteful of memory, so I checked in a modified version as c/s 19707, > which dynamically sizes the array. Please take a look and check it''s > okay. > > It probably breaks ia64 build due to undefined nr_ioapics and > nr_ioapic_registers[], but I think yours broke ia64 too so we''re > even. :-) > > Isaku: can you suggest ia64 equivalents for nr_ioapics and > nr_ioapic_registers[]? We can do some ifdef magic at the top of > intremap.c, including defining a nr_ioapic_registers() macro, if that > helps. > > Thanks, > Keir > > On 03/06/2009 10:28, "Han, Weidong" <weidong.han@intel.com> wrote: > >> Originally, it calls xmalloc to set index in >> ioapic_rte_to_remap_entry(). When make with debug=y, it may trigger >> spinlock BUG_ON because allocate memory with interrupt disabled. >> >> This patch doesn''t allocate list_head entry in >> ioapic_rte_to_remap_entry(), instead allocate the array in >> enable_intremap() to avoid allocating memory with interrupt >> disabled. >> >> >> Signed-off-by: Weidong Han <weidong.han@intel.com>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Han, Weidong
2009-Jun-04 05:39 UTC
RE: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Look fine for me. Thanks. Regards, Weidong Isaku Yamahata wrote:> On Wed, Jun 03, 2009 at 01:02:59PM +0100, Keir Fraser wrote: >> Wasteful of memory, so I checked in a modified version as c/s 19707, >> which dynamically sizes the array. Please take a look and check it''s >> okay. >> >> It probably breaks ia64 build due to undefined nr_ioapics and >> nr_ioapic_registers[], but I think yours broke ia64 too so we''re >> even. :-) >> >> Isaku: can you suggest ia64 equivalents for nr_ioapics and >> nr_ioapic_registers[]? We can do some ifdef magic at the top of >> intremap.c, including defining a nr_ioapic_registers() macro, if >> that helps. > > Here is the patch. I''m going to test it, though. > > [] operator is an obstacle to use CPP magic. > So I did just quick hack to send this patch as soon as possible. > You may want to wrap it with a function. > > thanks, > > vtd: ia64 fix of intremap.c > > 19707:07cf79dfb59c caused compilation error on ia64. > This patch fixes it. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > diff --git a/xen/arch/ia64/linux-xen/iosapic.c > b/xen/arch/ia64/linux-xen/iosapic.c --- > a/xen/arch/ia64/linux-xen/iosapic.c +++ > b/xen/arch/ia64/linux-xen/iosapic.c @@ -1275,4 +1275,22 @@ int > iosapic_guest_write(unsigned long ph > spin_unlock_irqrestore(&irq_descp(vec)->lock, flags); return 0; > } > + > +/* for vtd interrupt remapping. xen/drivers/vtd/intremap.c */ > +int iosapic_get_nr_iosapics(void) > +{ > + int index; > + > + for (index = NR_IOSAPICS - 1; index >= 0; index--) { > + if (iosapic_lists[index].addr) > + break; > + } > + > + return index + 1; > +} > + > +int iosapic_get_nr_pins(int index) > +{ > + return iosapic_lists[index].num_rte; > +} > #endif /* XEN */ > diff --git a/xen/drivers/passthrough/vtd/intremap.c > b/xen/drivers/passthrough/vtd/intremap.c --- > a/xen/drivers/passthrough/vtd/intremap.c +++ > b/xen/drivers/passthrough/vtd/intremap.c @@ -33,6 +33,10 @@ > > #ifdef __ia64__ > #define dest_SMI -1 > +#define nr_ioapics iosapic_get_nr_iosapics() > +#define nr_ioapic_registers(i) iosapic_get_nr_pins(i) > +#else > +#define nr_ioapic_registers(i) nr_ioapic_registers[i] > #endif > > /* apic_pin_2_ir_idx[apicid][pin] = interrupt remapping table index > */ @@ -45,7 +49,7 @@ static int init_apic_pin_2_ir_idx(void) > > nr_pins = 0; > for ( i = 0; i < nr_ioapics; i++ ) > - nr_pins += nr_ioapic_registers[i]; > + nr_pins += nr_ioapic_registers(i); > > _apic_pin_2_ir_idx = xmalloc_array(unsigned int, nr_pins); > apic_pin_2_ir_idx = xmalloc_array(unsigned int *, nr_ioapics); > @@ -63,7 +67,7 @@ static int init_apic_pin_2_ir_idx(void) > for ( i = 0; i < nr_ioapics; i++ ) > { > apic_pin_2_ir_idx[i] = &_apic_pin_2_ir_idx[nr_pins]; > - nr_pins += nr_ioapic_registers[i]; > + nr_pins += nr_ioapic_registers(i); > } > > return 0; > diff --git a/xen/include/asm-ia64/linux-xen/asm/iosapic.h > b/xen/include/asm-ia64/linux-xen/asm/iosapic.h --- > a/xen/include/asm-ia64/linux-xen/asm/iosapic.h +++ > b/xen/include/asm-ia64/linux-xen/asm/iosapic.h @@ -186,6 +186,9 @@ > struct rte_entry { #define IOSAPIC_RTEINDEX(reg) (((reg) - 0x10) >> > 1) extern unsigned long ia64_vector_mask[]; > extern unsigned long ia64_xen_vector[]; > + > +int iosapic_get_nr_iosapics(void); > +int iosapic_get_nr_pins(int index); > #endif /* XEN */ > > #define IO_APIC_BASE(idx) ((unsigned int *)iosapic_lists[idx].addr)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Jun-04 07:36 UTC
Re: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Ok, we do already support x2apic though, so I don''t know what extra Dexuan''s patch will do? Is x2apic+intremap currently broken? -- Keir On 04/06/2009 02:16, "Han, Weidong" <weidong.han@intel.com> wrote:> Keir, > > Yes, your modified patch saves memory. I wanted to do like it. But Dexuan is > working on a x2apic patch, which will move interrupt remapping enabling before > IOAPIC setup. So I''m wondering ioapic stuffs (nr_ioapic_registers[], > nr_ioapics, etc.) aren''t ready when enable interrupt remapping after that > moving. Dexuan, can you have a look at it? > > Regards, > Weidong > > Keir Fraser wrote: >> Wasteful of memory, so I checked in a modified version as c/s 19707, >> which dynamically sizes the array. Please take a look and check it''s >> okay. >> >> It probably breaks ia64 build due to undefined nr_ioapics and >> nr_ioapic_registers[], but I think yours broke ia64 too so we''re >> even. :-) >> >> Isaku: can you suggest ia64 equivalents for nr_ioapics and >> nr_ioapic_registers[]? We can do some ifdef magic at the top of >> intremap.c, including defining a nr_ioapic_registers() macro, if that >> helps. >> >> Thanks, >> Keir >> >> On 03/06/2009 10:28, "Han, Weidong" <weidong.han@intel.com> wrote: >> >>> Originally, it calls xmalloc to set index in >>> ioapic_rte_to_remap_entry(). When make with debug=y, it may trigger >>> spinlock BUG_ON because allocate memory with interrupt disabled. >>> >>> This patch doesn''t allocate list_head entry in >>> ioapic_rte_to_remap_entry(), instead allocate the array in >>> enable_intremap() to avoid allocating memory with interrupt >>> disabled. >>> >>> >>> Signed-off-by: Weidong Han <weidong.han@intel.com> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Han, Weidong
2009-Jun-04 07:47 UTC
RE: [xen-devel][PATCH][VTD] Fix apic pin to interrupt remapping table index
Keir Fraser wrote:> Ok, we do already support x2apic though, so I don''t know what extra > Dexuan''s patch will do? Is x2apic+intremap currently broken? >No break now. I think current x2apic implementation is incomplete. Per spec, interrupt remapping is pre-requirement for x2apic. So should enable interrupt remapping before x2apic. I think your modified patch is fine for now. We can change it when it''s really necessary. Regards, Weidong> -- Keir > > On 04/06/2009 02:16, "Han, Weidong" <weidong.han@intel.com> wrote: > >> Keir, >> >> Yes, your modified patch saves memory. I wanted to do like it. But >> Dexuan is working on a x2apic patch, which will move interrupt >> remapping enabling before IOAPIC setup. So I''m wondering ioapic >> stuffs (nr_ioapic_registers[], nr_ioapics, etc.) aren''t ready when >> enable interrupt remapping after that moving. Dexuan, can you have a >> look at it? >> >> Regards, >> Weidong >> >> Keir Fraser wrote: >>> Wasteful of memory, so I checked in a modified version as c/s 19707, >>> which dynamically sizes the array. Please take a look and check >>> it''s okay. >>> >>> It probably breaks ia64 build due to undefined nr_ioapics and >>> nr_ioapic_registers[], but I think yours broke ia64 too so we''re >>> even. :-) >>> >>> Isaku: can you suggest ia64 equivalents for nr_ioapics and >>> nr_ioapic_registers[]? We can do some ifdef magic at the top of >>> intremap.c, including defining a nr_ioapic_registers() macro, if >>> that helps. >>> >>> Thanks, >>> Keir >>> >>> On 03/06/2009 10:28, "Han, Weidong" <weidong.han@intel.com> wrote: >>> >>>> Originally, it calls xmalloc to set index in >>>> ioapic_rte_to_remap_entry(). When make with debug=y, it may trigger >>>> spinlock BUG_ON because allocate memory with interrupt disabled. >>>> >>>> This patch doesn''t allocate list_head entry in >>>> ioapic_rte_to_remap_entry(), instead allocate the array in >>>> enable_intremap() to avoid allocating memory with interrupt >>>> disabled. >>>> >>>> >>>> Signed-off-by: Weidong Han <weidong.han@intel.com>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel