Re: [PATCH for-next v3 01/22] x86/traps: move privilege instruction emulation code

From: Wei Liu <wei.liu2@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH for-next v3 01/22] x86/traps: move privilege instruction emulation code
Date: Tue, 30 May 2017 18:27:44 +0100	[thread overview]
Message-ID: <20170530172744.scqefvuebkum3m5r@citrix.com> (raw)
In-Reply-To: <592C56DF020000780015D575@prv-mh.provo.novell.com>

On Mon, May 29, 2017 at 09:14:07AM -0600, Jan Beulich wrote:
> >>> On 18.05.17 at 19:28, <wei.liu2@citrix.com> wrote:
> > From 58df816b937dc7a3598de01f053a6030e631057e Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Thu, 18 May 2017 16:18:56 +0100
> > Subject: [PATCH] x86/traps: move privilege instruction emulation code
> 
> privileged
> 
> > Move relevant code to pv/emulate.c. Export emulate_privileged_op in
> > pv/traps.h.
> 
> A name of "emulate.c" sounds like a container for all sorts of cruft.
> I'd prefer if we could use the opportunity of this re-org to see about
> not having overly large files. Therefore e.g. "emul-priv.c" or
> "priv-emul.c" or some such?
> 

I think this is a fine idea.

> > Note that read_descriptor is duplicated in emulate.c. The duplication
> > will be gone once all emulation code is moved.
> 
> That's not very desirable; we can only hope to have things
> committed together then? However, together with the naming
> issue above quite likely the function will want to become non-
> static anyway, so perhaps this should then be done right away
> instead of cloning it.
> 

Yes, I can do that.

> > Also move gpr_switch.S to pv/ because the code in that file is only
> > used by privilege instruction emulation.
> > 
> > No functional change.
> 
> For this size of a change this is too weak a statement for my taste:
> I don't really mean to review the 1.5k of lines you move, so I'd hope
> for a statement clarifying that perhaps other than formatting the
> code is being moved unchanged.
> 
> > +int emulate_privileged_op(struct cpu_user_regs *regs)
> 
> Does this perhaps want to gain a pv_ prefix?

OK.

> 
> > +#ifdef CONFIG_PV
> > +
> > +#include <public/xen.h>
> > +
> > +int emulate_privileged_op(struct cpu_user_regs *regs);
> > +
> > +#else  /* !CONFIG_PV */
> > +
> > +#include <xen/errno.h>
> > +
> > +int emulate_privileged_op(struct cpu_user_regs *regs) { return -EOPNOTSUPP; }
> 
> The function does not return -E... values.
> 

Right. Not sure why I missed this one. Later patches used 0 to match the
non-stub functions.

Wei.

> Jan
> 

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