Xen devel - Feb 2010 - [PATCH 1/2] Vcpu hotplug: Move ACPI processor from \_PR to \_SB

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>

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

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>


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

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>
>

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

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



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

>-----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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

>-----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
>

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

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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

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

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

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



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