Xen devel - Apr 2008 - [PATCH 1/9] Add cpu idle pwr mgmt to xen

Add basic acpi C-states based cpu idle power mgmt in xen for x86.

It includes:
 1. hypercall definition for passing ACPI info.
 2. C1/C2 support.
 3. Mwait support, as well as legacy ioport.
 4. Ladder policy from Linux kernel.

A lot of code & ideas came from Linux.

Signed-off-by: Wei Gang <gang.wei@intel.com>


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

On 25/4/08 06:07, "Wei, Gang" <gang.wei@intel.com> wrote:
> Add basic acpi C-states based cpu idle power mgmt in xen for x86.
> 
> It includes:
>  1. hypercall definition for passing ACPI info.
>  2. C1/C2 support.
>  3. Mwait support, as well as legacy ioport.
>  4. Ladder policy from Linux kernel.
> 
> A lot of code & ideas came from Linux.
Comments:
 1. In the idle loop you can just replace default_idle() with (*pm_idle)()
directly.
 2. Do not modify common/keyhandler.c. Instead add an __initcall() in
cpu_idle.c to register your keyhandler. The initcall function and your
keyhandler can both be ''static'' functions.
 3. I don''t like ifdef COMPAT all over new files. Define a separate
compat
shim file, built only for x86_64, which converts compat structures to native
64-bit structures. Then cpu_idle.c need know nothing about compat issues and
will be cleaner for it.
 4. Don''t define placeholders for Px and Tx info in the platform
hypercall
header. They should be introduced when they are actually implemented.

That''s it. I haven''t looked at the other patches yet, but you
can probably
make the above fixes and resubmit just this one patch without affecting the
others. I''ll look at them after this one goes in.

 -- Keir



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

On Friday, April 25, 2008 9:01 PM, Keir Fraser wrote:
> On 25/4/08 06:07, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>> Add basic acpi C-states based cpu idle power mgmt in xen for x86.
> 
> Comments:
>  1. In the idle loop you can just replace default_idle() with
(*pm_idle)()
> directly.
>  2. Do not modify common/keyhandler.c. Instead add an __initcall() in
> cpu_idle.c to register your keyhandler. The initcall function and your
> keyhandler can both be ''static'' functions.
>  3. I don''t like ifdef COMPAT all over new files. Define a
separate
compat
> shim file, built only for x86_64, which converts compat structures to
> native 64-bit structures. Then cpu_idle.c need know nothing about
compat
> issues and will be cleaner for it.
>  4. Don''t define placeholders for Px and Tx info in the platform
hypercall
> header. They should be introduced when they are actually implemented.
> 
> That''s it. I haven''t looked at the other patches yet, but
you can
probably
> make the above fixes and resubmit just this one patch without
affecting
> the others. I''ll look at them after this one goes in.
It is great idea to do it one after one. I will focus on above 4
comments for this patch and come back soon. Thanks for quick comments.
> 
>  -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Revised according to Keir''s comments. Resubmit.

Jimmy

On Friday, April 25, 2008 9:01 PM, Keir Fraser wrote:
> On 25/4/08 06:07, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>> Add basic acpi C-states based cpu idle power mgmt in xen for x86.
>> 
>> It includes:
>>  1. hypercall definition for passing ACPI info.
>>  2. C1/C2 support.
>>  3. Mwait support, as well as legacy ioport.
>>  4. Ladder policy from Linux kernel.
>> 
>> A lot of code & ideas came from Linux.
> 
> Comments:
>  1. In the idle loop you can just replace default_idle() with
(*pm_idle)()
> directly.
>  2. Do not modify common/keyhandler.c. Instead add an __initcall() in
> cpu_idle.c to register your keyhandler. The initcall function and your
> keyhandler can both be ''static'' functions.
>  3. I don''t like ifdef COMPAT all over new files. Define a
separate
compat
> shim file, built only for x86_64, which converts compat structures to
> native 64-bit structures. Then cpu_idle.c need know nothing about
compat
> issues and will be cleaner for it.
>  4. Don''t define placeholders for Px and Tx info in the platform
hypercall
> header. They should be introduced when they are actually implemented.
> 
> That''s it. I haven''t looked at the other patches yet, but
you can
probably
> make the above fixes and resubmit just this one patch without
affecting
> the others. I''ll look at them after this one goes in.
> 
>  -- Keir

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

>>> "Wei, Gang" <gang.wei@intel.com> 26.04.08 11:55
>>>
>Revised according to Keir''s comments. Resubmit.
Some comments regarding the compat guest handling: You cannot
validly set_xen_guest_handle() on space coming from xmalloc. It is
the purpose of the per-vCPU argument translation page to deal with
that (i.e. the translated arguments go into that page, subject to your
own management of how you assign space for the individual (sub-)
hypercall''s arguments).

Further, you shouldn''t manually copy fields, this should be done
through the machine generated macros in xen/include/compat/xlat.h,
which would require you to add the structures needing translation to
xen/include/xlat.lst.

Jan


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

Revising done according to Jan''s comments. Resend.

Jimmy

On Monday, April 28, 2008 5:25 PM, Jan Beulich wrote:
> Some comments regarding the compat guest handling: You cannot
> validly set_xen_guest_handle() on space coming from xmalloc. It is
> the purpose of the per-vCPU argument translation page to deal with
> that (i.e. the translated arguments go into that page, subject to your
> own management of how you assign space for the individual (sub-)
> hypercall''s arguments).
> 
> Further, you shouldn''t manually copy fields, this should be done
> through the machine generated macros in xen/include/compat/xlat.h,
> which would require you to add the structures needing translation to
> xen/include/xlat.lst.
> 
> Jan

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

>>> "Wei, Gang" <gang.wei@intel.com> 30.04.08 05:27
>>>
>Revising done according to Jan''s comments. Resend.
Thanks. Unfortunately you now use a static (but not per-CPU) variable -
while I understand that it is expected that the call is done just once, I
don''t think this is a good thing to do.

Further, xen_processor_csd_t seems to not need translation, so you
could simply add a check for the type to xen/include/xlat.lst and copy
the handle rather than what it points to. This would reduce size
constraints on the xlat area and also simplify the code.

As another suggestion - could you use uint32_t for the bitfield
declarations, making it more obvious that the remaining bits in the
32-bit quantity are reserved? Alternatively, could you use an
explicit padding field after the flags member of struct
xen_processor_power?

Also, I think there''s error checking missing on copy_from_guest*
throughout the patch. And I think I saw non-C89 constructs (loop
variables declared inside for() statements).

Jan


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

On 30/4/08 08:22, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> "Wei, Gang" <gang.wei@intel.com> 30.04.08 05:27
>>>
>> Revising done according to Jan''s comments. Resend.
> 
> Thanks. Unfortunately you now use a static (but not per-CPU) variable -
> while I understand that it is expected that the call is done just once, I
> don''t think this is a good thing to do.
Why is the variable even non-local? Is it just to make the xlat_malloc*()
interfaces simpler? It''s a false simplification if so, and I think
you''d be
better making the variable an explicit parameter to those functions.

Also I agree with Jan regarding non-ISO C usage of loop-header variable
declarations (don''t do it) and also you should check copy_from_guest*()
return values and return -EFAULT where appropriate. His comment regarding
explicit padding or use of uint32_t in your public bitfield also sounds good
to me.

 -- Keir



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

On Wednesday, April 30, 2008 4:54 PM, Keir Fraser wrote:
>> Thanks. Unfortunately you now use a static (but not per-CPU) variable
-
>> while I understand that it is expected that the call is done just
once, I
>> don''t think this is a good thing to do.
> 
> Why is the variable even non-local? Is it just to make the
xlat_malloc*()
> interfaces simpler? It''s a false simplification if so, and I think
you''d
> be better making the variable an explicit parameter to those
functions.

I was trying to make thing simple, and not aware the per_cpu issue for
global variable. Your suggestion sounds good, I will try to follow it.
> 
> Also I agree with Jan regarding non-ISO C usage of loop-header
variable
> declarations (don''t do it) and also you should check
copy_from_guest*()
> return values and return -EFAULT where appropriate. His comment
regarding
> explicit padding or use of uint32_t in your public bitfield also
sounds
> good to me.
Actually, I also agree will Jan regarding the other comments. I am
revising patch for them.

Jimmy

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

One thing kicking me just now is, whether Linux address check
style can be used here by temporarily increasing address limit
in compat logic to bypass relative check in common code? I
didn''t see obvious benefit to reserve a guest virtual addr range
and let each component to manage internal allocation themselves.
Linux style seems simpler and compat logic can just use xmalloc
to create native copy to reduce xlat complexity.

Thanks,
Kevin

>From: Keir Fraser
>Sent: 2008年4月30日 16:54
>
>On 30/4/08 08:22, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>>>>> "Wei, Gang" <gang.wei@intel.com> 30.04.08
05:27 >>>
>>> Revising done according to Jan''s comments. Resend.
>> 
>> Thanks. Unfortunately you now use a static (but not per-CPU) 
>variable -
>> while I understand that it is expected that the call is done 
>just once, I
>> don''t think this is a good thing to do.
>
>Why is the variable even non-local? Is it just to make the 
>xlat_malloc*()
>interfaces simpler? It''s a false simplification if so, and I 
>think you''d be
>better making the variable an explicit parameter to those functions.
>
>Also I agree with Jan regarding non-ISO C usage of loop-header variable
>declarations (don''t do it) and also you should check
copy_from_guest*()
>return values and return -EFAULT where appropriate. His 
>comment regarding
>explicit padding or use of uint32_t in your public bitfield 
>also sounds good
>to me.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>From: Tian, Kevin
>Sent: 2008年4月30日 17:13
>To: Keir Fraser; Jan Beulich; Wei, Gang
>Cc: xen-devel@lists.xensource.com
>Subject: RE: [Xen-devel] [PATCH 1/9] Add cpu idle pwr mgmt to xen
>
>One thing kicking me just now is, whether Linux address check
>style can be used here by temporarily increasing address limit
>in compat logic to bypass relative check in common code? I
>didn''t see obvious benefit to reserve a guest virtual addr range
>and let each component to manage internal allocation themselves.
>Linux style seems simpler and compat logic can just use xmalloc
I really meant local variable OR xmalloc here...
>to create native copy to reduce xlat complexity.
>
>Thanks,
>Kevin
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Tian, Kevin" <kevin.tian@intel.com> 30.04.08 11:12
>>>
>One thing kicking me just now is, whether Linux address check
>style can be used here by temporarily increasing address limit
>in compat logic to bypass relative check in common code? I
>didn''t see obvious benefit to reserve a guest virtual addr range
>and let each component to manage internal allocation themselves.
>Linux style seems simpler and compat logic can just use xmalloc
>to create native copy to reduce xlat complexity.
I intentionally did not go that route when I first wrote these translation
routines. For one, you wouldn''t be able to partly copy things (as I
suggested as an improvement here), since the validity checks would
apply to all or nothing during an individual hypercall (and a bad 64-bit
field representing a pointer might then slip through). Secondly, the
static pre-allocation used currently also avoids spurious failures of
hypercalls (there may be deterministic failures if the combined set
of indirect hypercall arguments exceeds the pre-allocation size.

Jan


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

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: 2008年4月30日 17:35
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 30.04.08
11:12 >>>
>>One thing kicking me just now is, whether Linux address check
>>style can be used here by temporarily increasing address limit
>>in compat logic to bypass relative check in common code? I
>>didn''t see obvious benefit to reserve a guest virtual addr
range
>>and let each component to manage internal allocation themselves.
>>Linux style seems simpler and compat logic can just use xmalloc
>>to create native copy to reduce xlat complexity.
>
>I intentionally did not go that route when I first wrote these 
>translation
>routines. For one, you wouldn''t be able to partly copy things (as I
>suggested as an improvement here), since the validity checks would
>apply to all or nothing during an individual hypercall (and a 
>bad 64-bit
>field representing a pointer might then slip through). Secondly, the
What do you mean by partly copying things? For a 32-on-64 guest,
all pointers from guest are 32-bit and compat_handler_okay already
ensures compat pointers validity. Only native structure may have 
64-bit pointer field, which is checked by common guest_handle_okay
if from a 64bit guest, or is trusted by increasing addr limitation if
from compat layer...
>static pre-allocation used currently also avoids spurious failures of
>hypercalls (there may be deterministic failures if the combined set
>of indirect hypercall arguments exceeds the pre-allocation size.
That''s also the limitation of current approach by pre-defined size,
which
is not scalable if 2nd level pointer are variable decided by some count
field.

Thanks,
Kevin

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

On 30/4/08 10:42, "Tian, Kevin" <kevin.tian@intel.com> wrote:
> What do you mean by partly copying things? For a 32-on-64 guest,
> all pointers from guest are 32-bit and compat_handler_okay already
> ensures compat pointers validity. Only native structure may have
> 64-bit pointer field, which is checked by common guest_handle_okay
> if from a 64bit guest, or is trusted by increasing addr limitation if
> from compat layer...
Yes, I don''t think we do partial copying anywhere right now. If we did,
we
could apply guest_handle_okay() checks explicitly before removing the
addr-space limitation.
>> static pre-allocation used currently also avoids spurious failures of
>> hypercalls (there may be deterministic failures if the combined set
>> of indirect hypercall arguments exceeds the pre-allocation size.
> 
> That''s also the limitation of current approach by pre-defined
size, which
> is not scalable if 2nd level pointer are variable decided by some count
> field.
Also the approaches are not mutually exclusive. We can still have a per-vcpu
pre-alloc''ed page for most hypercalls, and allow dynamic allocation for
hypercalls which require more space and which then have to tolerate ENOMEM
failure. The pre-alloc''ed pages would no longer require to be mapped in
a
special place.

On the other hand, I don''t think we have any hypercall right now where
4kB
is likely to be too little space, and where the hypercall cannot be
sub-divided into smaller chunks by the compat shim.

*But* having a way to flag that arguments have been copied would also be
useful for HVM compat shims too. We already have such a flag
(guest_handles_in_xen_space) there, so we would increase commonality. This
probably means we will go down this route for PV guests too when we merge
some of the compat shim mechanisms for PV and HVM guests.

 -- Keir



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

>>> "Tian, Kevin" <kevin.tian@intel.com> 30.04.08 11:42
>>>
>What do you mean by partly copying things? For a 32-on-64 guest,
>all pointers from guest are 32-bit and compat_handler_okay already
>ensures compat pointers validity. Only native structure may have 
>64-bit pointer field, which is checked by common guest_handle_okay
>if from a 64bit guest, or is trusted by increasing addr limitation if
>from compat layer...
VCPUOP_register_runstate_memory_area is an example of this.

Jan


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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 30.04.08 12:00
>>>
>Yes, I don''t think we do partial copying anywhere right now. If we
did, we
>could apply guest_handle_okay() checks explicitly before removing the
>addr-space limitation.
XENMEM_set_memory_map is one example where we do (valid here
because the E820 layout is identical for 32- and 64-bits).
>On the other hand, I don''t think we have any hypercall right now
where 4kB
>is likely to be too little space, and where the hypercall cannot be
>sub-divided into smaller chunks by the compat shim.
XENMEM_exchange and GNTTABOP_setup_table are examples for this.

Jan


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

Okay, it''s fair to say that any changes we make need careful checking
of all
existing compat shims then. :-) This is all a bit orthogonal to the issues
in the Cx patchset.

 -- Keir

On 30/4/08 11:25, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> Keir Fraser <keir.fraser@eu.citrix.com> 30.04.08 12:00
>>>
>> Yes, I don''t think we do partial copying anywhere right now.
If we did, we
>> could apply guest_handle_okay() checks explicitly before removing the
>> addr-space limitation.
> 
> XENMEM_set_memory_map is one example where we do (valid here
> because the E820 layout is identical for 32- and 64-bits).
> 
>> On the other hand, I don''t think we have any hypercall right
now where 4kB
>> is likely to be too little space, and where the hypercall cannot be
>> sub-divided into smaller chunks by the compat shim.
> 
> XENMEM_exchange and GNTTABOP_setup_table are examples for this.
> 
> Jan
> 


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

Revised according to below comments. Resend.

Jimmy

On Wednesday, April 30, 2008 3:22 PM, Jan Beulich wrote:
> Thanks. Unfortunately you now use a static (but not per-CPU) variable
-
> while I understand that it is expected that the call is done just
once, I
> don''t think this is a good thing to do.
> 
> Further, xen_processor_csd_t seems to not need translation, so you
> could simply add a check for the type to xen/include/xlat.lst and copy
> the handle rather than what it points to. This would reduce size
> constraints on the xlat area and also simplify the code.
> 
> As another suggestion - could you use uint32_t for the bitfield
> declarations, making it more obvious that the remaining bits in the
> 32-bit quantity are reserved? Alternatively, could you use an
> explicit padding field after the flags member of struct
> xen_processor_power?
> 
> Also, I think there''s error checking missing on copy_from_guest*
> throughout the patch. And I think I saw non-C89 constructs (loop
> variables declared inside for() statements).
> 
> Jan

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

Revised according to below comments. Resend.

Jimmy

On Wednesday, April 30, 2008 3:22 PM, Jan Beulich wrote:
> Thanks. Unfortunately you now use a static (but not per-CPU) variable
-
> while I understand that it is expected that the call is done just
once, I
> don''t think this is a good thing to do.
> 
> Further, xen_processor_csd_t seems to not need translation, so you
> could simply add a check for the type to xen/include/xlat.lst and copy
> the handle rather than what it points to. This would reduce size
> constraints on the xlat area and also simplify the code.
> 
> As another suggestion - could you use uint32_t for the bitfield
> declarations, making it more obvious that the remaining bits in the
> 32-bit quantity are reserved? Alternatively, could you use an
> explicit padding field after the flags member of struct
> xen_processor_power?
> 
> Also, I think there''s error checking missing on copy_from_guest*
> throughout the patch. And I think I saw non-C89 constructs (loop
> variables declared inside for() statements).
> 
> Jan

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

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: 2008年4月30日 18:25
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 30.04.08
11:42 >>>
>>What do you mean by partly copying things? For a 32-on-64 guest,
>>all pointers from guest are 32-bit and compat_handler_okay already
>>ensures compat pointers validity. Only native structure may have 
>>64-bit pointer field, which is checked by common guest_handle_okay
>>if from a 64bit guest, or is trusted by increasing addr limitation if
>>from compat layer...
>
>VCPUOP_register_runstate_memory_area is an example of this.
>
Thanks for pointing out. However I still didn''t understand why this 
becomes the benefit of the existing approach. For a normal parameter
conversion, the steps can be:

a) check pointer validity upon compat address limitation
b) allocate native structure with content translated from compat version
c) gear to native handler which checks native address limitation
d) back update compat structure if possible

Existing approach allocates native structure in guest address space
at step b) to bypass address check in step c), while my suggestion
is to allocate native version in Xen space by temporarily improving
address limitation at step b). You can see in either approach where
all necessary checks at step a) have to be done correctly before steping
next. For example, where partly copy applies can always be achieved
even when rest part is copied into Xen space (mixed with guest handle
but validated at step a)). Also 64bit pointer has to be checked at step a)
before improving address limitation.

Well, I''m not against existing approach since I didn''t find
obvious cons
to not use it. :-) As I said earlier, the intent is to get more backgrounds
and make this compat slim clearer to me. BTW, is it possible to let
guest register such compat page within its own address space? This
can release Xen overhead from managing this extra range...

Thanks,
Kevin

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