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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.14 at 15:11, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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).

The best thing for the caller would be to zero the whole buffer.
But other known out of range values (like all ones) would do to.
In the end it's up to the caller to pre-fill the array suitably for it
to recognize valid fields.

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

I actually meant to raise the question of deprecation vs deletion/
replacement, but then forgot. So are you saying then that deleting/
replacing a libxc interface is not an issue, i.e. there's no need for
API compatibility? If so I'd indeed prefer to merge this and the 3rd
patch and simply adjust the structure definition and function
implementation, without any v2 or other subtleties.

>> +{
>> +    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.

Added for v2 (subject to an answer to the above).

Jan


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