[PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Liu, Jinsong]

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

Vcpu hotplug: Move ACPI processor from \_PR to \_SB

Move processor from \_PR to \_SB. ACPI processor can be defined under 
\_PR or \_SB. However, recently os like linux 2.6.30/32 support cpu hotplug 
better for \_SB processor object.

Signed-off-by: Jiang, Yunhong   <yunhong.jiang@intel.com>
                     Liu, Jinsong     <jinsong.liu@intel.com>

[-- Attachment #2: ras_vcpuhotplug_1.patch --]
[-- Type: application/octet-stream, Size: 4166 bytes --]

Vcpu hotplug: Move ACPI	processor from \_PR to \_SB

Move processor from \_PR to \_SB. ACPI processor can be defined under 
\_PR or \_SB. However, recently os like linux 2.6.30/32 support cpu hotplug 
better for \_SB processor object.

Signed-off-by: Jiang, Yunhong   <yunhong.jiang@intel.com>
               Liu, Jinsong     <jinsong.liu@intel.com>

diff -r 5b895c3f4386 tools/firmware/hvmloader/acpi/Makefile
--- a/tools/firmware/hvmloader/acpi/Makefile	Mon Feb 08 10:18:51 2010 +0000
+++ b/tools/firmware/hvmloader/acpi/Makefile	Thu Feb 11 14:38:39 2010 +0800
@@ -35,7 +35,7 @@ dsdt.c: dsdt.asl mk_dsdt.c
 dsdt.c: dsdt.asl mk_dsdt.c
 	$(MAKE) iasl
 	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_include) -o mk_dsdt mk_dsdt.c
-	head -n -1 $< >_dsdt.asl
+	head -n -2 $< >_dsdt.asl
 	./mk_dsdt >>_dsdt.asl
 	iasl -tc _dsdt.asl
 	mv _dsdt.hex dsdt.c
diff -r 5b895c3f4386 tools/firmware/hvmloader/acpi/mk_dsdt.c
--- a/tools/firmware/hvmloader/acpi/mk_dsdt.c	Mon Feb 08 10:18:51 2010 +0000
+++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c	Thu Feb 11 14:38:39 2010 +0800
@@ -77,11 +77,9 @@ int main(void)
 
     /**** DSDT DefinitionBlock start ****/
     /* (we append to existing DSDT definition block) */
-    indent_level++;
+    indent_level+=2;
 
     /**** Processor start ****/
-    push_block("Scope", "\\_PR");
-
     /* MADT checksum */
     stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1");
     push_block("Field", "MSUM, ByteAcc, NoLock, Preserve");
@@ -91,7 +89,7 @@ int main(void)
     /* Define processor objects and control methods. */
     for ( cpu = 0; cpu < HVM_MAX_VCPUS; cpu++)
     {
-        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
+        push_block("Processor", "\\_SB.PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
 
         stmt("Name", "_HID, \"ACPI0007\"");
 
@@ -117,7 +115,7 @@ int main(void)
         stmt("Return", "0xF");
         pop_block();
         push_block("Else", NULL);
-        stmt("Return", "0x9");
+        stmt("Return", "0x0");
         pop_block();
         pop_block();
 
@@ -136,7 +134,7 @@ int main(void)
 
     /* Control method 'PRSC': CPU hotplug GPE handler. */
     push_block("Method", "PRSC, 0");
-    stmt("Store", "PRS, Local0");
+    stmt("Store", "ToBuffer(PRS), Local0");
     for ( cpu = 0; cpu < HVM_MAX_VCPUS; cpu++ )
     {
         /* Read a byte at a time from the PRST online-CPU bitmask. */
@@ -147,16 +145,16 @@ int main(void)
         /* Extract current CPU's status: 0=offline; 1=online. */
         stmt("And", "Local1, 1, Local2");
         /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
-        push_block("If", "LNotEqual(Local2, \\_PR.PR%02X.FLG)", cpu);
+        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
         /* ...If not, update it and the MADT checksum, and notify OSPM. */
-        stmt("Store", "Local2, \\_PR.PR%02X.FLG", cpu);
+        stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
         push_block("If", "LEqual(Local2, 1)");
-        stmt("Notify", "PR%02X, 1", cpu); /* Notify: Device Check */
-        stmt("Subtract", "\\_PR.MSU, 1, \\_PR.MSU"); /* Adjust MADT csum */
+        stmt("Notify", "\\_SB.PR%02X, 1", cpu); /* Notify: Device Check */
+        stmt("Subtract", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         push_block("Else", NULL);
-        stmt("Notify", "PR%02X, 3", cpu); /* Notify: Eject Request */
-        stmt("Add", "\\_PR.MSU, 1, \\_PR.MSU"); /* Adjust MADT csum */
+        stmt("Notify", "\\_SB.PR%02X, 3", cpu); /* Notify: Eject Request */
+        stmt("Add", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         pop_block();
     }
@@ -168,10 +166,11 @@ int main(void)
     /* Define GPE control method '_L02'. */
     push_block("Scope", "\\_GPE");
     push_block("Method", "_L02");
-    stmt("Return", "\\_PR.PRSC()");
-    pop_block();
+    stmt("Return", "\\_SB.PRSC()");
     pop_block();
     /**** Processor end ****/
+    /**** Scope end *******/
+    pop_block();
 
 
     /**** PCI0 start ****/

[-- 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 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

On 11/02/2010 17:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> Vcpu hotplug: Move ACPI processor from \_PR to \_SB
> 
> Move processor from \_PR to \_SB. ACPI processor can be defined under
> \_PR or \_SB. However, recently os like linux 2.6.30/32 support cpu hotplug
> better for \_SB processor object.

What's 'better' about it? I don't want to mess with that kind of thing right
now unless there's a good reason. In my experience the majority of systems
still define a \_PR block -- is Linux having some kind of problem with all
of them?

 -- Keir

> Signed-off-by: Jiang, Yunhong   <yunhong.jiang@intel.com>
>                      Liu, Jinsong     <jinsong.liu@intel.com>

RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Jiang, Yunhong]

Per my checking to the code, the Linux has bug to support cpu hotplug for \_PR defined processor object.

In kernel, the _PR is defined as ACPI_TYPE_LOCAL_SCOPE type (see acpi_gbl_pre_defined_names ) and is skipped without creating corresponding device in acpi_bus_scan (checking at acpi_bus_check_add), while _SB is defined as ACPI_TYPE_DEVICE.

When DEVICE_CHECK notify received for a processor object, acpi_processor_device_add() will be called. When acpi_processor_device_add try to get parent device, it will fail for \_PR situation. _SB is ok because it is defined as ACPI_TYPE_DEVICE. 

static
int acpi_processor_device_add(acpi_handle handle, struct acpi_device **device)
{ 
.......
	acpi_bus_get_device(phandle, &pdev)) {
        return -ENODEV;
    }

This issue is workaround by lucky in current Xen code for 2.6.30 kernel, mainly because of the non-present Processor's status is defined as 0x9 (present, functional, not enabled) in Processor object's _STA function. In 2.6.30 kernel, this means a device is created by acpi module when kernel booting. Later, when the processor hot added, acpi_processor_hotplug_notify() will not call the acpi_processor_device_add(), and call the acpi_processor_start() directly to start the processor.
However, in 2.6.32, changeset 970b04929a68134acca17878b1d93e115e58c12a remove the acpi_processor_start(), remove the acpi_processor_start() at that point, assume the acpi_processor_device_add() will call the acpi_processor_start() in the end. That means in .32 kernel, s the CPU is not started when hot-added. That means the workaround of 0x9 _STA is not working here.

In fact, marked a non-present CPU as 0x9 is really strange. We should of course return 0x0 for non-present processor. However, after change the _STA to return 0x0, it will fail for even 30 kernel for _PR implementation, because then the processor device is not crated by acpi module when kernel booting, and the acpi_processor_device_add() can't get the parent device, the new processor device will not be added when hot-plug.

So in summary, kernel has hotplug bug for Processor defined under _PR, which is workaround in xen by lucky for .30 kernel, but failed in .32 kernel.

I also noticed in Win2k8 data center version, cpu hotplug successed for _SB definition, but failed for _PR definition. (success mean, after we hot-add vcpu, the device manager will shown new processor, but the task manager has no changes, same as http://www.boche.net/blog/index.php/2009/05/10/vsphere-memory-hot-add-cpu-hot-plug/ .Fail mean even the device manager has no changes). But I didn't debug to the w2k8 issue.

However, according to ACPI spec, the _PR is for ACPI1.0 compatible. We have no idea which OS is ACPI 1.0 OS. As HeQing found ACPI 1.0 bugs in Win2K, so we assume W2K is ACPI 1.0. We test shows W2K guest is ok with the _SB definition in our testing. Maybe Win98/WinMe is ACPI 1.0, but we have no image for these OS. But yes, that's a main issue for _SB method and we need more consideration here.

In fact, we have internal argue to choose _PR or _SB method before Jinsong's initial patch sent out. Later _PR method is chosen because of the ACPI 1.0 compatible benifit, and kernel 30 version is ok. (IIRC, .32 kernel is not released at that time). 

--jyh

>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: Friday, February 12, 2010 2:36 AM
>To: Liu, Jinsong; xen-devel
>Cc: Jiang, Yunhong
>Subject: Re: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB
>
>On 11/02/2010 17:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>
>> Vcpu hotplug: Move ACPI processor from \_PR to \_SB
>>
>> Move processor from \_PR to \_SB. ACPI processor can be defined under
>> \_PR or \_SB. However, recently os like linux 2.6.30/32 support cpu hotplug
>> better for \_SB processor object.
>
>What's 'better' about it? I don't want to mess with that kind of thing right
>now unless there's a good reason. In my experience the majority of systems
>still define a \_PR block -- is Linux having some kind of problem with all
>of them?
>
> -- Keir
>
>> Signed-off-by: Jiang, Yunhong   <yunhong.jiang@intel.com>
>>                      Liu, Jinsong     <jinsong.liu@intel.com>
>

Re: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

On 12/02/2010 02:57, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> However, according to ACPI spec, the _PR is for ACPI1.0 compatible. We have no
> idea which OS is ACPI 1.0 OS. As HeQing found ACPI 1.0 bugs in Win2K, so we
> assume W2K is ACPI 1.0. We test shows W2K guest is ok with the _SB definition
> in our testing. Maybe Win98/WinMe is ACPI 1.0, but we have no image for these
> OS. But yes, that's a main issue for _SB method and we need more consideration
> here.
> 
> In fact, we have internal argue to choose _PR or _SB method before Jinsong's
> initial patch sent out. Later _PR method is chosen because of the ACPI 1.0
> compatible benifit, and kernel 30 version is ok. (IIRC, .32 kernel is not
> released at that time).

Well, that's tricky. 2.6.32 is supposed to be a long-term maintained kernel,
so presumably there will be a 2.6.32.x along in the not-too-far future which
fixes this Linux bug? I feel we're a bit close to the wire to make this
change now.

I'd be a bit more comfortable if we had the cover of lots of other modern
systems putting their processor objects under \_SB, but actually I've never
seen one. Then again I haven't been looking at high-end systems supporting
CPU hotplug and the like.

 -- Keir

RE: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Jiang, Yunhong]

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: Friday, February 12, 2010 4:30 PM
>To: Jiang, Yunhong; Liu, Jinsong; xen-devel
>Subject: Re: [Xen-devel] RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from
>\_PR to \_SB
>
>On 12/02/2010 02:57, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>
>> However, according to ACPI spec, the _PR is for ACPI1.0 compatible. We have no
>> idea which OS is ACPI 1.0 OS. As HeQing found ACPI 1.0 bugs in Win2K, so we
>> assume W2K is ACPI 1.0. We test shows W2K guest is ok with the _SB definition
>> in our testing. Maybe Win98/WinMe is ACPI 1.0, but we have no image for these
>> OS. But yes, that's a main issue for _SB method and we need more consideration
>> here.
>>
>> In fact, we have internal argue to choose _PR or _SB method before Jinsong's
>> initial patch sent out. Later _PR method is chosen because of the ACPI 1.0
>> compatible benifit, and kernel 30 version is ok. (IIRC, .32 kernel is not
>> released at that time).
>
>Well, that's tricky. 2.6.32 is supposed to be a long-term maintained kernel,
>so presumably there will be a 2.6.32.x along in the not-too-far future which
>fixes this Linux bug? I feel we're a bit close to the wire to make this
>change now.

We talked this issue with our colleague working on kernel side. As one key engineer is on vocation, I will get more information when he is back after Chinese New Year. 
But I'm not sure of Win2K8.

>
>I'd be a bit more comfortable if we had the cover of lots of other modern
>systems putting their processor objects under \_SB, but actually I've never
>seen one. Then again I haven't been looking at high-end systems supporting
>CPU hotplug and the like.

Yes. I only saw \_SB definition in system supporting CPU hotplug. In fact, in that system, the processor is defined under an container object in \_SB. As currently all system in our lab is shutdown for CNY, I can't find more system to check. And I suspect that we need care \_PR soluation, legacy OS support is an important usage model for virtualization.

One thing I noticed in my system is, there is a ACPI version option in my desktop system, and I remember I saw that option in other system also. So one possible solution is, place all processor definition under a seperated SSDT file. An option is provided so that build.c can select different SSDT based on user's input. But that make thing tricky still.

--jyh

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

Re: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

On 12/02/2010 09:25, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

>> I'd be a bit more comfortable if we had the cover of lots of other modern
>> systems putting their processor objects under \_SB, but actually I've never
>> seen one. Then again I haven't been looking at high-end systems supporting
>> CPU hotplug and the like.
> 
> Yes. I only saw \_SB definition in system supporting CPU hotplug. In fact, in
> that system, the processor is defined under an container object in \_SB. As
> currently all system in our lab is shutdown for CNY, I can't find more system
> to check. And I suspect that we need care \_PR soluation, legacy OS support is
> an important usage model for virtualization.
> 
> One thing I noticed in my system is, there is a ACPI version option in my
> desktop system, and I remember I saw that option in other system also. So one
> possible solution is, place all processor definition under a seperated SSDT
> file. An option is provided so that build.c can select different SSDT based on
> user's input. But that make thing tricky still.

You can see that xen-unstable tip can do this now. But I don't want to start
dumping in loads of alternative DSDTs, as each one is a fair size. What I'm
hoping is that this Linux regression is fixed fairly swiftly, and we can
hence ignore it. :-) If not, we can think about what to do.

 -- Keir

RE: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Jiang, Yunhong]

>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: Friday, February 12, 2010 5:31 PM
>To: Jiang, Yunhong; Liu, Jinsong; xen-devel
>Subject: Re: [Xen-devel] RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from
>\_PR to \_SB
>
>On 12/02/2010 09:25, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>
>>> I'd be a bit more comfortable if we had the cover of lots of other modern
>>> systems putting their processor objects under \_SB, but actually I've never
>>> seen one. Then again I haven't been looking at high-end systems supporting
>>> CPU hotplug and the like.
>>
>> Yes. I only saw \_SB definition in system supporting CPU hotplug. In fact, in
>> that system, the processor is defined under an container object in \_SB. As
>> currently all system in our lab is shutdown for CNY, I can't find more system
>> to check. And I suspect that we need care \_PR soluation, legacy OS support is
>> an important usage model for virtualization.
>>
>> One thing I noticed in my system is, there is a ACPI version option in my
>> desktop system, and I remember I saw that option in other system also. So one
>> possible solution is, place all processor definition under a seperated SSDT
>> file. An option is provided so that build.c can select different SSDT based on
>> user's input. But that make thing tricky still.
>
>You can see that xen-unstable tip can do this now. But I don't want to start
>dumping in loads of alternative DSDTs, as each one is a fair size. What I'm
>hoping is that this Linux regression is fixed fairly swiftly, and we can
>hence ignore it. :-) If not, we can think about what to do.

Sure, we will try to talk with our kernel engineer on this after CNY.

--jyh

>
> -- Keir
>

RE: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Liu, Jinsong]

Keir Fraser wrote:
> On 12/02/2010 09:25, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> 
>>> I'd be a bit more comfortable if we had the cover of lots of other
>>> modern systems putting their processor objects under \_SB, but
>>> actually I've never seen one. Then again I haven't been looking at
>>> high-end systems supporting CPU hotplug and the like.
>> 
>> Yes. I only saw \_SB definition in system supporting CPU hotplug. In
>> fact, in that system, the processor is defined under an container
>> object in \_SB. As currently all system in our lab is shutdown for
>> CNY, I can't find more system to check. And I suspect that we need
>> care \_PR soluation, legacy OS support is an important usage model
>> for virtualization. 
>> 
>> One thing I noticed in my system is, there is a ACPI version option
>> in my desktop system, and I remember I saw that option in other
>> system also. So one possible solution is, place all processor
>> definition under a seperated SSDT file. An option is provided so
>> that build.c can select different SSDT based on user's input. But
>> that make thing tricky still. 
> 
> You can see that xen-unstable tip can do this now. But I don't want
> to start dumping in loads of alternative DSDTs, as each one is a fair
> size. What I'm hoping is that this Linux regression is fixed fairly
> swiftly, and we can hence ignore it. :-) If not, we can think about
> what to do. 
> 
>  -- Keir

Yes, we hope so :)
Compared with linux 2.6.30, 2.6.32 change a lot at processor_hotplug path, but still not clean to support \_PR processor, while both versions seems work well for \_SB processor. Win2k8 is similar situation.

