RE: Alt SeaBIOS SSDT cpu hotplug

2010-08-05 Thread Liu, Jinsong 
Kevin O'Connor wrote:
 On Tue, Aug 03, 2010 at 05:00:49PM +0800, Liu, Jinsong wrote:
 I just test your new patch with Windows 2008 DataCenter at my
 platform, it works OK! We can hot-add new cpus and they appear at
 Device Manager.  (BTW, yesterday I test your new patch with linux
 2.6.32 hvm, it works fine, we can add-remove-add-remove... cpus)
 Sorry for make you spend more time. It's our fault.
 
 Thanks.
 
 I'll go ahead and commit it then.  I have one incremental patch (see
 below) which I will also commit.
 
 -Kevin
 
 
 --- ssdt-proc.dsl   2010-08-03 18:45:12.0 -0400
 +++ src/ssdt-proc.dsl   2010-08-03 18:45:17.0 -0400
 @@ -44,7 +44,7 @@
  Return(CPST(ID))
  }
  Method (_EJ0, 1, NotSerialized) {
 -Return(CPEJ(ID, Arg0))
 +CPEJ(ID, Arg0)
  }
  }
  }

Kevin,

We find the root cause why your patch works fine at some platform but fail at 
other platform:
1). At platform A, success, use old qemu-kvm, commit is 
3925c857a5740f62b30a5334f02477a23b61cc33
2). At platform B, fail, use new qemu-kvm, commit is 
59d71ddb432db04b57ee2658ce50a3e35d7db97e
so seabios is OK, but qemu-kvm has bug.
I have fix it and send to kvm maillist.

With the fix qemu-kvm patch, and latest seabios and latest qemu-kvm, we re-do 
test,
now vcpu hotplug works fine with both linux2.6.32 and win2k8 datacenter.

Thanks,
Jinsong--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-08-03 Thread Liu, Jinsong 
Zheng, Shaohui wrote:
 In our experiences, windows 2008 datacenter is the only version to
 support CPU hotplug, and we did not find any official announce for
 other windows, so we tested windows 2008 data center only.  
 
 Thanks for Kevin pointing out it, we will try windows7 hotplug
 feature. 
 
 Thanks  Regards,
 Shaohui
 
 
 -Original Message-
 From: Kevin O'Connor [mailto:ke...@koconnor.net]
 Sent: Tuesday, August 03, 2010 1:27 AM
 To: Avi Kivity
 Cc: Alexander Graf; Liu, Jinsong; seab...@seabios.org;
 kvm@vger.kernel.org; Jiang, Yunhong; Li, Xin; Zheng, Shaohui; Zhang,
 Jianwu; You, Yongkang  
 Subject: Re: Alt SeaBIOS SSDT cpu hotplug
 
 On Mon, Aug 02, 2010 at 07:13:34PM +0300, Avi Kivity wrote:
  On 08/02/2010 06:55 PM, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:
 On 02.08.2010, at 07:49, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 It seems the Windows acpi interpreter is significantly different
 from the Linux one.  The only guess I have is that Windows
 doesn't like one of the ASL constructs even though they all look
 valid.  I'd try to debug this by commenting out parts of the ASL
 until I narrowed down the parts causing the problem. 
 Unfortunately, I don't have Windows 2008 to do this directly. 
 
 Any other ideas?
 Just grab yourself a free copy of the Hyper-V server 2008:
 
 http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars
 I downloaded and installed it, but I can't reproduce the crash.  It
 seems like a really stripped down version of Windows, so I can't
 tell if it actually worked or not either.
 
 I thought only the Datacenter edition supported cpu hotplug.
 
 I just tried an old Win 7 Ultimate beta (build 7100) I had on my HD.
 It looks like it supports cpu hotplug.  However, I don't see any
 failures - it seems to work fine.  (After running cpu_set 1 online,
 the event pops up in the system event log as a UserPnP event, and the
 CPU appears in the system devices list.)
 
 -Kevin

Kevin,

I just test your new patch with Windows 2008 DataCenter at my platform, it 
works OK! We can hot-add new cpus and they appear at Device Manager.
(BTW, yesterday I test your new patch with linux 2.6.32 hvm, it works fine, we 
can add-remove-add-remove... cpus)
Sorry for make you spend more time. It's our fault.

Thanks,
Jinsong

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-03 Thread Kevin O'Connor 
On Tue, Aug 03, 2010 at 05:00:49PM +0800, Liu, Jinsong wrote:
 I just test your new patch with Windows 2008 DataCenter at my
 platform, it works OK! We can hot-add new cpus and they appear at
 Device Manager.  (BTW, yesterday I test your new patch with linux
 2.6.32 hvm, it works fine, we can add-remove-add-remove... cpus)
 Sorry for make you spend more time. It's our fault.

Thanks.

I'll go ahead and commit it then.  I have one incremental patch (see
below) which I will also commit.

-Kevin


--- ssdt-proc.dsl   2010-08-03 18:45:12.0 -0400
+++ src/ssdt-proc.dsl   2010-08-03 18:45:17.0 -0400
@@ -44,7 +44,7 @@
 Return(CPST(ID))
 }
 Method (_EJ0, 1, NotSerialized) {
-Return(CPEJ(ID, Arg0))
+CPEJ(ID, Arg0)
 }
 }
 }
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Alexander Graf 

On 02.08.2010, at 07:49, Kevin O'Connor wrote:

 On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 Kevin,
 
 This patch still has issue, It can boot Windows 2008 DataCenter,
 however, when run cpu_set cpu online command, windows 2008
 Datacenter system shutdown at once.
 
 Thanks for testing.
 
 I've inspected the generated DSDT and SSDT files, and I don't see
 anything wrong with them.  (To inspect a generated SSDT, uncomment the
 call to hexdump(), cut and paste the hexdump output into SeaBIOS'
 tools/transdump.py, and then call iasl -d on the output.)
 
 It seems the Windows acpi interpreter is significantly different from
 the Linux one.  The only guess I have is that Windows doesn't like one
 of the ASL constructs even though they all look valid.  I'd try to
 debug this by commenting out parts of the ASL until I narrowed down
 the parts causing the problem.  Unfortunately, I don't have Windows
 2008 to do this directly.
 
 Any other ideas?

Just grab yourself a free copy of the Hyper-V server 2008:

http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars


Alex

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Kevin O'Connor 
On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:
 On 02.08.2010, at 07:49, Kevin O'Connor wrote:
  On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
  It seems the Windows acpi interpreter is significantly different from
  the Linux one.  The only guess I have is that Windows doesn't like one
  of the ASL constructs even though they all look valid.  I'd try to
  debug this by commenting out parts of the ASL until I narrowed down
  the parts causing the problem.  Unfortunately, I don't have Windows
  2008 to do this directly.
  
  Any other ideas?
 
 Just grab yourself a free copy of the Hyper-V server 2008:
 
 http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars

I downloaded and installed it, but I can't reproduce the crash.  It
seems like a really stripped down version of Windows, so I can't tell
if it actually worked or not either.

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Alexander Graf 

On 02.08.2010, at 17:55, Kevin O'Connor wrote:

 On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:
 On 02.08.2010, at 07:49, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 It seems the Windows acpi interpreter is significantly different from
 the Linux one.  The only guess I have is that Windows doesn't like one
 of the ASL constructs even though they all look valid.  I'd try to
 debug this by commenting out parts of the ASL until I narrowed down
 the parts causing the problem.  Unfortunately, I don't have Windows
 2008 to do this directly.
 
 Any other ideas?
 
 Just grab yourself a free copy of the Hyper-V server 2008:
 
 http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars
 
 I downloaded and installed it, but I can't reproduce the crash.  It
 seems like a really stripped down version of Windows, so I can't tell
 if it actually worked or not either.

I haven't tried for a while now, but last time I used that stripped down 
version I could log in as administrator. Maybe it was through RDP only. But 
from there it was almost a fully functional copy of Windows that certainly 
sufficed to do simple functionality tests.

Alex

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Alexander Graf 

On 02.08.2010, at 18:13, Avi Kivity wrote:

 On 08/02/2010 06:55 PM, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:
 On 02.08.2010, at 07:49, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 It seems the Windows acpi interpreter is significantly different from
 the Linux one.  The only guess I have is that Windows doesn't like one
 of the ASL constructs even though they all look valid.  I'd try to
 debug this by commenting out parts of the ASL until I narrowed down
 the parts causing the problem.  Unfortunately, I don't have Windows
 2008 to do this directly.
 
 Any other ideas?
 Just grab yourself a free copy of the Hyper-V server 2008:
 
 http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars
 I downloaded and installed it, but I can't reproduce the crash.  It
 seems like a really stripped down version of Windows, so I can't tell
 if it actually worked or not either.
 
 I thought only the Datacenter edition supported cpu hotplug.

That could be the reason for it not showing the breakage. Sorry for the fuss :(.

Alex

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Avi Kivity 

 On 08/02/2010 06:55 PM, Kevin O'Connor wrote:

On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:

On 02.08.2010, at 07:49, Kevin O'Connor wrote:

On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
It seems the Windows acpi interpreter is significantly different from
the Linux one.  The only guess I have is that Windows doesn't like one
of the ASL constructs even though they all look valid.  I'd try to
debug this by commenting out parts of the ASL until I narrowed down
the parts causing the problem.  Unfortunately, I don't have Windows
2008 to do this directly.

Any other ideas?

Just grab yourself a free copy of the Hyper-V server 2008:

http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars

I downloaded and installed it, but I can't reproduce the crash.  It
seems like a really stripped down version of Windows, so I can't tell
if it actually worked or not either.


I thought only the Datacenter edition supported cpu hotplug.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-08-02 Thread Zheng, Shaohui 
In our experiences, windows 2008 datacenter is the only version to support CPU 
hotplug, and we did not find any official announce for other windows, so we 
tested windows 2008 data center only.

Thanks for Kevin pointing out it, we will try windows7 hotplug feature.

Thanks  Regards,
Shaohui


-Original Message-
From: Kevin O'Connor [mailto:ke...@koconnor.net] 
Sent: Tuesday, August 03, 2010 1:27 AM
To: Avi Kivity
Cc: Alexander Graf; Liu, Jinsong; seab...@seabios.org; kvm@vger.kernel.org; 
Jiang, Yunhong; Li, Xin; Zheng, Shaohui; Zhang, Jianwu; You, Yongkang
Subject: Re: Alt SeaBIOS SSDT cpu hotplug

On Mon, Aug 02, 2010 at 07:13:34PM +0300, Avi Kivity wrote:
  On 08/02/2010 06:55 PM, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:12:31AM +0200, Alexander Graf wrote:
 On 02.08.2010, at 07:49, Kevin O'Connor wrote:
 On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 It seems the Windows acpi interpreter is significantly different from
 the Linux one.  The only guess I have is that Windows doesn't like one
 of the ASL constructs even though they all look valid.  I'd try to
 debug this by commenting out parts of the ASL until I narrowed down
 the parts causing the problem.  Unfortunately, I don't have Windows
 2008 to do this directly.
 
 Any other ideas?
 Just grab yourself a free copy of the Hyper-V server 2008:
 
 http://arstechnica.com/microsoft/news/2009/08/microsoft-hyper-v-server-2008-r2-arrives-for-free.ars
 I downloaded and installed it, but I can't reproduce the crash.  It
 seems like a really stripped down version of Windows, so I can't tell
 if it actually worked or not either.
 
 I thought only the Datacenter edition supported cpu hotplug.

I just tried an old Win 7 Ultimate beta (build 7100) I had on my HD.
It looks like it supports cpu hotplug.  However, I don't see any
failures - it seems to work fine.  (After running cpu_set 1 online,
the event pops up in the system event log as a UserPnP event, and the
CPU appears in the system devices list.)

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-08-01 Thread Liu, Jinsong 
Kevin,

This patch still has issue, 
It can boot Windows 2008 DataCenter, however,
when run cpu_set cpu online command, windows 2008 Datacenter system shutdown 
at once.

Thanks,
Jinsong

 
 Sorry about that.  It looks like I messed up the SSDT ScopeOp length.
 New patch attached below.  I've tested it by adding/removing cpus on
 Linux, and I've now also boot tested winxp and winvista.  (I don't
 have Windows 2008 DataCenter.)
 
 -Kevin
 
 
 diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
 index cc31112..640716c 100644
 --- a/src/acpi-dsdt.dsl
 +++ b/src/acpi-dsdt.dsl
 @@ -648,6 +648,78 @@ DefinitionBlock (
  Zero   /* reserved */
  })
 
 +/* CPU hotplug */
 +Scope(\_SB) {
 +/* Objects filled in by run-time generated SSDT */
 +External(NTFY, MethodObj)
 +External(CPON, PkgObj)
 +
 +/* Methods called by run-time generated SSDT Processor
 objects */ +Method (CPMA, 1, NotSerialized) {
 +// _MAT method - create an madt apic buffer
 +// Local0 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Arg0)), Local0)
 +// Local1 = Buffer (in madt apic form) to return
 +Store(Buffer(8) {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0},
 Local1) +// Update the processor id, lapic id, and
 enable/disable status +Store(Arg0, Index(Local1, 2))
 +Store(Arg0, Index(Local1, 3))
 +Store(Local0, Index(Local1, 4))
 +Return (Local1)
 +}
 +Method (CPST, 1, NotSerialized) {
 +// _STA method - return ON status of cpu
 +// Local0 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Arg0)), Local0)
 +If (Local0) { Return(0xF) } Else { Return(0x0) }
 +}
 +Method (CPEJ, 2, NotSerialized) {
 +// _EJ0 method - eject callback
 +Sleep(200)
 +}
 +
 +/* CPU hotplug notify method */
 +OperationRegion(PRST, SystemIO, 0xaf00, 32)
 +Field (PRST, ByteAcc, NoLock, Preserve)
 +{
 +PRS, 256
 +}
 +Method(PRSC, 0) {
 +// Local5 = active cpu bitmap
 +Store (PRS, Local5)
 +// Local2 = last read byte from bitmap
 +Store (Zero, Local2)
 +// Local0 = cpuid iterator
 +Store (Zero, Local0)
 +While (LLess(Local0, SizeOf(CPON))) {
 +// Local1 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Local0)), Local1)
 +If (And(Local0, 0x07)) {
 +// Shift down previously read bitmap byte
 +ShiftRight(Local2, 1, Local2)
 +} Else {
 +// Read next byte from cpu bitmap
 +Store(DerefOf(Index(Local5, ShiftRight(Local0,
 3))), Local2) +}
 +// Local3 = active state for this cpu
 +Store(And(Local2, 1), Local3)
 +
 +If (LNotEqual(Local1, Local3)) {
 +// State change - update CPON with new state
 +Store(Local3, Index(CPON, Local0))
 +// Do CPU notify
 +If (LEqual(Local3, 1)) {
 +NTFY(Local0, 1)
 +} Else {
 +NTFY(Local0, 3)
 +}
 +}
 +Increment(Local0)
 +}
 +Return(One)
 +}
 +}
 +
  Scope (\_GPE)
  {
  Name(_HID, ACPI0006)
 @@ -701,7 +773,8 @@ DefinitionBlock (
 
  }
  Method(_L02) {
 -Return(0x01)
 +// CPU hotplug event
 +Return(\_SB.PRSC())
  }
  Method(_L03) {
  Return(0x01)
 diff --git a/src/acpi.c b/src/acpi.c
 index e91f8e0..3b49c4e 100644
 --- a/src/acpi.c
 +++ b/src/acpi.c
 @@ -1,6 +1,6 @@
  // Support for generating ACPI tables (on emulators)
  //
 -// Copyright (C) 2008,2009  Kevin O'Connor ke...@koconnor.net
 +// Copyright (C) 2008-2010  Kevin O'Connor ke...@koconnor.net
  // Copyright (C) 2006 Fabrice Bellard
  //
  // This file may be distributed under the terms of the GNU LGPLv3
 license. @@ -329,64 +329,121 @@ build_madt(void)
  return madt;
  }
 
 +// Encode a hex value
 +static inline char getHex(u32 val) {
 +val = 0x0f;
 +return (val = 9) ? ('0' + val) : ('A' + val - 10);
 +}
 +
 +// Encode a length in an SSDT.
 +static u8 *
 +encodeLen(u8 *ssdt_ptr, int length, int bytes)
 +{
 +switch (bytes) {
 +default:
 +case 4: ssdt_ptr[3] = ((length  20)  0xff);
 +case 3: ssdt_ptr[2] = ((length  12)  0xff);
 +case 2: ssdt_ptr[1] = ((length  4)  0xff);
 +ssdt_ptr[0] = (((bytes-1)  0x3)  6) | (length  0x0f);
 +break;
 +case 1: ssdt_ptr[0] = length  0x3f;
 +}
 +return ssdt_ptr + bytes;
 +}
 +
 +// AML Processor() object.  See src/ssdt-proc.dsl for info.

Re: Alt SeaBIOS SSDT cpu hotplug

2010-08-01 Thread Kevin O'Connor 
On Mon, Aug 02, 2010 at 10:41:39AM +0800, Liu, Jinsong wrote:
 Kevin,
 
 This patch still has issue, It can boot Windows 2008 DataCenter,
 however, when run cpu_set cpu online command, windows 2008
 Datacenter system shutdown at once.

Thanks for testing.

I've inspected the generated DSDT and SSDT files, and I don't see
anything wrong with them.  (To inspect a generated SSDT, uncomment the
call to hexdump(), cut and paste the hexdump output into SeaBIOS'
tools/transdump.py, and then call iasl -d on the output.)

It seems the Windows acpi interpreter is significantly different from
the Linux one.  The only guess I have is that Windows doesn't like one
of the ASL constructs even though they all look valid.  I'd try to
debug this by commenting out parts of the ASL until I narrowed down
the parts causing the problem.  Unfortunately, I don't have Windows
2008 to do this directly.

Any other ideas?

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-07-30 Thread Liu, Jinsong 
Liu, Jinsong wrote:
 Kevin O'Connor wrote:
 On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
 Kevin O'Connor wrote:
 On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
 Avi Kivity wrote:
 Very nice.  I thought about doing this but abandoned it as
 unmaintainable.  Using external functions and the ID variable,
 however, reduces the mess to tolerable proportions, and gains us
 a lot of flexibility.  We can now have any combinations of
 sockets and installed cpus.
 
 Agree, only 1 concern
 will it bring debugable/ scalable issue by hardcode aml code?
 
 I've updated the patch (see below).  This version documents how one
 can build a new version of the Processor() ssdt snippet.
 
 I've tested this under linux - there were a few bugs in the
 previous patch.  I also had to replace the dynamically created
 CPUS array with a dynamically created NTFY method - which is a bit
 more complicated.
 
 Yeah, thanks Kevin.
 After you done patch and draft test, our QA may do nightly test.
 
 Hi Jinsong,
 
 Have you had any feedback from tests?
 
 Thanks,
 -Kevin
 
 Oh, I misunderstand.
 I originally thought our QA will test after your patch send to
 upstream, so we wait for it :) 
 
 I will talk with our QA this week to arrange test for it.
 
 Thanks,
 Jinsong

Kevin,

We have done test for your patch, result are:
1. For linux 2.6.32 hvm guest, it works OK, for both vcpu add and vcpu remove, 
and re-add, re-remove, ...
2. For Window 2008 DataCenter hvm guest, it BSOD every time, QA feedback below:
==
[QA] We test it as following:
1 copy bios.bin to /usr/local/share/qemu/
 (BTW, we also test as your method , copy bios.bin to qemu-kvm/pc-bios, then at 
qemu-kvm ./configure make  make install)
2 create qemu img for Windows 2008 Datacenter
 qemu-img  create -b /share/xvs/img/Windows/ia32e_win2k8_datacenter.img  -f 
