On Mon, Sep 12, 2016 at 8:56 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 09.09.16 at 17:41, <tamas.leng...@zentific.com> wrote:
> > When emulating instructions the emulator maintains a small i-cache
> fetched
> > from the guest memory. Under certain scenarios this memory region may
> contain
> > instructions that a monitor subscriber would prefer to hide, namely
> INT3, and
> > instead would prefer to emulate a different instruction in-place.
> >
> > This patch extends the vm_event interface to allow returning this
> i-cache via
> > the vm_event response.
>
> Please try to explain better what this change is about, perhaps along
> the lines of what George used as description, which you then
> confirmed.
>
Sure.
>
> > @@ -1783,7 +1786,14 @@ 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) )
> > + {
> > + memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_
> data.data,
> > + curr->arch.vm_event->emul_data.size);
> > + hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_
> data.size;
> > + }
> > + else if ( !vio->mmio_insn_bytes )
>
> I'm not sure about this ordering: Do you really mean to allow an
> external entity to override insn bytes e.g. in an MMIO retry, i.e.
> allowing the retried insn to be different from the original one?
>
I'm not sure but there shouldn't be INT3's coming from MMIO, right? So we
would not have a response-opportunity to override it.
>
> And additionally I think you need to deal with the case of the
> supplied data not being a full insn. There shouldn't be any
> fetching from guest memory in that case imo, emulation should
> just fail.
>
So the idea was to allow partial "masking" of the i-cache. For example, if
all I want to hide is the 0xCC then it's enough to return 1 byte in
vm_event, the rest can be (and already is) grabbed from memory.
> > @@ -1927,17 +1937,19 @@ void hvm_mem_access_emulate_one(enum emul_kind
> kind, unsigned int trapnr,
> > struct hvm_emulate_ctxt ctx = {{ 0 }};
> > int rc;
> >
> > + gdprintk(XENLOG_WARNING, "memaccess emulate one called\n");
> > +
> > hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
> >
> > - switch ( kind )
> > - {
> > - case EMUL_KIND_NOWRITE:
> > + if ( kind == EMUL_KIND_NOWRITE )
> > rc = hvm_emulate_one_no_write(&ctx);
> > - break;
> > - case EMUL_KIND_SET_CONTEXT:
> > - ctx.set_context = 1;
> > - /* Intentional fall-through. */
> > - default:
> > + else
> > + {
> > + if ( kind == EMUL_KIND_SET_CONTEXT_DATA )
> > + ctx.set_context_data = 1;
> > + else if ( kind == EMUL_KIND_SET_CONTEXT_INSN )
> > + ctx.set_context_insn = 1;
> > +
> > rc = hvm_emulate_one(&ctx);
> > }
>
> Could you try to retain the switch() statement?
>
Sure.
>
> > --- a/xen/arch/x86/vm_event.c
> > +++ b/xen/arch/x86/vm_event.c
> > @@ -66,6 +66,16 @@ void vm_event_toggle_singlestep(struct domain *d,
> struct vcpu *v)
> > hvm_toggle_singlestep(v);
> > }
> >
> > +void vm_event_interrupt_emulate_check(struct vcpu *v,
> vm_event_response_t *rsp)
> > +{
> > + if ( !!(rsp->flags & VM_EVENT_FLAG_EMULATE) &&
> > + !!(rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA) )
>
> Please avoid !! when not really needed.
>
Ack.
>
> > + {
> > + v->arch.vm_event->emulate_flags = rsp->flags;
> > + v->arch.vm_event->emul_data = rsp->data.emul_data;
> > + }
> > +}
>
> Overall I'm having difficulty associating the function's name with
> what it does.
>
Yea, I'm not too happy with it either. Will move some things around.
>
> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -407,8 +407,11 @@ void vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> > vm_event_register_write_resume(v, &rsp);
> > break;
> >
> > + case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> > + vm_event_interrupt_emulate_check(v, &rsp);
> > + break;
> > +
> > #ifdef CONFIG_HAS_MEM_ACCESS
> > - case VM_EVENT_REASON_MEM_ACCESS:
> > mem_access_resume(v, &rsp);
> > break;
>
> I don't think you meant to remove that line?
>
Oops.
>
> Jan
>
>
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
