From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH RFC V7 1/5] xen: Emulate with no writes Date: Tue, 26 Aug 2014 17:45:29 +0300 Message-ID: <53FC9D89.8010706@bitdefender.com> References: <1407943689-9249-1-git-send-email-rcojocaru@bitdefender.com> <53FCAE48020000780002DA36@mail.emea.novell.com> <53FC934D.1000901@bitdefender.com> <53FCB38C020000780002DA90@mail.emea.novell.com> <53FC9A09.9090809@bitdefender.com> <53FCB89A020000780002DB05@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FCB89A020000780002DB05@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 08/26/2014 05:40 PM, Jan Beulich wrote: >>>> On 26.08.14 at 16:30, wrote: >> On 08/26/2014 05:19 PM, Jan Beulich wrote: >>>>>> On 26.08.14 at 16:01, wrote: >>>> On 08/26/2014 04:56 PM, Jan Beulich wrote: >>>>>>>> On 13.08.14 at 17:28, wrote: >>>>>> +void hvm_emulate_one_full(bool_t nowrite, unsigned int trapnr, >>>>>> + unsigned int errcode) >>>>>> +{ >>>>>> + struct hvm_emulate_ctxt ctx = {{ 0 }}; >>>>>> + int rc; >>>>>> + >>>>>> + hvm_emulate_prepare(&ctx, guest_cpu_user_regs()); >>>>>> + >>>>>> + if ( nowrite ) >>>>>> + rc = hvm_emulate_one_no_write(&ctx); >>>>>> + else >>>>>> + rc = hvm_emulate_one(&ctx); >>>>>> + >>>>>> + 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]); >>>>>> + hvm_inject_hw_exception(trapnr, errcode); >>>>>> + break; >>>>>> + case X86EMUL_EXCEPTION: >>>>>> + if ( ctx.exn_pending ) >>>>>> + hvm_inject_hw_exception(ctx.exn_vector, ctx.exn_error_code); >>>>>> + break; >>>>> >>>>> Shouldn't you act on X86EMUL_RETRY here? Or at least not fall through >>>>> to the writeback below? >>>> >>>> Thanks for the review, I did initially loop around hvm_emulate_one() >>>> until rc != X86EMUL_RETRY, but I've been told that that might block >>>> against time calibration rendezvous points. >>> >>> In any event it strikes me as odd that you ignore that state >>> altogether rather than propagating it back up, so that someone >>> in suitable position to do the retry can invoke it. >> >> Since it's being called in the context of handling a mem_event response, >> the X86EMUL_RETRY case would lead to a retry anyway (since we couldn't >> emulate the current instruction, and we haven't lifted the page access >> restrictions). So if we've failed to somehow modify the guest's EIP, the >> instruction will hit the page again, cause a new mem_event and a new >> attempt to emulate it - so that would seem to fit with the spirit of >> X86EMUL_RETRY. > > Makes sense. Please add a brief comment to this effect when you > add this specific case (bailing without writeback). One thing to > consider though is which function you're in: Based on its name it > has no connection to the specific mem-access use, and hence - with > the behavior you intend to have here not being generically usable - > renaming the function may be a good idea. Will do, thank you very much for your comments! Thanks, Razvan Cojocaru