[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] x86/vm_event: Allow returning i-cache for emulation
>>> On 13.09.16 at 20:12, <tamas.lengyel@xxxxxxxxxxxx> wrote: > When emulating instructions the emulator maintains a small i-cache fetched > from the guest memory. This patch extends the vm_event interface to allow > returning this i-cache via the vm_event response instead. I guess I'm sightly confused: Isn't the purpose to have the monitoring app _write_ to the cache maintained in Xen? Or else, who is "emulator" here? If that's the app, then I think subject and description for hypervisor patches would better be written taking the perspective of the hypervisor, especially when using potentially ambiguous terms. > Note: This patch only has been compile-tested. This certainly needs to change before this can be committed. > @@ -1793,7 +1793,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) && curr->arch.vm_event ) > + { > + 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); > + } > + else if ( !vio->mmio_insn_bytes ) > { > hvmemul_ctxt->insn_buf_bytes = > hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?: > @@ -1944,11 +1951,19 @@ 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: > + case EMUL_KIND_SET_CONTEXT_DATA: > + ctx.set_context_data = 1; > + rc = hvm_emulate_one(&ctx); > + break; > + case EMUL_KIND_SET_CONTEXT_INSN: > + ctx.set_context_insn = 1; > rc = hvm_emulate_one(&ctx); > + break; > + case EMUL_KIND_NORMAL: > + rc = hvm_emulate_one(&ctx); > + break; One of the former two surely can be made fall through into the latter? > + default: > + return; Why don't you retain the previous default handling? > --- a/xen/include/asm-x86/hvm/emulate.h > +++ b/xen/include/asm-x86/hvm/emulate.h > @@ -34,20 +34,22 @@ struct hvm_emulate_ctxt { > > uint32_t intr_shadow; > > - bool_t set_context; > + bool_t set_context_data; > + bool_t set_context_insn; As you have been told by others on patch 1 already - please take the opportunity to switch to plain bool. > --- a/xen/include/asm-x86/vm_event.h > +++ b/xen/include/asm-x86/vm_event.h > @@ -27,7 +27,8 @@ > */ > struct arch_vm_event { > uint32_t emulate_flags; > - struct vm_event_emul_read_data emul_read_data; > + struct vm_event_emul_read_data emul_read; > + struct vm_event_emul_insn_data emul_insn; Why don't these get put in a union, when ... > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -97,6 +97,13 @@ > * Requires the vCPU to be paused already (synchronous events only). > */ > #define VM_EVENT_FLAG_SET_REGISTERS (1 << 8) > +/* > + * Instruction cache is being sent back to the hypervisor in the event > response > + * to be used by the emulator. This flag is only useful when combined with > + * VM_EVENT_FLAG_EMULATE and is incompatible with also setting > + * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA. > + */ > +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9) ... place these restrictions and use a union in in the public header? > @@ -265,6 +272,10 @@ struct vm_event_emul_read_data { > uint8_t data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)]; > }; > > +struct vm_event_emul_insn_data { > + uint8_t data[16]; /* Has to be completely filled */ > +}; Any reason for the 16 (rather than the architectural 15) here? Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |