Re: [Xen-devel] [PATCH 2/3] x86/idle: update to include further package/core residency MSRs

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-03-05 at 10:37 +0000, Jan Beulich wrote:
> With the number of these growing it becomes increasingly desirable to
> not repeatedly alter the sysctl interface to accommodate them. Replace
> the explicit listing of numbered states by arrays,

I don't have much of an opinion on the hypercall interface, so I'm just
taking that as a given and looking at the tools side accordingly.

> unused fields of which will remain untouched by the hypercall.

Are you supposed to initialise them to some known sentinal or are the
valid entries identified somewhere else (sorry, don't know much about
x86 pm).

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- 2014-02-13.orig/tools/libxc/xc_pm.c       2014-03-04 17:43:06.000000000 
> +0100
> +++ 2014-02-13/tools/libxc/xc_pm.c    2014-03-05 08:54:58.000000000 +0100
> @@ -123,46 +123,90 @@ int xc_pm_get_max_cx(xc_interface *xch, 
>  
>  int xc_pm_get_cxstat(xc_interface *xch, int cpuid, struct xc_cx_stat *cxpt)
> [...]
> +int xc_pm_get_cx_stat(xc_interface *xch, int cpuid, struct xc_cx_stat_v2 
> *cxpt)

That is an incredibly subtle difference in the naming!

If v1 is considered deprecated then lets just get rid of it. There's
only one caller which you update in the next patch. I'd be perfectly
happy to have those collapsed and for the old interface to go away
immediately (or do it in patch 4/3 if you prefer).

> +{
> +    DECLARE_SYSCTL;
> +    DECLARE_NAMED_HYPERCALL_BOUNCE(triggers, cxpt->triggers,
> +                                   cxpt->nr * sizeof(*cxpt->triggers),
> +                                   XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +    DECLARE_NAMED_HYPERCALL_BOUNCE(residencies, cxpt->residencies,
> +                                   cxpt->nr * sizeof(*cxpt->residencies),
> +                                   XC_HYPERCALL_BUFFER_BOUNCE_OUT);

The original had these as BOUNCE_BOTH. If that was wrong  and this
change was therefore intentional (which I suspect is the case) please
note it in the commit message.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel