Re: [PATCH RFC 12/31] tools: Utility for dealing with featuresets

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/01/16 16:34, Ian Campbell wrote:
> On Tue, 2016-01-05 at 16:14 +0000, Andrew Cooper wrote:
>> On 05/01/16 15:17, Ian Campbell wrote:
>>> On Wed, 2015-12-16 at 21:24 +0000, Andrew Cooper wrote:
>>>> It is able to reports the current featuresets; both the static masks
>>>> and
>>>> dynamic featuresets from Xen, or to decode an arbitrary featureset
>>>> into
>>>> `/proc/cpuinfo` style strings.
>>> More than adding a utility does this not also arrange for a whole bunch
>>> of
>>> new functionality to be compiled into libxc? That's worth mentioning
>>> here.
>> It is almost all just data.  Only a single (trivial) function at
>> present.  But yes - it is worth mentioning.
>>
>>> And doesn't it do so in a non-library namespaced way (e.g.
>>> with calculate_featuresets() and decode_featureset() being exposed by
>>> the
>>> library)?
>> calculate_featuresets() is hypervisor-only code and not included by this
>> change, and decode_featureset() is a local function to the utility.
> Ah, I obviously just got confused.
>
> What are the symbols exported by cpuid.o in a tools build then?

There are two cpuid.o's, which is a source of confusion.

cpuid-private.h is the declaration of the symbols which get shared
between Xen and libxc.

They are: SHARED_1d, known_features[], inverted_features[],
{pv,hvm_shadow,hvm_hap}_featuremask[], struct tagged_featureset,
deep_deps[], nr_deep_deps, deep_dep_features[] and lookup_deep_deps().

>
>>> Granted there's lots of that sort of thing already, but should we
>>> really be
>>> making it worse?
>>>
>>> libelf avoids this by namespacing itself as a quasi-standalone library
>>> in
>>> both the tools and hypervisor contexts.
>> Nothing from the shared .c files is expected to be exposed in the API. 
>> The guts of xc_cpuid_policy() end up using it, but that is an
>> implementation detail.
> Ah, so is the presence of the vpath stuff in libxc here spurious then?

No - this utility needs the generated data (specifically the static
*_featuremask[] bitmaps), so the vpath is needed.

> Since the utility you are adding doesn't actually use anything which it
> provides?
>
> That would explain my confusion. (and makes the namespacing moot I think)

The namespacing is probably not moot, sadly.

>
>
>>> (and then naturally be part of the autogeneration which has been
>>> discussed)
>>>> + option_error:
>>>> +            printf("Usage: %s [ info | detail | <featureset>* ]\n",
>>>> argv[0]);
>>> What format does <featureset> take?
>> : or - delimited 32bit hex strings.
>>
>> Some sample outputs look like:
> Thanks, so one could call
>
> xen-cpuid ffeffbff:fffef7ff:efdbfbff:2469bfff:0000000f:21dcffbb:00000001:00000100:00000001
>
> and expect output like:
> [...]
>>   [00] 0x00000001.edx     fpu vme de pse tsc msr pae mce cx8 apic
>> sysenter mtrr pge mca cmov pat pse36 clflsh acpi mmx fxsr sse sse2
>>   [01] 0x00000001.ecx     sse3 vmx cx16 hyper
>>   [02] 0x80000001.edx     syscall nx lm
>>   [03] 0x80000001.ecx     lahf_lm
>>   [04] 0x0000000d:1.eax 
>>   [05] 0x00000007:0.ebx 
>>   [06] 0x00000007:0.ecx 
>>   [07] 0x80000007.edx   
>>   [08] 0x80000008.ebx   
> (except not w/space mangled by my MUA)?
>
> If one was only interested in an 0x00000001.edx value one could say
>
> xen-cpuid ffeffbff
>
> But what if one wants only e.g. 0x80000001.ecx? Can you say :::2469bfff or
> is it just not possible?

Currently,

[root@idol ~]# xen-cpuid 4
Raw                       00000004
  [00] 0x00000001.edx     de
[root@idol ~]# xen-cpuid 0::2
Raw                       00000000
  [00] 0x00000001.edx   
[root@idol ~]# xen-cpuid 0:0:2
Raw                       00000000:00000000:00000002
  [00] 0x00000001.edx   
  [01] 0x00000001.ecx   
  [02] 0x80000001.edx     vme

But I can easily edit it to permit something like :::2469bfff

>
> I don't think this tools warrants full docs or a manpage or whatever, but
> if you were to put the usage into a function then it could be more easily
> multiline and perhaps include a bit more info, plus then my terribly minor
> gripe about the use of goto which I didn't feel was worth mentioning would
> go away ;-)

Its main use is without any parameters, at which point it dumps the
static and dynamic masks for the currently-running hypervisor.

I thought it might be nice to be able to set difference calculations,
but that got rather complicated.  It is a debug utility after all.

~Andrew