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

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

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.

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

What actually breaks with the existing definitions?

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

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

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

> +
> +        # Expected duplicate symbols.  Discard
> +        if name in ('3DNOW_ALT', ):
> +            continue

OOI how does this arise?



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