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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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?

> 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.

> 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?

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

Jan


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