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

From: Ian Campbell <ian.campbell@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH RFC 15/31] x86: Generate deep dependencies of x86 features
Date: Tue, 5 Jan 2016 17:19:04 +0000	[thread overview]
Message-ID: <1452014344.16082.2.camel@citrix.com> (raw)
In-Reply-To: <568BF8AE.6040609@citrix.com>

On Tue, 2016-01-05 at 17:09 +0000, Andrew Cooper wrote:
> On 05/01/16 16:54, Ian Campbell wrote:
> > 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?
> 
> Xen already uses void *, so the code using __set_bit() can use its
> preferred underlying types for bitmaps.

Oh sorry, you were talking about the actual declared variables when you
said int, not the functions which operate on them.

> libxc bitmaps are unsigned long *, but the featureset abi specifies
> uint32_t *.  IIRC several bits of libxc code uses deliberate typecasting
> to use xc_bitops.h.  void * is definitely the correct solution all
> around.

Right.
> 
> > 
> > > > > +
> > > > > +        # 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.
> 
> Now I think about it, it is probably better to drop 3DNOW_ALT and just
> use PBE in the AMD bugfix, beside the comment.
> 
> That way, we take a "strictly no duplicate" policy.

That sounds plausible.

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