Zhang, Xiantao
2009-Sep-03 04:14 UTC
[Xen-devel] [PATCH] Don''t free irqaction for com irq when release irq.
# HG changeset patch # User root@localhost.localdomain # Date 1251916103 14400 # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 x86: com devices''s irqaction shouldn''t free. Since irqs of serial devices are initialized in early Xen and its irqaction is not allocated from heap, so doesn''t need free in release irq logic. Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com> diff -r 3b7cbf32fee9 -r 49e847aed58d xen/arch/x86/irq.c --- a/xen/arch/x86/irq.c Mon Aug 31 10:54:32 2009 +0100 +++ b/xen/arch/x86/irq.c Wed Sep 02 14:28:23 2009 -0400 @@ -564,7 +564,7 @@ void release_irq(unsigned int irq) /* Wait to make sure it''s not being used on another CPU */ do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); - if (action) + if ( !COM_IRQ(irq) && action ) xfree(action); } diff -r 3b7cbf32fee9 -r 49e847aed58d xen/arch/x86/setup.c --- a/xen/arch/x86/setup.c Mon Aug 31 10:54:32 2009 +0100 +++ b/xen/arch/x86/setup.c Wed Sep 02 14:28:23 2009 -0400 @@ -464,10 +464,10 @@ void __init __start_xen(unsigned long mb /* We initialise the serial devices very early so we can get debugging. */ ns16550.io_base = 0x3f8; - ns16550.irq = 4; + ns16550.irq = COM1_IRQ; ns16550_init(0, &ns16550); ns16550.io_base = 0x2f8; - ns16550.irq = 3; + ns16550.irq = COM2_IRQ; ns16550_init(1, &ns16550); console_init_preirq(); diff -r 3b7cbf32fee9 -r 49e847aed58d xen/include/asm-x86/irq.h --- a/xen/include/asm-x86/irq.h Mon Aug 31 10:54:32 2009 +0100 +++ b/xen/include/asm-x86/irq.h Wed Sep 02 14:28:23 2009 -0400 @@ -26,6 +26,11 @@ #define MAX_NR_IRQS (2 * MAX_GSI_IRQS) #define irq_cfg(irq) &irq_cfg[(irq)] + +#define COM1_IRQ 4 +#define COM2_IRQ 3 + +#define COM_IRQ(irq) ((irq) == COM1_IRQ || (irq) == COM2_IRQ) struct irq_cfg { int vector; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-03 06:14 UTC
[Xen-devel] Re: [PATCH] Don''t free irqaction for com irq when release irq.
On 03/09/2009 05:14, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote:> # HG changeset patch > # User root@localhost.localdomain > # Date 1251916103 14400 > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > x86: com devices''s irqaction shouldn''t free. > > Since irqs of serial devices are initialized in early Xen and > its irqaction is not allocated from heap, so doesn''t need free > in release irq logic. > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>Does this fix host S3? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Cui, Dexuan
2009-Sep-03 08:14 UTC
RE: [Xen-devel] Re: [PATCH] Don''t free irqaction for com irq when release irq.
Just now I found another issue about Dom0 S3 resume when VT-d is enabled. I''ll send a patch for it. Thanks, -- Dexuan -----Original Message----- From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser Sent: 2009?9?3? 14:14 To: Zhang, Xiantao; Xen Subject: [Xen-devel] Re: [PATCH] Don''t free irqaction for com irq when release irq. On 03/09/2009 05:14, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote:> # HG changeset patch > # User root@localhost.localdomain > # Date 1251916103 14400 > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > x86: com devices''s irqaction shouldn''t free. > > Since irqs of serial devices are initialized in early Xen and > its irqaction is not allocated from heap, so doesn''t need free > in release irq logic. > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>Does this fix host S3? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Xiantao
2009-Sep-03 09:38 UTC
[Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
No, host S3 issue is caused by ioapic suspend/resume in dom0. IMO, dom0 shouldn''t perform S3 for ioapic , right ? After fixing that issue, domain S3 works well. Xiantao -----Original Message----- From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] Sent: Thursday, September 03, 2009 2:14 PM To: Zhang, Xiantao; Xen Subject: Re: [PATCH] Don''t free irqaction for com irq when release irq. On 03/09/2009 05:14, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote:> # HG changeset patch > # User root@localhost.localdomain > # Date 1251916103 14400 > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > x86: com devices''s irqaction shouldn''t free. > > Since irqs of serial devices are initialized in early Xen and > its irqaction is not allocated from heap, so doesn''t need free > in release irq logic. > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>Does this fix host S3? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2009-Sep-03 10:04 UTC
Re: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
On Thursday 03 September 2009 11:38:35 Zhang, Xiantao wrote:> No, host S3 issue is caused by ioapic suspend/resume in dom0. IMO, dom0 > shouldn''t perform S3 for ioapic , right ?dom0 initializes and programs ioapic. In short: ioapic is under control of dom0. Please explain why dom0 shouldn''t do ioapic suspend/resume. Christoph> After fixing that issue, domain > S3 works well. Xiantao > > -----Original Message----- > From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] > Sent: Thursday, September 03, 2009 2:14 PM > To: Zhang, Xiantao; Xen > Subject: Re: [PATCH] Don''t free irqaction for com irq when release irq. > > On 03/09/2009 05:14, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote: > > # HG changeset patch > > # User root@localhost.localdomain > > # Date 1251916103 14400 > > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > > x86: com devices''s irqaction shouldn''t free. > > > > Since irqs of serial devices are initialized in early Xen and > > its irqaction is not allocated from heap, so doesn''t need free > > in release irq logic. > > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com> > > Does this fix host S3? > > -- Keir > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Sep-03 10:20 UTC
Re: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
>>> Christoph Egger <Christoph.Egger@amd.com> 03.09.09 12:04 >>> >On Thursday 03 September 2009 11:38:35 Zhang, Xiantao wrote: >> No, host S3 issue is caused by ioapic suspend/resume in dom0. IMO, dom0 >> shouldn''t perform S3 for ioapic , right ? > >dom0 initializes and programs ioapic. In short: ioapic is under control of >dom0. Please explain why dom0 shouldn''t do ioapic suspend/resume.Because Xen itself does so (and has to for its own interrupts to work). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Xiantao
2009-Sep-03 12:53 UTC
RE: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
I think Jan also answered your question. Dom0 shouldn''t touch ioapic after initialization time any more. That is to say, maybe we can find a way to get rid of ioapic from dom0. Actually I can''t see why dom0 cares so much about ioapic. Jeremy, do you know the reason ? IMO, dom0 should only cares about GSI and pirq mapping, but currently GSI is always equal to pirq in dom0, so no reason to keep ioapic in dom0. Xiantao -----Original Message----- From: Christoph Egger [mailto:Christoph.Egger@amd.com] Sent: Thursday, September 03, 2009 6:05 PM To: xen-devel@lists.xensource.com Cc: Zhang, Xiantao; Keir Fraser Subject: Re: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq. On Thursday 03 September 2009 11:38:35 Zhang, Xiantao wrote:> No, host S3 issue is caused by ioapic suspend/resume in dom0. IMO, dom0 > shouldn''t perform S3 for ioapic , right ?dom0 initializes and programs ioapic. In short: ioapic is under control of dom0. Please explain why dom0 shouldn''t do ioapic suspend/resume. Christoph> After fixing that issue, domain > S3 works well. Xiantao > > -----Original Message----- > From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] > Sent: Thursday, September 03, 2009 2:14 PM > To: Zhang, Xiantao; Xen > Subject: Re: [PATCH] Don''t free irqaction for com irq when release irq. > > On 03/09/2009 05:14, "Zhang, Xiantao" <xiantao.zhang@intel.com> wrote: > > # HG changeset patch > > # User root@localhost.localdomain > > # Date 1251916103 14400 > > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > > x86: com devices''s irqaction shouldn''t free. > > > > Since irqs of serial devices are initialized in early Xen and > > its irqaction is not allocated from heap, so doesn''t need free > > in release irq logic. > > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com> > > Does this fix host S3? > > -- Keir > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Sep-03 15:48 UTC
RE: [Xen-devel] [PATCH] Don''t free irqaction for com irq when release irq.
Unless I missed something, it looks like Keir''s modified version of this patch (20153) neglects to set the free_on_release=0 for the com irqs.> -----Original Message----- > From: Zhang, Xiantao [mailto:xiantao.zhang@intel.com] > Sent: Wednesday, September 02, 2009 10:15 PM > To: Xen; Keir Fraser > Subject: [Xen-devel] [PATCH] Don''t free irqaction for com irq when > release irq. > > > # HG changeset patch > # User root@localhost.localdomain > # Date 1251916103 14400 > # Node ID 49e847aed58dde35f8a0f909999d01d97be6f531 > # Parent 3b7cbf32fee909d860daacf70b8c3b97eaf036b5 > x86: com devices''s irqaction shouldn''t free. > > Since irqs of serial devices are initialized in early Xen and > its irqaction is not allocated from heap, so doesn''t need free > in release irq logic. > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com> > > diff -r 3b7cbf32fee9 -r 49e847aed58d xen/arch/x86/irq.c > --- a/xen/arch/x86/irq.c Mon Aug 31 10:54:32 2009 +0100 > +++ b/xen/arch/x86/irq.c Wed Sep 02 14:28:23 2009 -0400 > @@ -564,7 +564,7 @@ void release_irq(unsigned int irq) > /* Wait to make sure it''s not being used on another CPU */ > do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); > > - if (action) > + if ( !COM_IRQ(irq) && action ) > xfree(action); > } > > diff -r 3b7cbf32fee9 -r 49e847aed58d xen/arch/x86/setup.c > --- a/xen/arch/x86/setup.c Mon Aug 31 10:54:32 2009 +0100 > +++ b/xen/arch/x86/setup.c Wed Sep 02 14:28:23 2009 -0400 > @@ -464,10 +464,10 @@ void __init __start_xen(unsigned long mb > > /* We initialise the serial devices very early so we can > get debugging. */ > ns16550.io_base = 0x3f8; > - ns16550.irq = 4; > + ns16550.irq = COM1_IRQ; > ns16550_init(0, &ns16550); > ns16550.io_base = 0x2f8; > - ns16550.irq = 3; > + ns16550.irq = COM2_IRQ; > ns16550_init(1, &ns16550); > console_init_preirq(); > > diff -r 3b7cbf32fee9 -r 49e847aed58d xen/include/asm-x86/irq.h > --- a/xen/include/asm-x86/irq.h Mon Aug 31 10:54:32 2009 +0100 > +++ b/xen/include/asm-x86/irq.h Wed Sep 02 14:28:23 2009 -0400 > @@ -26,6 +26,11 @@ > #define MAX_NR_IRQS (2 * MAX_GSI_IRQS) > > #define irq_cfg(irq) &irq_cfg[(irq)] > + > +#define COM1_IRQ 4 > +#define COM2_IRQ 3 > + > +#define COM_IRQ(irq) ((irq) == COM1_IRQ || (irq) == COM2_IRQ) > > struct irq_cfg { > int vector;_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Sep-03 16:03 UTC
RE: [Xen-devel] [PATCH] Don''t free irqaction for com irq when release irq.
>>> Dan Magenheimer <dan.magenheimer@oracle.com> 03.09.09 17:48 >>> >Unless I missed something, it looks like Keir''s modified >version of this patch (20153) neglects to set the >free_on_release=0 for the com irqs.ns16550_com[] starts out as being all zero, hence there''s no need to explicitly clear that flag. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2009-Sep-03 16:26 UTC
RE: [Xen-devel] [PATCH] Don''t free irqaction for com irq when release irq.
Oops yes, thanks for pointing that out. This won''t be the first or last time I''ve been fooled by that convention. For the sake of good programming practices and (self-)documentation, since "don''t free" is a rare exception rather than the rule, may I suggest changing the name/sense of the variable in the struct to "dont_free", for which zero is a reasonable default, and just enabling it for the com irqs?> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@novell.com] > Sent: Thursday, September 03, 2009 10:03 AM > To: Dan Magenheimer > Cc: Keir Fraser; Xiantao Zhang; Xen > Subject: RE: [Xen-devel] [PATCH] Don''t free irqaction for com irq when > release irq. > > > >>> Dan Magenheimer <dan.magenheimer@oracle.com> 03.09.09 17:48 >>> > >Unless I missed something, it looks like Keir''s modified > >version of this patch (20153) neglects to set the > >free_on_release=0 for the com irqs. > > ns16550_com[] starts out as being all zero, hence there''s no need to > explicitly clear that flag. > > Jan > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Sep-03 16:40 UTC
Re: [Xen-devel] [PATCH] Don''t free irqaction for com irq when release irq.
On 03/09/2009 17:26, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:> Oops yes, thanks for pointing that out. This won''t > be the first or last time I''ve been fooled by that > convention. > > For the sake of good programming practices and > (self-)documentation, since "don''t free" is > a rare exception rather than the rule, may > I suggest changing the name/sense of the variable > in the struct to "dont_free", for which zero is > a reasonable default, and just enabling > it for the com irqs?All irqactions which *should* be freed are allocated in exactly one place. It is therefore sensible to make that the ''exception'' which sets a flag to 1. The variou sother users of the setup_irq() interface then do not need to be changed, as they get a default flag value of zero. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-Sep-03 20:58 UTC
Re: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
On 09/03/09 05:53, Zhang, Xiantao wrote:> I think Jan also answered your question. Dom0 shouldn''t touch ioapic after initialization time any more. That is to say, maybe we can find a way to get rid of ioapic from dom0. Actually I can''t see why dom0 cares so much about ioapic. Jeremy, do you know the reason ? IMO, dom0 should only cares about GSI and pirq mapping, but currently GSI is always equal to pirq in dom0, so no reason to keep ioapic in dom0. >Yes, I agree. I have a prototype branch: git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git #rebase/dom0/new-interrupt-routing which replaces the whole interrupt handing subsystem to avoid any direct interactions with the IO APICs. I add a new hypercall to directly bind a gsi to a given pirq (which has some similarities to your patch). I''ve attached it below. (new-interrupt-routing shouldn''t need this patch to function, however.) Unfortunately I haven''t had a chance to work on this lately, but when I last tried it, it hung shortly after initializing ACPI. I didn''t get much further than that. I do think, however, that this is this right way to go for dom0, esp with regard to upstreaming. (The second patch is to allow dom0 to get Xen''s acpi interrupt model so they can always be consistent rather than independently arriving at the same result - not not.) J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhang, Xiantao
2009-Sep-04 01:54 UTC
RE: [Xen-devel] RE: [PATCH] Don''t free irqaction for com irq when release irq.
Jeremy Fitzhardinge wrote:> On 09/03/09 05:53, Zhang, Xiantao wrote: >> I think Jan also answered your question. Dom0 shouldn''t touch >> ioapic after initialization time any more. That is to say, maybe we >> can find a way to get rid of ioapic from dom0. Actually I can''t see >> why dom0 cares so much about ioapic. Jeremy, do you know the >> reason ? IMO, dom0 should only cares about GSI and pirq mapping, >> but currently GSI is always equal to pirq in dom0, so no reason to >> keep ioapic in dom0. >> > > Yes, I agree. I have a prototype branch: > git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git > #rebase/dom0/new-interrupt-routing which replaces the whole interrupt > handing subsystem to avoid any direct interactions with the IO APICs.Okay, I will read your code and give my comments later.> I add a new hypercall to directly bind a gsi to a given pirq (which > has some similarities to your patch). I''ve attached it below. > (new-interrupt-routing shouldn''t need this patch to function, > however.) > > Unfortunately I haven''t had a chance to work on this lately, but when > I last tried it, it hung shortly after initializing ACPI. I didn''t > get much further than that. I do think, however, that this is this > right way to go for dom0, esp with regard to upstreaming.Seems you didn''t assign vectors for GSI >= 16 ? Currenlty, hypervisor only allocate vectors for pin0 -> pin16, so irq_to_vector may not get the vectors for the required GSI.> (The second patch is to allow dom0 to get Xen''s acpi interrupt model > so they can always be consistent rather than independently arriving > at the same result - not not.)Maybe it is not a must to introduce new hypercall and just using PHYSDEVOP_map_pirq like MSI side to bind pirq and GSI should work for GSI irqs also. For the second patch, maybe we can ignore it since machines produced in recent 10 years should always use the model XEN_ACPI_IRQ_MODEL_IOAPIC. For old machines, I don''t think Xen can run well on them. For the model XEN_ACPI_IRQ_MODEL_IOSAPIC, it is only available for Intel Itanium processors, so... :-) Xiantao _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel