|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 1/6] xen: Emulate with no writes
On 11/07/14 16:43, Razvan Cojocaru wrote:
> Added support for emulating an instruction with no memory writes.
> Additionally, introduced hvm_emulate_one_full(), which acts upon all
> possible return values from the hvm_emulate_one() functions (RETRY,
> EXCEPTION, UNHANDLEABLE).
>
> Changes since V1:
> - Removed the Linux code that computes the length of an instruction.
> - Unused function parameters are no longer marked.
> - Refactored the code to eliminate redundancy.
> - Made the exception passed on to the guest by hvm_emulate_one_full()
> configurable.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> ---
> xen/arch/x86/hvm/emulate.c | 73
> +++++++++++++++++++++++++++++++++++--
> xen/include/asm-x86/hvm/emulate.h | 5 +++
> 2 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index eac159f..114a77a 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -688,6 +688,17 @@ static int hvmemul_write(
> return X86EMUL_OKAY;
> }
>
> +static int hvmemul_write_dummy(
hvmemul_write_discard() would make its purpose more clear.
> + enum x86_segment seg,
> + unsigned long offset,
> + void *p_data,
> + unsigned int bytes,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + /* discarding the write */
Full stop please.
> + return X86EMUL_OKAY;
> +}
> +
> static int hvmemul_cmpxchg(
> enum x86_segment seg,
> unsigned long offset,
> @@ -1138,8 +1149,8 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
> .invlpg = hvmemul_invlpg
> };
>
> -int hvm_emulate_one(
> - struct hvm_emulate_ctxt *hvmemul_ctxt)
> +static int hvm_emulate_one_with_ops(struct hvm_emulate_ctxt *hvmemul_ctxt,
> + const struct x86_emulate_ops *ops)
You could get away with a renaming this to _hvm_emulate_one() to help
identify that it is basically identical to hvm_emulate_one(), just with
slightly different parameters.
> {
> struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
> struct vcpu *curr = current;
> @@ -1191,7 +1202,7 @@ int hvm_emulate_one(
> vio->mmio_retrying = vio->mmio_retry;
> vio->mmio_retry = 0;
>
> - rc = x86_emulate(&hvmemul_ctxt->ctxt, &hvm_emulate_ops);
> + rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>
> if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> rc = X86EMUL_RETRY;
> @@ -1239,6 +1250,62 @@ int hvm_emulate_one(
> return X86EMUL_OKAY;
> }
>
> +int hvm_emulate_one(
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> + return hvm_emulate_one_with_ops(hvmemul_ctxt, &hvm_emulate_ops);
> +}
> +
> +int hvm_emulate_one_no_write(
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
> + struct x86_emulate_ops local_ops = hvm_emulate_ops;
> + local_ops.write = hvmemul_write_dummy;
This is runtime constant and somwhat expensive to set up for each
function call.
You could make a static const struct x86_emulate_ops
hvm_emulate_ops_no_write and use that each time.
> +
> + return hvm_emulate_one_with_ops(hvmemul_ctxt, &local_ops);
> +}
> +
> +void hvm_emulate_one_full(bool_t nowrite,
> + unsigned int unhandleable_trapnr,
> + int unhandleable_errcode)
> +{
> + struct hvm_emulate_ctxt ctx[1] = {};
This construct looks suspect. What is wrong with
struct hvm_emulate_ctxt ctx = { 0 }; and using &ctx below ?
> + int rc = X86EMUL_RETRY;
> +
> + hvm_emulate_prepare(ctx, guest_cpu_user_regs());
> +
> + while ( rc == X86EMUL_RETRY )
> + {
> + if ( nowrite )
> + rc = hvm_emulate_one_no_write(ctx);
> + else
> + rc = hvm_emulate_one(ctx);
> + }
This however is not appropriate. X86EMUL_RETRY can include "talk to
dom0 and get the paging subsystem to page in part of the VM", which
given some current work-in-progress can be to the other end of a network
connection on the far side of an optimistic migration.
You can't cause a Xen pcpu to spin like this for that length of time.
Anything longer than a second and all other pcpus will block against the
time calibration rendezvous, and longer than 5 will panic on the NMI
watchdog.
> +
> + switch ( rc )
> + {
> + case X86EMUL_UNHANDLEABLE:
> + gdprintk(XENLOG_DEBUG, "Emulation failed @ %04x:%lx: "
> + "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> + hvmemul_get_seg_reg(x86_seg_cs, ctx)->sel,
> + ctx->insn_buf_eip,
> + ctx->insn_buf[0], ctx->insn_buf[1],
> + ctx->insn_buf[2], ctx->insn_buf[3],
> + ctx->insn_buf[4], ctx->insn_buf[5],
> + ctx->insn_buf[6], ctx->insn_buf[7],
> + ctx->insn_buf[8], ctx->insn_buf[9]);
Hmm - I keep meaning to add a new %p custom format to print out hex
buffers like this. It is sadly quite a long way down my todo list.
> + hvm_inject_hw_exception(unhandleable_trapnr, unhandleable_errcode);
> + break;
> + case X86EMUL_EXCEPTION:
> + if ( ctx->exn_pending )
> + hvm_inject_hw_exception(ctx->exn_vector, ctx->exn_error_code);
> + /* fall through */
> + default:
> + hvm_emulate_writeback(ctx);
> + break;
> + }
> +}
> +
> void hvm_emulate_prepare(
> struct hvm_emulate_ctxt *hvmemul_ctxt,
> struct cpu_user_regs *regs)
> diff --git a/xen/include/asm-x86/hvm/emulate.h
> b/xen/include/asm-x86/hvm/emulate.h
> index 00a06cc..d68b485 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -37,6 +37,11 @@ struct hvm_emulate_ctxt {
>
> int hvm_emulate_one(
> struct hvm_emulate_ctxt *hvmemul_ctxt);
> +int hvm_emulate_one_no_write(
> + struct hvm_emulate_ctxt *hvmemul_ctxt);
> +void hvm_emulate_one_full(bool_t nowrite,
> + unsigned int unhandleable_trapnr,
> + int unhandleable_errcode);
Prevailing Xen nomenclature would be 'trapnr' and 'error_code'. The
error code should be unsigned as well.
~Andrew
> void hvm_emulate_prepare(
> struct hvm_emulate_ctxt *hvmemul_ctxt,
> struct cpu_user_regs *regs);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |