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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2016-01-05 at 16:42 +0000, Andrew Cooper wrote:
>Â
> > 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.

Did you also change Xen to void somewhere in this series, or if not why not
change libxc to use an unsigned int[] if we want them to be consistent?


> > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > ---
> > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> > > CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> > > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > 
> > > 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.

The secondary.c seems ok, it'll get vpathd.

The header, I suppose it depends which way the #include goes from the
generated things, at worst it'd be an #ifdef or a flag to the generator I
guess?

> > > +/* 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.

I'd probably just call this a (sorted) list of dependencies, but ok then ;-
)

> > > +
> > > +ÂÂÂÂÂÂÂÂ# 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

Lovely!

I think I'd say "Expected duplicate values" since I thought the name
3DNOW_ALT was what was duplicated, which made me wonder how it compiled...

If you dropped the appearance of being generic the comment could usefully
give a hint about BPE, afterall this should be a pretty exceptional
occurrence and another if with another comment for the next one doesn't
seem too hateful.

> 
> ~Andrew

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