Re: [PATCH RFC 15/31] x86: Generate deep dependencies of x86 features

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

On 05/01/16 16:03, Ian Campbell wrote:
> On Wed, 2015-12-16 at 21:24 +0000, Andrew Cooper wrote:
>> Some features depend on other features.  Working out and maintaining the exact
>> dependency tree is complicated, so it is expressed in script form instead.
>>
>> `gen-feature-deps.py` parses 'xen/include/public/arch-x86/featureset.h' (To
>> obtain some literal names conforming to the API), contains some single-step
>> dependency information, performs some number crunching, and writes autogen.c
>> to make the results of the number crunching available.
> In libxl we prepend a _ to autogenerated file names, may as well do the
> same here I guess.
>
>> In this case, it writes out deep dependency infomarion, to allow featureset
> "information"
>> code to find all eventual features cleared in a dependency chain.
>>
>> To be able to compile for userspace, libxc's bitmap macros are made more
>> generic (to match Xen's), and accept a void * instead of unsigned long *.
> Can we do this in a separate patch please.

Will do (for all points)

>
> IIRC void * arithmetic is a gcc (and presumably clang) extension, which we
> can specify for Xen itself, but I'm not sure about libxc (cf: recent
> discussions about building stuff for Windows).

It isn't void* arithmetic.  Simply taking a void * to facilitate
different underlying types for a bitmap.

>
> What actually breaks with the existing definitions?

Xen uses unsigned int[] for bitmaps, while libxc uses unsigned long[]. 
The compiler objects to the mix of pointers to different sized.

For a bitmap however it really doesn't matter.  At the end of the
pointer is a contiguous block of bits.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> TODO: The generation of autogen.c now means that libxc needs to be compiled
>> after the hypervisor, as the vpath doesn't convey the generation information.
>> I need to find a way to fix this.
> I'd be inclined to do the autogeneration twice with the output being
> outside the shared directory or gaining a (tools|xen)_ prefix.
>
> It's a bit wasteful to do it twice but anything else would add an
> undesirable linkage between tools and hypervisor build I think. I'm certain
> we don't want to commit the generated files.

I will see how much of this I can get into the automatically generated
part.  The problem is that there is a header file and secondary .c file
which are not automatically generated, but needed in both places.  I
definitely do not want to duplicate those.

>
>> diff --git a/xen/arch/x86/cpuid/cpuid-private.h
>> b/xen/arch/x86/cpuid/cpuid-private.h
>> index 1c92ee4..438f5d2 100644
>> --- a/xen/arch/x86/cpuid/cpuid-private.h
>> +++ b/xen/arch/x86/cpuid/cpuid-private.h
>> @@ -64,6 +64,24 @@ extern const uint32_t
>> pv_featuremask[XEN_NR_FEATURESET_ENTRIES];
>>  extern const uint32_t hvm_shadow_featuremask[XEN_NR_FEATURESET_ENTRIES];
>>  extern const uint32_t hvm_hap_featuremask[XEN_NR_FEATURESET_ENTRIES];
>>  
>> +/* A featureset with a tag. */
>> +struct tagged_featureset
>> +{
>> +    uint32_t tag;
>> +    uint32_t fs[XEN_NR_FEATURESET_ENTRIES];
>> +};
>> +
>> +/* Sparse feature matrix identifying all features which depend on a
>> feature. */
>> +extern const struct tagged_featureset deep_deps[];
> This'll be XEN_NR_FEATURESET_ENTRIES^2 entries? How big is that getting
> OOI?

It is a sparse matrix, not an n^2 matrix.  It only has entries for
features which sit in a dependency tree, which is vastly fewer than all
known features.  Currently, it is an array of 9 entries.

>
>> diff --git a/xen/arch/x86/cpuid/gen-feature-deps.py b/xen/arch/x86/cpuid/gen-feature-deps.py
>> new file mode 100755
>> index 0000000..f0ecbba
>> --- /dev/null
>> +++ b/xen/arch/x86/cpuid/gen-feature-deps.py
>> @@ -0,0 +1,152 @@
>> +#!/usr/bin/env python
>> +import sys, os, os.path as path, re
>> +
>> +names = {}
>> +
>> +with open(path.join(os.environ["XEN_ROOT"],
>> +                    "xen/include/public/arch-x86/featureset.h")) as f:
> I'd weakly prefer all the input and output paths to come from the command
> line.
>
> I have a feeling you are planning to redo a bunch of this (or if not then
> Jan was requesting something more structured which may or may not cause big
> changes here), so I'll leave the actual review for now.

Yeah - there have been a number of suggestions, so v2 is going to look
very different.

>
>> +
>> +        # Expected duplicate symbols.  Discard
>> +        if name in ('3DNOW_ALT', ):
>> +            continue
> OOI how does this arise?

Bug in older AMD processors.  See the first and final hunks of c/s d8ba3a9

~Andrew