[PATCH][SVM] fix spinlock panic

[PATCH][SVM] fix spinlock panic

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]


Hi!

Attached patch enables irq when initializing SVM so it's safe to take locks.
This fixes a panic like this:

(XEN) Xen BUG at spinlock.c:24
[...]
(XEN) Xen call trace:
(XEN)    [<ffff828c8011b839>] check_lock+0x3d/0x49
(XEN)    [<ffff828c8011b9a4>] _spin_lock+0x11/0x2d
(XEN)    [<ffff828c80111d4e>] alloc_heap_pages+0xcf/0x43d
(XEN)    [<ffff828c80113775>] alloc_domheap_pages+0x115/0x160
(XEN)    [<ffff828c80113810>] alloc_xenheap_pages+0x50/0xd7
(XEN)    [<ffff828c801a4163>] alloc_host_save_area+0x18/0x45
(XEN)    [<ffff828c801a21b0>] start_svm+0x6b/0x1f5
(XEN)    [<ffff828c80184b5d>] init_amd+0x92b/0x96e
(XEN)    [<ffff828c801852cf828c80163108>] smp_store_cpu_info+0x3b/0xbd
(XEN)    [<ffff828c8016328b>] smp_callin+0x101/0x217
(XEN)    [<ffff828c80164314>] start_secondary+0xb0/0x419


Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_svm_irq.diff --]
[-- Type: text/x-diff, Size: 499 bytes --]

diff -r 45ca3f3c3f98 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c	Tue Jun 16 11:53:47 2009 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c	Wed Jun 17 16:15:44 2009 +0200
@@ -850,6 +850,9 @@ int start_svm(struct cpuinfo_x86 *c)
     ecx = cpuid_ecx(0x80000001);
     boot_cpu_data.x86_capability[5] = ecx;
     
+    /* Now enable interrupts so it's safe to take locks. */
+    local_irq_enable();
+
     if ( !(test_bit(X86_FEATURE_SVME, &boot_cpu_data.x86_capability)) )
         return 0;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH][SVM] fix spinlock panic

[Keir Fraser]

On 17/06/2009 15:20, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> Attached patch enables irq when initializing SVM so it's safe to take locks.
> This fixes a panic like this:

The bug is related to the fact you leak memory on every S3 resume, by
reallocating hsa[cpu] and root_vmcb[cpu] which were never freed. Please see
c/s 19784 in the staging tree which does a general cleanup and fix of
start_svm(). It should fix your crash.

It does change behaviour a bit -- primarily: It does not reset the ASID
logic completely (I'm not certain there couldn't be VCPUs hanging around
pointing at earlier generations on this CPU, in which case it would be
dangerous to reset the ASID generation back to zero); Also, as well as not
reallocating hsa and root_vmcb, we don't re-init them either. I don't know
ehether that strictly matters but obviously we could easily re-jig the code
to always clear_page() those pages unconditionally.

 -- Keir

Re: [PATCH][SVM] fix spinlock panic

[Christoph Egger]

On Thursday 18 June 2009 10:51:17 Keir Fraser wrote:
> On 17/06/2009 15:20, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > Attached patch enables irq when initializing SVM so it's safe to take
> > locks. This fixes a panic like this:
>
> The bug is related to the fact you leak memory on every S3 resume, by
> reallocating hsa[cpu] and root_vmcb[cpu] which were never freed. Please see
> c/s 19784 in the staging tree which does a general cleanup and fix of
> start_svm(). It should fix your crash.
>
> It does change behaviour a bit -- primarily: It does not reset the ASID
> logic completely (I'm not certain there couldn't be VCPUs hanging around
> pointing at earlier generations on this CPU, in which case it would be
> dangerous to reset the ASID generation back to zero); Also, as well as not
> reallocating hsa and root_vmcb, we don't re-init them either. I don't know
> ehether that strictly matters but obviously we could easily re-jig the code
> to always clear_page() those pages unconditionally.

Thanks for it. Regarding ASID, I will ask back.
Now, I trigger an ASSERT() called by setup_vmcb_dump():

(XEN) Assertion 'key_table[key].u.handler == ((void*)0)' failed at 
keyhandler.c:68
[]
(XEN) Xen call trace:
(XEN)    [<ffff828c8010e0b6>] register_keyhandler+0x24/0x65
(XEN)    [<ffff828c801a7487>] setup_vmcb_dump+0x1c/0x1e
(XEN)    [<ffff828c801a5a37>] start_svm+0x98/0xdc
(XEN)    [<ffff828c8018747d>] init_amd+0x92b/0x96e
(XEN)    [<ffff828c80187be8>] identify_cpu+0xc3/0x23e
(XEN)    [<ffff828c8016576b>] smp_store_cpu_info+0x3b/0xca
(XEN)    [<ffff828c801658fb>] smp_callin+0x101/0x217
(XEN)    [<ffff828c80166997>] start_secondary+0xb0/0x419

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH][SVM] fix spinlock panic

[Keir Fraser]

On 18/06/2009 10:25, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> dangerous to reset the ASID generation back to zero); Also, as well as not
>> reallocating hsa and root_vmcb, we don't re-init them either. I don't know
>> ehether that strictly matters but obviously we could easily re-jig the code
>> to always clear_page() those pages unconditionally.
> 
> Thanks for it. Regarding ASID, I will ask back.
> Now, I trigger an ASSERT() called by setup_vmcb_dump():
> 
> (XEN) Assertion 'key_table[key].u.handler == ((void*)0)' failed at
> keyhandler.c:68

Oh good grief, I got the test at the top of start_svm() inverted. Please try
changeset 19798 which fixes this.

 -- Keir

Re: [PATCH][SVM] fix spinlock panic

[Christoph Egger]

On Thursday 18 June 2009 11:47:07 Keir Fraser wrote:
> On 18/06/2009 10:25, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> dangerous to reset the ASID generation back to zero); Also, as well as
> >> not reallocating hsa and root_vmcb, we don't re-init them either. I
> >> don't know ehether that strictly matters but obviously we could easily
> >> re-jig the code to always clear_page() those pages unconditionally.
> >
> > Thanks for it. Regarding ASID, I will ask back.
> > Now, I trigger an ASSERT() called by setup_vmcb_dump():
> >
> > (XEN) Assertion 'key_table[key].u.handler == ((void*)0)' failed at
> > keyhandler.c:68
>
> Oh good grief, I got the test at the top of start_svm() inverted. Please
> try changeset 19798 which fixes this.

Yes, works now. Tnx.

Can you apply c/s 19784 and c/s 19798 to xen-3.4-testing and xen-3.3-testing, 
please ?

Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632