[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>>> On 18.05.17 at 19:28, <wei.liu2@xxxxxxxxxx> wrote:
> From 58df816b937dc7a3598de01f053a6030e631057e Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> Date: Thu, 18 May 2017 16:18:56 +0100
> Subject: [PATCH] x86/traps: move privilege instruction emulation code


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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.