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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jan 30, 2017 at 07:24:57AM -0700, Jan Beulich wrote:
> >>> 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.
> 

That's true. And I understand that.

My idea is that I want to avoid unnecessary duplication to reduce the
distraction when one needs to modify userspace test harness. If there is
already definitions in xen, I will just use that.

Wei.

> Jan
> 

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