Thanks,
Jinsong

Re: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

On 12/02/2010 09:50, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

>> You can see that xen-unstable tip can do this now. But I don't want
>> to start dumping in loads of alternative DSDTs, as each one is a fair
>> size. What I'm hoping is that this Linux regression is fixed fairly
>> swiftly, and we can hence ignore it. :-) If not, we can think about
>> what to do. 
>> 
>>  -- Keir
> 
> Yes, we hope so :)
> Compared with linux 2.6.30, 2.6.32 change a lot at processor_hotplug path, but
> still not clean to support \_PR processor, while both versions seems work well
> for \_SB processor. Win2k8 is similar situation.

Win2k8 has a similar problem?

 -- Keir

RE: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Liu, Jinsong]

Keir Fraser wrote:
> On 12/02/2010 09:50, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 
>>> You can see that xen-unstable tip can do this now. But I don't want
>>> to start dumping in loads of alternative DSDTs, as each one is a
>>> fair size. What I'm hoping is that this Linux regression is fixed
>>> fairly swiftly, and we can hence ignore it. :-) If not, we can
>>> think about what to do. 
>>> 
>>>  -- Keir
>> 
>> Yes, we hope so :)
>> Compared with linux 2.6.30, 2.6.32 change a lot at processor_hotplug
>> path, but still not clean to support \_PR processor, while both
>> versions seems work well for \_SB processor. Win2k8 is similar
>> situation. 
> 
> Win2k8 has a similar problem?
> 
>  -- Keir

