Re: [Xen-devel] [PATCH 3/7] x86: extract macros to x86-defns.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.17 at 15:17, <wei.liu2@xxxxxxxxxx> wrote:
> On Mon, Jan 30, 2017 at 07:14:23AM -0700, Jan Beulich wrote:
>> >>> On 30.01.17 at 13:46, <wei.liu2@xxxxxxxxxx> wrote:
>> > On Mon, Jan 30, 2017 at 12:32:27PM +0000, Wei Liu wrote:
>> >> On Thu, Jan 26, 2017 at 03:55:27AM -0700, Jan Beulich wrote:
>> >> > >>> On 25.01.17 at 16:44, <wei.liu2@xxxxxxxxxx> wrote:
>> >> > > ... so that they can be used by userspace x86 instruction emulator 
>> >> > > test
>> >> > > program and fuzzer as well.
>> >> > 
>> >> > Ah, here we go. This should be patch 2 then imo.
>> >> > 
>> >> > > --- /dev/null
>> >> > > +++ b/xen/include/asm-x86/x86-defns.h
>> >> > > @@ -0,0 +1,125 @@
>> >> > > +#ifndef __XEN_X86_DEFNS_H__
>> >> > > +#define __XEN_X86_DEFNS_H__
>> >> > > +
>> >> > > +/*
>> >> > > + * CPU vendor IDs
>> >> > > + */
>> >> > > +#define X86_VENDOR_INTEL 0
>> >> > > +#define X86_VENDOR_AMD 1
>> >> > > +#define X86_VENDOR_CENTAUR 2
>> >> > > +#define X86_VENDOR_NUM 3
>> >> > > +#define X86_VENDOR_UNKNOWN 0xff
>> >> > 
>> >> > Let's strictly separate things: These aren't architectural definitions,
>> >> > so should - if we really mean to share them - go into another header.
>> >> > I'm not sure though whether sharing these is all that useful.
>> >> 
>> >> Actually I don't think these are needed.
>> >> 
>> >> I moved them because there were duplicates in x86emul/test. But after
>> >> having a closer look those duplicates are not used.
>> > 
>> > Spoke too soon. I only grepped for X86_VENDOR_INTEL and came to that
>> > conclusion. Actually X86_VENDOR_AMD is used in
>> > x86_emualte/x86_emualte.c and X86_VENDOR_UNKNOWN in test_x86_emulate.c.
>> > 
>> > I propose we move these to x86-vendors.h. What do you think?
>> 
>> Well, if you really think moving them is useful, then the header name
>> would be fine. But as said, I don't really see the point.
> 
> Yes, I think that's going to be useful because that allows emulator
> compiles in both xen and userspace harness. The other route is to
> disentangle some more code to achieve the same effect. But since you
> don't object to the proposed approach, I would avoid the more
> complicated route.

Just to clarify - the main reason for me not really liking the idea is
that (other than the architecture defines) the X86_VENDOR_*
values have no need to be in sync (regarding their values) in
hypervisor and test harness. All we need is for constants of that
name (with whatever values) to be in scope. And that's being
achieved by current code already.

Jan


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