qow2 test.img
3 create guest
 qemu-system-x86_64  -hda test.img  -m 1024 -smp 2,maxvcpus=16
4 blueScreen
==

BTW, attached is the latest patch I received from you.
Do you have updated version?

Thanks,
Jinsong---BeginMessage---
On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
 Avi Kivity wrote:
  Very nice.  I thought about doing this but abandoned it as
  unmaintainable.  Using external functions and the ID variable,
  however, reduces the mess to tolerable proportions, and gains us a
  lot of flexibility.  We can now have any combinations of sockets and
  installed cpus.
 
 Agree, only 1 concern
 will it bring debugable/ scalable issue by hardcode aml code?

I've updated the patch (see below).  This version documents how one
can build a new version of the Processor() ssdt snippet.

I've tested this under linux - there were a few bugs in the previous
patch.  I also had to replace the dynamically created CPUS array with
a dynamically created NTFY method - which is a bit more complicated.

-Kevin


diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index cc31112..24674fc 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -648,6 +648,78 @@ DefinitionBlock (
 Zero   /* reserved */
 })
 
+/* CPU hotplug */
+Scope(\_SB) {
+/* Objects filled in by run-time generated SSDT */
+External(NTFY, MethodObj)
+External(CPON, PkgObj)
+
+/* Methods called by run-time generated SSDT Processor objects */
+Method (CPMA, 1, NotSerialized) {
+// _MAT method - create an madt apic buffer
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+// Local1 = Buffer (in madt apic form) to return
+Store(Buffer(8) {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0}, Local1)
+// Update the processor id, lapic id, and enable/disable status
+Store(Arg0, Index(Local1, 2))
+Store(Arg0, Index(Local1, 3))
+Store(Local0, Index(Local1, 4))
+Return (Local1)
+}
+Method (CPST, 1, NotSerialized) {
+// _STA method - return ON status of cpu
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+If (Local0) { Return(0xF) } Else { Return(0x0) }
+}
+Method (CPEJ, 2, NotSerialized) {
+// _EJ0 method - eject callback
+Sleep (0xC8)
+}
+
+/* CPU hotplug notify method */
+OperationRegion(PRST, SystemIO, 0xaf00, 32)
+Field (PRST, ByteAcc, NoLock, Preserve)
+{
+PRS, 256
+}
+Method(PRSC, 0) {
+// Local5 = active cpu bitmap
+Store (PRS, Local5)
+// Local2 = last read byte from bitmap
+Store (Zero, Local2)
+// Local0 = cpuid iterator
+Store (Zero, Local0)
+While (LLess(Local0, SizeOf(CPON))) {
+// Local1 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Local0)), Local1)
+If

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-30 Thread Kevin O'Connor 
On Sat, Jul 31, 2010 at 01:22:59AM +0800, Liu, Jinsong wrote:
 Liu, Jinsong wrote:
  Kevin O'Connor wrote:
  On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
  Kevin O'Connor wrote:
  I've tested this under linux - there were a few bugs in the
  previous patch.  I also had to replace the dynamically created
  CPUS array with a dynamically created NTFY method - which is a bit
  more complicated.
  
  Yeah, thanks Kevin.
  After you done patch and draft test, our QA may do nightly test.
[...]
 Kevin,
 
 We have done test for your patch, result are:
 1. For linux 2.6.32 hvm guest, it works OK, for both vcpu add and vcpu 
 remove, and re-add, re-remove, ...
 2. For Window 2008 DataCenter hvm guest, it BSOD every time, QA feedback 
 below:

Sorry about that.  It looks like I messed up the SSDT ScopeOp length.
New patch attached below.  I've tested it by adding/removing cpus on
Linux, and I've now also boot tested winxp and winvista.  (I don't
have Windows 2008 DataCenter.)

-Kevin


diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index cc31112..640716c 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -648,6 +648,78 @@ DefinitionBlock (
 Zero   /* reserved */
 })
 
+/* CPU hotplug */
+Scope(\_SB) {
+/* Objects filled in by run-time generated SSDT */
+External(NTFY, MethodObj)
+External(CPON, PkgObj)
+
+/* Methods called by run-time generated SSDT Processor objects */
+Method (CPMA, 1, NotSerialized) {
+// _MAT method - create an madt apic buffer
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+// Local1 = Buffer (in madt apic form) to return
+Store(Buffer(8) {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0}, Local1)
+// Update the processor id, lapic id, and enable/disable status
+Store(Arg0, Index(Local1, 2))
+Store(Arg0, Index(Local1, 3))
+Store(Local0, Index(Local1, 4))
+Return (Local1)
+}
+Method (CPST, 1, NotSerialized) {
+// _STA method - return ON status of cpu
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+If (Local0) { Return(0xF) } Else { Return(0x0) }
+}
+Method (CPEJ, 2, NotSerialized) {
+// _EJ0 method - eject callback
+Sleep(200)
+}
+
+/* CPU hotplug notify method */
+OperationRegion(PRST, SystemIO, 0xaf00, 32)
+Field (PRST, ByteAcc, NoLock, Preserve)
+{
+PRS, 256
+}
+Method(PRSC, 0) {
+// Local5 = active cpu bitmap
+Store (PRS, Local5)
+// Local2 = last read byte from bitmap
+Store (Zero, Local2)
+// Local0 = cpuid iterator
+Store (Zero, Local0)
+While (LLess(Local0, SizeOf(CPON))) {
+// Local1 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Local0)), Local1)
+If (And(Local0, 0x07)) {
+// Shift down previously read bitmap byte
+ShiftRight(Local2, 1, Local2)
+} Else {
+// Read next byte from cpu bitmap
+Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), 
Local2)
+}
+// Local3 = active state for this cpu
+Store(And(Local2, 1), Local3)
+
+If (LNotEqual(Local1, Local3)) {
+// State change - update CPON with new state
+Store(Local3, Index(CPON, Local0))
+// Do CPU notify
+If (LEqual(Local3, 1)) {
+NTFY(Local0, 1)
+} Else {
+NTFY(Local0, 3)
+}
+}
+Increment(Local0)
+}
+Return(One)
+}
+}
+
 Scope (\_GPE)
 {
 Name(_HID, ACPI0006)
@@ -701,7 +773,8 @@ DefinitionBlock (
 
 }
 Method(_L02) {
-Return(0x01)
+// CPU hotplug event
+Return(\_SB.PRSC())
 }
 Method(_L03) {
 Return(0x01)
diff --git a/src/acpi.c b/src/acpi.c
index e91f8e0..3b49c4e 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -1,6 +1,6 @@
 // Support for generating ACPI tables (on emulators)
 //
-// Copyright (C) 2008,2009  Kevin O'Connor ke...@koconnor.net
+// Copyright (C) 2008-2010  Kevin O'Connor ke...@koconnor.net
 // Copyright (C) 2006 Fabrice Bellard
 //
 // This file may be distributed under the terms of the GNU LGPLv3 license.
@@ -329,64 +329,121 @@ build_madt(void)
 return madt;
 }
 
+// Encode a hex value
+static inline char getHex(u32 val) {
+val = 0x0f;
+return (val = 9) ? ('0' + val) : ('A' + val - 10);
+}
+
+// Encode a length in an SSDT.
+static u8 *
+encodeLen(u8 *ssdt_ptr, int

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-24 Thread Kevin O'Connor 
On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
 Kevin O'Connor wrote:
  On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
  Avi Kivity wrote:
  Very nice.  I thought about doing this but abandoned it as
  unmaintainable.  Using external functions and the ID variable,
  however, reduces the mess to tolerable proportions, and gains us a
  lot of flexibility.  We can now have any combinations of sockets and
  installed cpus.
  
  Agree, only 1 concern
  will it bring debugable/ scalable issue by hardcode aml code?
  
  I've updated the patch (see below).  This version documents how one
  can build a new version of the Processor() ssdt snippet.
  
  I've tested this under linux - there were a few bugs in the previous
  patch.  I also had to replace the dynamically created CPUS array with
  a dynamically created NTFY method - which is a bit more complicated.
 
 Yeah, thanks Kevin.
 After you done patch and draft test, our QA may do nightly test.

Hi Jinsong,

Have you had any feedback from tests?

Thanks,
-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-07-24 Thread Liu, Jinsong 
Kevin O'Connor wrote:
 On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
 Kevin O'Connor wrote:
 On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
 Avi Kivity wrote:
 Very nice.  I thought about doing this but abandoned it as
 unmaintainable.  Using external functions and the ID variable,
 however, reduces the mess to tolerable proportions, and gains us a
 lot of flexibility.  We can now have any combinations of sockets
 and installed cpus.
 
 Agree, only 1 concern
 will it bring debugable/ scalable issue by hardcode aml code?
 
 I've updated the patch (see below).  This version documents how one
 can build a new version of the Processor() ssdt snippet.
 
 I've tested this under linux - there were a few bugs in the previous
 patch.  I also had to replace the dynamically created CPUS array
 with a dynamically created NTFY method - which is a bit more
 complicated. 
 
 Yeah, thanks Kevin.
 After you done patch and draft test, our QA may do nightly test.
 
 Hi Jinsong,
 
 Have you had any feedback from tests?
 
 Thanks,
 -Kevin

Oh, I misunderstand.
I originally thought our QA will test after your patch send to upstream, so we 
wait for it :)

I will talk with our QA this week to arrange test for it.

Thanks,
Jinsong--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-11 Thread Kevin O'Connor 
On Sat, Jul 10, 2010 at 12:41:18AM +0800, Liu, Jinsong wrote:
 Kevin O'Connor wrote:
  I've tested this under linux - there were a few bugs in the previous
  patch.  I also had to replace the dynamically created CPUS array with
  a dynamically created NTFY method - which is a bit more complicated.
[...]
 Yeah, thanks Kevin.
 After you done patch and draft test, our QA may do nightly test.

Thanks.  I've tested it with a Fedora Core 13 guest using kvm.  Basic
add cpu and eject cpu appears to work.

I'll wait to hear back on further testing, and for any other comments.

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-09 Thread Gleb Natapov 
On Thu, Jul 08, 2010 at 08:45:06PM -0400, Kevin O'Connor wrote:
 On Thu, Jul 08, 2010 at 03:54:10PM +0300, Gleb Natapov wrote:
  On Wed, Jul 07, 2010 at 07:26:07PM -0400, Kevin O'Connor wrote:
   On Wed, Jul 07, 2010 at 01:22:49PM +0300, Gleb Natapov wrote:
On Wed, Jul 07, 2010 at 12:57:05AM -0400, Kevin O'Connor wrote:
 The CPUS package stores references to the Processor objects, and the
 CPON package stores the state of which cpus are active.  With this
 info, hopefully there is no need to update the MADT tables.
 
The way you wrote it acpi_id is always equal to lapic_id which is not
alway true. By using MADT from memory we remove this dependency from
DSDT code.
   
   The current code always sets the apic-processor_id equal to
   apic-local_apic_id in the madt apic table.  If this changes, we could
   add a dynamically created mapping table to the ssdt.  Something like:
   
   Name(CPLA, Package() { 0x00, 0x01, 0x02, 0x03, 0x04 })
   
  Yeah, but if we can avoid it in the first place why not doing so.
 
 There is a cost to reading/writing the MADT tables.  The hardcoding of
 0x514 has a risk - it's not clear to me that an optionrom wont clobber
 that space.  It also recently occurred to me that there may be a
 problem if a guest expects the MADT address to remain constant across
 a hibernate/restore cycle - the malloc_high() function doesn't
 guarentee stable addresses across reboots.
 
That's definitely an issue. HW shouldn't change between hibernate and
resume, so boot process should be the same and address should end up be
the same too, but I wouldn't want to rely on this blindly.

 BTW how
  do you pass to DSDT what cpus are initially on? Previously this info was
  taken from memory copy of MADT too.
 
 The CPON array is dynamically generated with the status of the
 processors.  (It is then kept up to date by the DSDT code.)  The code
 for generating CPON is:
 
 // build Name(CPON, Package() { One, One, ..., Zero, Zero, ... })
 *(ssdt_ptr++) = 0x08; // NameOp
 *(ssdt_ptr++) = 'C';
 *(ssdt_ptr++) = 'P';
 *(ssdt_ptr++) = 'O';
 *(ssdt_ptr++) = 'N';
 *(ssdt_ptr++) = 0x12; // PackageOp
 ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2);
 *(ssdt_ptr++) = acpi_cpus;
 for (i=0; iacpi_cpus; i++)
 *(ssdt_ptr++) = (i  CountCPUs) ? 0x01 : 0x00;
 