Yes. Yunhong test Win2k8, and found in Win2k8 data center version, cpu hotplug successed for _SB definition, but failed for _PR definition. (success mean, after we hot-add vcpu, the device manager will shown new processor, but the task manager has no changes, same as http://www.boche.net/blog/index.php/2009/05/10/vsphere-memory-hot-add-cpu-hot-plug/ .Fail mean even the device manager has no changes).

Thanks,
Jinsong

Re: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

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

On 12/02/2010 10:07, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

>> Win2k8 has a similar problem?
>> 
>>  -- Keir
> 
> Yes. Yunhong test Win2k8, and found in Win2k8 data center version, cpu hotplug
> successed for _SB definition, but failed for _PR definition. (success mean,
> after we hot-add vcpu, the device manager will shown new processor, but the
> task manager has no changes, same as
> http://www.boche.net/blog/index.php/2009/05/10/vsphere-memory-hot-add-cpu-hot-
> plug/ .Fail mean even the device manager has no changes).

Okay, how about the attached alternative patch then? Smaller than yours but
hoepfully equivalent. It doesn't include your change to the _STA method to
return 0 instead of 9 in the offline case however -- there was no
explanation for that in the changeset comment?

 -- Keir


[-- Attachment #2: 00-dsdt --]
[-- Type: application/octet-stream, Size: 1869 bytes --]

diff -r 3bb163b74673 tools/firmware/hvmloader/acpi/mk_dsdt.c
--- a/tools/firmware/hvmloader/acpi/mk_dsdt.c	Fri Feb 12 09:24:18 2010 +0000
+++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c	Fri Feb 12 10:27:47 2010 +0000
@@ -83,7 +83,7 @@
     indent_level++;
 
     /**** Processor start ****/
-    push_block("Scope", "\\_PR");
+    push_block("Scope", "\\_SB");
 
     /* MADT checksum */
     stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1");
@@ -150,16 +150,16 @@
         /* Extract current CPU's status: 0=offline; 1=online. */
         stmt("And", "Local1, 1, Local2");
         /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
-        push_block("If", "LNotEqual(Local2, \\_PR.PR%02X.FLG)", cpu);
+        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
         /* ...If not, update it and the MADT checksum, and notify OSPM. */
-        stmt("Store", "Local2, \\_PR.PR%02X.FLG", cpu);
+        stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
         push_block("If", "LEqual(Local2, 1)");
         stmt("Notify", "PR%02X, 1", cpu); /* Notify: Device Check */
-        stmt("Subtract", "\\_PR.MSU, 1, \\_PR.MSU"); /* Adjust MADT csum */
+        stmt("Subtract", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         push_block("Else", NULL);
         stmt("Notify", "PR%02X, 3", cpu); /* Notify: Eject Request */
-        stmt("Add", "\\_PR.MSU, 1, \\_PR.MSU"); /* Adjust MADT csum */
+        stmt("Add", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         pop_block();
     }
@@ -171,7 +171,7 @@
     /* Define GPE control method '_L02'. */
     push_block("Scope", "\\_GPE");
     push_block("Method", "_L02");
-    stmt("Return", "\\_PR.PRSC()");
+    stmt("Return", "\\_SB.PRSC()");
     pop_block();
     pop_block();
     /**** Processor end ****/

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

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

RE: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Liu, Jinsong]

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

Keir Fraser wrote:
> On 12/02/2010 10:07, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 
>>> Win2k8 has a similar problem?
>>> 
>>>  -- Keir
>> 
>> Yes. Yunhong test Win2k8, and found in Win2k8 data center version,
>> cpu hotplug successed for _SB definition, but failed for _PR
>> definition. (success mean, after we hot-add vcpu, the device manager
>> will shown new processor, but the task manager has no changes, same
>> as
>> http://www.boche.net/blog/index.php/2009/05/10/vsphere-memory-hot-add-cpu-hot-
>> plug/ .Fail mean even the device manager has no changes). 
> 
> Okay, how about the attached alternative patch then? Smaller than
> yours but hoepfully equivalent. It doesn't include your change to the
> _STA method to return 0 instead of 9 in the offline case however --
> there was no explanation for that in the changeset comment?
> 
>  -- Keir

It's fine to me, except it need change offline flag from 0x09 to 0x00.
Yunhong gave a detailed explanation for the reason, see attached.
We have tested the flag value under some hvm guest:
Define cpu by \_SB + 0X00 flag: linux 2.6.30 OK, linux 2.6.32 OK, Win2K8 OK;
Define cpu by \_PR + 0x00 flag: linux 2.6.30 fail, linux 2.6.32 fail, Win2K8 fail;
Define cpu by \_PR + 0x09 flag: linux 2.6.30 OK (just lucky), linux 2.6.32 fail, Win2K8 fail;

We didn't test \_SB + 0x09 flag, but per my checking kernel code, .30 will lucky wrok but .32 will fail.

Thanks,
Jinsong

[-- Attachment #2: Type: message/rfc822, Size: 5605 bytes --]

From: "Jiang, Yunhong" <yunhong.jiang@intel.com>
To: Keir Fraser <keir.fraser@eu.citrix.com>, "Liu, Jinsong" <jinsong.liu@intel.com>, xen-devel <xen-devel@lists.xensource.com>
Subject: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB
Date: Fri, 12 Feb 2010 10:57:18 +0800
Message-ID: <C8EDE645B81E5141A8C6B2F73FD926511BA551B8DB@shzsmsx501.ccr.corp.intel.com>

Per my checking to the code, the Linux has bug to support cpu hotplug for \_PR defined processor object.

In kernel, the _PR is defined as ACPI_TYPE_LOCAL_SCOPE type (see acpi_gbl_pre_defined_names ) and is skipped without creating corresponding device in acpi_bus_scan (checking at acpi_bus_check_add), while _SB is defined as ACPI_TYPE_DEVICE.

When DEVICE_CHECK notify received for a processor object, acpi_processor_device_add() will be called. When acpi_processor_device_add try to get parent device, it will fail for \_PR situation. _SB is ok because it is defined as ACPI_TYPE_DEVICE. 

static
int acpi_processor_device_add(acpi_handle handle, struct acpi_device **device)
{ 
.......
	acpi_bus_get_device(phandle, &pdev)) {
        return -ENODEV;
    }

This issue is workaround by lucky in current Xen code for 2.6.30 kernel, mainly because of the non-present Processor's status is defined as 0x9 (present, functional, not enabled) in Processor object's _STA function. In 2.6.30 kernel, this means a device is created by acpi module when kernel booting. Later, when the processor hot added, acpi_processor_hotplug_notify() will not call the acpi_processor_device_add(), and call the acpi_processor_start() directly to start the processor.
However, in 2.6.32, changeset 970b04929a68134acca17878b1d93e115e58c12a remove the acpi_processor_start(), remove the acpi_processor_start() at that point, assume the acpi_processor_device_add() will call the acpi_processor_start() in the end. That means in .32 kernel, s the CPU is not started when hot-added. That means the workaround of 0x9 _STA is not working here.

In fact, marked a non-present CPU as 0x9 is really strange. We should of course return 0x0 for non-present processor. However, after change the _STA to return 0x0, it will fail for even 30 kernel for _PR implementation, because then the processor device is not crated by acpi module when kernel booting, and the acpi_processor_device_add() can't get the parent device, the new processor device will not be added when hot-plug.

So in summary, kernel has hotplug bug for Processor defined under _PR, which is workaround in xen by lucky for .30 kernel, but failed in .32 kernel.

I also noticed in Win2k8 data center version, cpu hotplug successed for _SB definition, but failed for _PR definition. (success mean, after we hot-add vcpu, the device manager will shown new processor, but the task manager has no changes, same as http://www.boche.net/blog/index.php/2009/05/10/vsphere-memory-hot-add-cpu-hot-plug/ .Fail mean even the device manager has no changes). But I didn't debug to the w2k8 issue.

However, according to ACPI spec, the _PR is for ACPI1.0 compatible. We have no idea which OS is ACPI 1.0 OS. As HeQing found ACPI 1.0 bugs in Win2K, so we assume W2K is ACPI 1.0. We test shows W2K guest is ok with the _SB definition in our testing. Maybe Win98/WinMe is ACPI 1.0, but we have no image for these OS. But yes, that's a main issue for _SB method and we need more consideration here.

In fact, we have internal argue to choose _PR or _SB method before Jinsong's initial patch sent out. Later _PR method is chosen because of the ACPI 1.0 compatible benifit, and kernel 30 version is ok. (IIRC, .32 kernel is not released at that time). 

--jyh

>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: Friday, February 12, 2010 2:36 AM
>To: Liu, Jinsong; xen-devel
>Cc: Jiang, Yunhong
>Subject: Re: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB
>
>On 11/02/2010 17:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>
>> Vcpu hotplug: Move ACPI processor from \_PR to \_SB
>>
>> Move processor from \_PR to \_SB. ACPI processor can be defined under
>> \_PR or \_SB. However, recently os like linux 2.6.30/32 support cpu hotplug
>> better for \_SB processor object.
>
>What's 'better' about it? I don't want to mess with that kind of thing right
>now unless there's a good reason. In my experience the majority of systems
>still define a \_PR block -- is Linux having some kind of problem with all
>of them?
>
> -- Keir
>
>> Signed-off-by: Jiang, Yunhong   <yunhong.jiang@intel.com>
>>                      Liu, Jinsong     <jinsong.liu@intel.com>
>


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

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

Re: RE: [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

[Keir Fraser]

On 12/02/2010 20:46, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

>> 
>> Okay, how about the attached alternative patch then? Smaller than
>> yours but hoepfully equivalent. It doesn't include your change to the
>> _STA method to return 0 instead of 9 in the offline case however --
>> there was no explanation for that in the changeset comment?
>> 
>>  -- Keir
> 
> It's fine to me, except it need change offline flag from 0x09 to 0x00.

Okay, I'll add that and check it in.

 Thanks,
 Keir