>>> On 15.09.16 at 18:51, <tamas.leng...@zentific.com> wrote:
> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt
> *hvmemul_ctxt,
> pfec |= PFEC_user_mode;
>
> hvmemul_ctxt->insn_buf_eip = regs->eip;
> - if ( !vio->mmio_insn_bytes )
> +
> + if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
> + {
> + BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
> + sizeof(curr->arch.vm_event->emul.insn));
This should quite clearly be !=, and I think it builds only because you
use the wrong operand in the first sizeof().
> + hvmemul_ctxt->insn_buf_bytes =
> sizeof(curr->arch.vm_event->emul.insn);
> + memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
> + hvmemul_ctxt->insn_buf_bytes);
This memcpy()s between dissimilar types. Please omit the & and
properly add .data on the second argument (and this .data
addition should then also be mirrored in the BUILD_BUG_ON()).
> + }
> + else if ( !vio->mmio_insn_bytes )
And then - I'm sorry for not having thought of this before - I think
this would better not live here, or have an effect more explicitly
only when coming here through hvm_emulate_one_vm_event().
Since the former seems impractical, I think giving _hvm_emulate_one()
one or two extra parameters would be the most straightforward
approach.
> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind,
> unsigned int trapnr,
> case EMUL_KIND_NOWRITE:
> rc = hvm_emulate_one_no_write(&ctx);
> break;
> - case EMUL_KIND_SET_CONTEXT:
> - ctx.set_context = 1;
> - /* Intentional fall-through. */
> default:
> + ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> + ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
> rc = hvm_emulate_one(&ctx);
> + break;
> }
Thanks - this is a lot better readable now!
> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
> hvmemul_ctxt->ctxt.force_writeback = 1;
> hvmemul_ctxt->seg_reg_accessed = 0;
> hvmemul_ctxt->seg_reg_dirty = 0;
> - hvmemul_ctxt->set_context = 0;
> + hvmemul_ctxt->set_context_data = 0;
> + hvmemul_ctxt->set_context_insn = 0;
I think you can almost guess the comment here and on the declaration
of these fields: Please use false here and plain bool there.
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>
> if ( v->arch.vm_event->emulate_flags &
> VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> - kind = EMUL_KIND_SET_CONTEXT;
> + kind = EMUL_KIND_SET_CONTEXT_DATA;
> else if ( v->arch.vm_event->emulate_flags &
> VM_EVENT_FLAG_EMULATE_NOWRITE )
> kind = EMUL_KIND_NOWRITE;
> + else if ( v->arch.vm_event->emulate_flags &
> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> + kind = EMUL_KIND_SET_CONTEXT_INSN;
The header talking about incompatibilities between these flags -
wouldn't it be a good idea to actually -EINVAL such combinations?
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -57,6 +57,7 @@
> #include <asm/altp2m.h>
> #include <asm/event.h>
> #include <asm/monitor.h>
> +#include <asm/vm_event.h>
> #include <public/arch-x86/cpuid.h>
I have to admit that looking through the header changes you do I
can't figure why this adjustment is necessary.
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v,
> vm_event_response_t *rsp)
> if ( p2m_mem_access_emulate_check(v, rsp) )
> {
> if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> - v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> + v->arch.vm_event->emul.read = rsp->data.emul.read;
>
> v->arch.vm_event->emulate_flags = rsp->flags;
> }
> break;
> + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> + {
> + v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> + v->arch.vm_event->emulate_flags = rsp->flags;
> + }
> + break;
> default:
Blank lines between non-fall-through case statements please (part
of me thinks I've said so before).
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> * In some cases the response type needs extra handling, so here
> * we call the appropriate handlers.
> */
> -
> /* Check flags which apply only when the vCPU is paused */
Stray change.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
