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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-01-05 at 17:13 +0000, Andrew Cooper wrote:
> 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().

Which ones of these are expected to be consumed by applications/utilities
using libxc as well as shared between Xen and libxc?

You mention *_featuremask below.

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

OK, so those which are used by this tool are therefore being added to the
set of exported libxenctrl symbols and are not namespaced correctly.

Also, it seems wrong for something in tools/misc to be delving into
xen/blah and including "cpuid-private.h" (or if it does then it should
build it's own cpuid.o, but I don't like that either). This is akin to the
various inclusions of x?_private.h which we are trying to remove.

If these symbols are to be consumed outside of libxc and Xen by apps
linking against libxc then they need to be prototyped in xenctrl.h, I
think, or the header needs to become public itself.

How bad does providing xc_get_*_featuremask sound? How to cleanly expose
the size of the various arrays consistently is the first annoying niggle
which I thought of.

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

Indeed.

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

Seems nice to me, but really I don't mind as long it is clear how one
drives it.

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

Sure, but it needs to be possible to figure out how to drive the non-main
uses too, which it currently isn't.

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

Ian.

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