See it now. Thanks.

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-07-09 Thread Liu, Jinsong 
Kevin O'Connor wrote:
 On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
 Avi Kivity wrote:
 Very nice.  I thought about doing this but abandoned it as
 unmaintainable.  Using external functions and the ID variable,
 however, reduces the mess to tolerable proportions, and gains us a
 lot of flexibility.  We can now have any combinations of sockets and
 installed cpus.
 
 Agree, only 1 concern
 will it bring debugable/ scalable issue by hardcode aml code?
 
 I've updated the patch (see below).  This version documents how one
 can build a new version of the Processor() ssdt snippet.
 
 I've tested this under linux - there were a few bugs in the previous
 patch.  I also had to replace the dynamically created CPUS array with
 a dynamically created NTFY method - which is a bit more complicated.
 
 -Kevin
 
 

Yeah, thanks Kevin.
After you done patch and draft test, our QA may do nightly test.

Thanks,
Jinsong

 diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
 index cc31112..24674fc 100644
 --- a/src/acpi-dsdt.dsl
 +++ b/src/acpi-dsdt.dsl
 @@ -648,6 +648,78 @@ DefinitionBlock (
  Zero   /* reserved */
  })
 
 +/* CPU hotplug */
 +Scope(\_SB) {
 +/* Objects filled in by run-time generated SSDT */
 +External(NTFY, MethodObj)
 +External(CPON, PkgObj)
 +
 +/* Methods called by run-time generated SSDT Processor
 objects */ +Method (CPMA, 1, NotSerialized) {
 +// _MAT method - create an madt apic buffer
 +// Local0 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Arg0)), Local0)
 +// Local1 = Buffer (in madt apic form) to return
 +Store(Buffer(8) {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0},
 Local1) +// Update the processor id, lapic id, and
 enable/disable status +Store(Arg0, Index(Local1, 2))
 +Store(Arg0, Index(Local1, 3))
 +Store(Local0, Index(Local1, 4))
 +Return (Local1)
 +}
 +Method (CPST, 1, NotSerialized) {
 +// _STA method - return ON status of cpu
 +// Local0 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Arg0)), Local0)
 +If (Local0) { Return(0xF) } Else { Return(0x0) }
 +}
 +Method (CPEJ, 2, NotSerialized) {
 +// _EJ0 method - eject callback
 +Sleep (0xC8)
 +}
 +
 +/* CPU hotplug notify method */
 +OperationRegion(PRST, SystemIO, 0xaf00, 32)
 +Field (PRST, ByteAcc, NoLock, Preserve)
 +{
 +PRS, 256
 +}
 +Method(PRSC, 0) {
 +// Local5 = active cpu bitmap
 +Store (PRS, Local5)
 +// Local2 = last read byte from bitmap
 +Store (Zero, Local2)
 +// Local0 = cpuid iterator
 +Store (Zero, Local0)
 +While (LLess(Local0, SizeOf(CPON))) {
 +// Local1 = CPON flag for this cpu
 +Store(DerefOf(Index(CPON, Local0)), Local1)
 +If (And(Local0, 0x07)) {
 +// Shift down previously read bitmap byte
 +ShiftRight(Local2, 1, Local2)
 +} Else {
 +// Read next byte from cpu bitmap
 +Store(DerefOf(Index(Local5, ShiftRight(Local0,
 3))), Local2) +}
 +// Local3 = active state for this cpu
 +Store(And(Local2, 1), Local3)
 +
 +If (LNotEqual(Local1, Local3)) {
 +// State change - update CPON with new state
 +Store(Local3, Index(CPON, Local0))
 +// Do CPU notify
 +If (LEqual(Local3, 1)) {
 +NTFY(Local0, 1)
 +} Else {
 +NTFY(Local0, 3)
 +}
 +}
 +Increment(Local0)
 +}
 +Return(One)
 +}
 +}
 +
  Scope (\_GPE)
  {
  Name(_HID, ACPI0006)
 @@ -701,7 +773,8 @@ DefinitionBlock (
 
  }
  Method(_L02) {
 -Return(0x01)
 +// CPU hotplug event
 +Return(\_SB.PRSC())
  }
  Method(_L03) {
  Return(0x01)
 diff --git a/src/acpi.c b/src/acpi.c
 index 0559443..082ef73 100644
 --- a/src/acpi.c
 +++ b/src/acpi.c
 @@ -406,16 +406,56 @@ build_madt(void)
  return madt;
  }
 
 +// Encode a hex value
 +static inline char getHex(u32 val) {
 +val = 0x0f;
 +return (val = 9) ? ('0' + val) : ('A' + val - 10);
 +}
 +
 +// Encode a length in an SSDT.
 +static u8 *
 +encodeLen(u8 *ssdt_ptr, int length, int bytes)
 +{
 +if (bytes = 1) {
 +*ssdt_ptr = length  0x3f;
 +return ssdt_ptr+1;
 +}
 +ssdt_ptr[0] = (((bytes-1)  0x3)  6) | (length  0x0f);
 +ssdt_ptr[1] = ((length  4)  0xff);
 +ssdt_ptr[2] = ((length  12)  0xff);
 +ssdt_ptr[3] = ((length

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-08 Thread Gleb Natapov 
On Wed, Jul 07, 2010 at 07:26:07PM -0400, Kevin O'Connor wrote:
 On Wed, Jul 07, 2010 at 01:22:49PM +0300, Gleb Natapov wrote:
  On Wed, Jul 07, 2010 at 12:57:05AM -0400, Kevin O'Connor wrote:
   The CPUS package stores references to the Processor objects, and the
   CPON package stores the state of which cpus are active.  With this
   info, hopefully there is no need to update the MADT tables.
   
  The way you wrote it acpi_id is always equal to lapic_id which is not
  alway true. By using MADT from memory we remove this dependency from
  DSDT code.
 
 The current code always sets the apic-processor_id equal to
 apic-local_apic_id in the madt apic table.  If this changes, we could
 add a dynamically created mapping table to the ssdt.  Something like:
 
 Name(CPLA, Package() { 0x00, 0x01, 0x02, 0x03, 0x04 })
 
Yeah, but if we can avoid it in the first place why not doing so. BTW how
do you pass to DSDT what cpus are initially on? Previously this info was
taken from memory copy of MADT too.

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Alt SeaBIOS SSDT cpu hotplug

2010-07-08 Thread Liu, Jinsong 
Avi Kivity wrote:
 On 07/07/2010 07:57 AM, Kevin O'Connor wrote:
 Hi,
 
 I've been playing with the cpu hotplug SSDT changes.  Attached is a
 proposal for an alternative method of adding ACPI support.
 
 The idea is to continue to build a dynamic SSDT based on CountCPUs
 and MaxCountCPUs.  The dynamic SSDT entries just call methods in the
 main DSDT. 
 
 This is completely untested.  Hopefully the patch will demonstrate
 the idea though. 
 
 The objective is to dynamically build an SSDT that looks something
 like: 
 
 === {
  Scope (_SB) {
  External(CPMA, MethodObj)
  External(CPST, MethodObj)
  External(CPEJ, MethodObj)
 #define DefCPU(nr)  \
  Processor (CP##nr, 0x##nr, 0xb010, 0x06) {  \
  Name (_HID, ACPI0007) \
  Name (ID, 0x##nr)   \
  Method(_MAT, 0) {   \
  Return(CPMA(ID))\
  }   \
  Method (_STA) { \
  Return(CPST(ID))\
  }   \
  Method (_EJ0, 1, NotSerialized) {   \
  Return(CPEJ(ID, Arg0))  \
  }   \  }
  DefCPU(00)
  DefCPU(01)
  DefCPU(02)
  DefCPU(03)
  DefCPU(AA)
  Name(CPUS, Package() {
  CP00, CP01, CP02, CP03, CPAA,
  })
  Name(CPON, Package() {
  One, One, One, Zero, Zero
  })
  }
 }
 ===
 
 with a dynamic number of cpus.
 
 The CPUS package stores references to the Processor objects, and
 the CPON package stores the state of which cpus are active.  With
 this info, hopefully there is no need to update the MADT tables.
 
 Thoughts?
 
 
 Very nice.  I thought about doing this but abandoned it as
 unmaintainable.  Using external functions and the ID variable,
 however, reduces the mess to tolerable proportions, and gains us a
 lot of flexibility.  We can now have any combinations of sockets and
 installed cpus.

Agree, only 1 concern
will it bring debugable/ scalable issue by hardcode aml code?

Thanks,
Jinsong--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-08 Thread Kevin O'Connor 
On Thu, Jul 08, 2010 at 03:54:10PM +0300, Gleb Natapov wrote:
 On Wed, Jul 07, 2010 at 07:26:07PM -0400, Kevin O'Connor wrote:
  On Wed, Jul 07, 2010 at 01:22:49PM +0300, Gleb Natapov wrote:
   On Wed, Jul 07, 2010 at 12:57:05AM -0400, Kevin O'Connor wrote:
The CPUS package stores references to the Processor objects, and the
CPON package stores the state of which cpus are active.  With this
info, hopefully there is no need to update the MADT tables.

   The way you wrote it acpi_id is always equal to lapic_id which is not
   alway true. By using MADT from memory we remove this dependency from
   DSDT code.
  
  The current code always sets the apic-processor_id equal to
  apic-local_apic_id in the madt apic table.  If this changes, we could
  add a dynamically created mapping table to the ssdt.  Something like:
  
  Name(CPLA, Package() { 0x00, 0x01, 0x02, 0x03, 0x04 })
  
 Yeah, but if we can avoid it in the first place why not doing so.

There is a cost to reading/writing the MADT tables.  The hardcoding of
0x514 has a risk - it's not clear to me that an optionrom wont clobber
that space.  It also recently occurred to me that there may be a
problem if a guest expects the MADT address to remain constant across
a hibernate/restore cycle - the malloc_high() function doesn't
guarentee stable addresses across reboots.

BTW how
 do you pass to DSDT what cpus are initially on? Previously this info was
 taken from memory copy of MADT too.

The CPON array is dynamically generated with the status of the
processors.  (It is then kept up to date by the DSDT code.)  The code
for generating CPON is:

// build Name(CPON, Package() { One, One, ..., Zero, Zero, ... })
*(ssdt_ptr++) = 0x08; // NameOp
*(ssdt_ptr++) = 'C';
*(ssdt_ptr++) = 'P';
*(ssdt_ptr++) = 'O';
*(ssdt_ptr++) = 'N';
*(ssdt_ptr++) = 0x12; // PackageOp
ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2);
*(ssdt_ptr++) = acpi_cpus;
for (i=0; iacpi_cpus; i++)
*(ssdt_ptr++) = (i  CountCPUs) ? 0x01 : 0x00;

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-08 Thread Kevin O'Connor 
On Thu, Jul 08, 2010 at 09:19:13PM +0800, Liu, Jinsong wrote:
 Avi Kivity wrote:
  Very nice.  I thought about doing this but abandoned it as
  unmaintainable.  Using external functions and the ID variable,
  however, reduces the mess to tolerable proportions, and gains us a
  lot of flexibility.  We can now have any combinations of sockets and
  installed cpus.
 
 Agree, only 1 concern
 will it bring debugable/ scalable issue by hardcode aml code?

I've updated the patch (see below).  This version documents how one
can build a new version of the Processor() ssdt snippet.

I've tested this under linux - there were a few bugs in the previous
patch.  I also had to replace the dynamically created CPUS array with
a dynamically created NTFY method - which is a bit more complicated.

-Kevin


diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index cc31112..24674fc 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -648,6 +648,78 @@ DefinitionBlock (
 Zero   /* reserved */
 })
 
+/* CPU hotplug */
+Scope(\_SB) {
+/* Objects filled in by run-time generated SSDT */
+External(NTFY, MethodObj)
+External(CPON, PkgObj)
+
+/* Methods called by run-time generated SSDT Processor objects */
+Method (CPMA, 1, NotSerialized) {
+// _MAT method - create an madt apic buffer
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+// Local1 = Buffer (in madt apic form) to return
+Store(Buffer(8) {0x00, 0x08, 0x00, 0x00, 0x00, 0, 0, 0}, Local1)
+// Update the processor id, lapic id, and enable/disable status
+Store(Arg0, Index(Local1, 2))
+Store(Arg0, Index(Local1, 3))
+Store(Local0, Index(Local1, 4))
+Return (Local1)
+}
+Method (CPST, 1, NotSerialized) {
+// _STA method - return ON status of cpu
+// Local0 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Arg0)), Local0)
+If (Local0) { Return(0xF) } Else { Return(0x0) }
+}
+Method (CPEJ, 2, NotSerialized) {
+// _EJ0 method - eject callback
+Sleep (0xC8)
+}
+
+/* CPU hotplug notify method */
+OperationRegion(PRST, SystemIO, 0xaf00, 32)
+Field (PRST, ByteAcc, NoLock, Preserve)
+{
+PRS, 256
+}
+Method(PRSC, 0) {
+// Local5 = active cpu bitmap
+Store (PRS, Local5)
+// Local2 = last read byte from bitmap
+Store (Zero, Local2)
+// Local0 = cpuid iterator
+Store (Zero, Local0)
+While (LLess(Local0, SizeOf(CPON))) {
+// Local1 = CPON flag for this cpu
+Store(DerefOf(Index(CPON, Local0)), Local1)
+If (And(Local0, 0x07)) {
+// Shift down previously read bitmap byte
+ShiftRight(Local2, 1, Local2)
+} Else {
+// Read next byte from cpu bitmap
+Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), 
Local2)
+}
+// Local3 = active state for this cpu
+Store(And(Local2, 1), Local3)
+
+If (LNotEqual(Local1, Local3)) {
+// State change - update CPON with new state
+Store(Local3, Index(CPON, Local0))
+// Do CPU notify
+If (LEqual(Local3, 1)) {
+NTFY(Local0, 1)
+} Else {
+NTFY(Local0, 3)
+}
+}
+Increment(Local0)
+}
+Return(One)
+}
+}
+
 Scope (\_GPE)
 {
 Name(_HID, ACPI0006)
@@ -701,7 +773,8 @@ DefinitionBlock (
 
 }
 Method(_L02) {
-Return(0x01)
+// CPU hotplug event
+Return(\_SB.PRSC())
 }
 Method(_L03) {
 Return(0x01)
diff --git a/src/acpi.c b/src/acpi.c
index 0559443..082ef73 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -406,16 +406,56 @@ build_madt(void)
 return madt;
 }
 
+// Encode a hex value
+static inline char getHex(u32 val) {
+val = 0x0f;
+return (val = 9) ? ('0' + val) : ('A' + val - 10);
+}
+
+// Encode a length in an SSDT.
+static u8 *
+encodeLen(u8 *ssdt_ptr, int length, int bytes)
+{
+if (bytes = 1) {
+*ssdt_ptr = length  0x3f;
+return ssdt_ptr+1;
+}
+ssdt_ptr[0] = (((bytes-1)  0x3)  6) | (length  0x0f);
+ssdt_ptr[1] = ((length  4)  0xff);
+ssdt_ptr[2] = ((length  12)  0xff);
+ssdt_ptr[3] = ((length  20)  0xff);
+return ssdt_ptr + bytes;
+}
+
+// AML Processor() object.  See src/ssdt-proc.dsl for info.
+static unsigned char ssdt_proc[] = {
+0x5b,0x83,0x43,0x05,0x43,0x50,0x41,0x41,
+0xaa,0x10,0xb0,0x00,0x00,0x06,0x08,0x49,
+

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-07 Thread Avi Kivity 

On 07/07/2010 07:57 AM, Kevin O'Connor wrote:

Hi,

I've been playing with the cpu hotplug SSDT changes.  Attached is a
proposal for an alternative method of adding ACPI support.

The idea is to continue to build a dynamic SSDT based on CountCPUs and
MaxCountCPUs.  The dynamic SSDT entries just call methods in the main
DSDT.

This is completely untested.  Hopefully the patch will demonstrate the
idea though.

The objective is to dynamically build an SSDT that looks something
like:

===
{
 Scope (_SB) {
 External(CPMA, MethodObj)
 External(CPST, MethodObj)
 External(CPEJ, MethodObj)
#define DefCPU(nr)  \
 Processor (CP##nr, 0x##nr, 0xb010, 0x06) {  \
 Name (_HID, ACPI0007) \
 Name (ID, 0x##nr)   \
 Method(_MAT, 0) {   \
 Return(CPMA(ID))\
 }   \
 Method (_STA) { \
 Return(CPST(ID))\
 }   \
 Method (_EJ0, 1, NotSerialized) {   \
 Return(CPEJ(ID, Arg0))  \
 }   \
 }
 DefCPU(00)
 DefCPU(01)
 DefCPU(02)
 DefCPU(03)
 DefCPU(AA)
 Name(CPUS, Package() {
 CP00, CP01, CP02, CP03, CPAA,
 })
 Name(CPON, Package() {
 One, One, One, Zero, Zero
 })
 }
}
===

with a dynamic number of cpus.

The CPUS package stores references to the Processor objects, and the
CPON package stores the state of which cpus are active.  With this
info, hopefully there is no need to update the MADT tables.

Thoughts?
   


Very nice.  I thought about doing this but abandoned it as 
unmaintainable.  Using external functions and the ID variable, however, 
reduces the mess to tolerable proportions, and gains us a lot of 
flexibility.  We can now have any combinations of sockets and installed 
cpus.


--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-07 Thread Gleb Natapov 
On Wed, Jul 07, 2010 at 12:57:05AM -0400, Kevin O'Connor wrote:
 Hi,
 
 I've been playing with the cpu hotplug SSDT changes.  Attached is a
 proposal for an alternative method of adding ACPI support.
 
 The idea is to continue to build a dynamic SSDT based on CountCPUs and
 MaxCountCPUs.  The dynamic SSDT entries just call methods in the main
 DSDT.
 
If it is doable and remains to be debugable it is great.

 This is completely untested.  Hopefully the patch will demonstrate the
 idea though.
 
 The objective is to dynamically build an SSDT that looks something
 like:
 
 ===
 {
 Scope (_SB) {
 External(CPMA, MethodObj)
 External(CPST, MethodObj)
 External(CPEJ, MethodObj)
 #define DefCPU(nr)  \
 Processor (CP##nr, 0x##nr, 0xb010, 0x06) {  \
 Name (_HID, ACPI0007) \
 Name (ID, 0x##nr)   \
 Method(_MAT, 0) {   \
 Return(CPMA(ID))\
 }   \
 Method (_STA) { \
 Return(CPST(ID))\
 }   \
 Method (_EJ0, 1, NotSerialized) {   \
 Return(CPEJ(ID, Arg0))  \
 }   \
 }
 DefCPU(00)
 DefCPU(01)
 DefCPU(02)
 DefCPU(03)
 DefCPU(AA)
 Name(CPUS, Package() {
 CP00, CP01, CP02, CP03, CPAA,
 })
 Name(CPON, Package() {
 One, One, One, Zero, Zero
 })
 }
 }
 ===
 
 with a dynamic number of cpus.
 
 The CPUS package stores references to the Processor objects, and the
 CPON package stores the state of which cpus are active.  With this
 info, hopefully there is no need to update the MADT tables.
 
The way you wrote it acpi_id is always equal to lapic_id which is not
alway true. By using MADT from memory we remove this dependency from
DSDT code.

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alt SeaBIOS SSDT cpu hotplug

2010-07-07 Thread Kevin O'Connor 
On Wed, Jul 07, 2010 at 01:22:49PM +0300, Gleb Natapov wrote:
 On Wed, Jul 07, 2010 at 12:57:05AM -0400, Kevin O'Connor wrote:
  The CPUS package stores references to the Processor objects, and the
  CPON package stores the state of which cpus are active.  With this
  info, hopefully there is no need to update the MADT tables.
  
 The way you wrote it acpi_id is always equal to lapic_id which is not
 alway true. By using MADT from memory we remove this dependency from
 DSDT code.

The current code always sets the apic-processor_id equal to
apic-local_apic_id in the madt apic table.  If this changes, we could
add a dynamically created mapping table to the ssdt.  Something like:

Name(CPLA, Package() { 0x00, 0x01, 0x02, 0x03, 0x04 })

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html