From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator Date: Fri, 16 Nov 2007 08:41:50 +0200 Message-ID: <473D3BAE.1050207@qumranet.com> References: <200711151532.20558.sheng.yang@intel.com> <473C1C38.4000700@qumranet.com> <200711161028.09501.sheng.yang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Sheng Yang Return-path: In-Reply-To: <200711161028.09501.sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Sheng Yang wrote: > On Thursday 15 November 2007 18:15:20 Avi Kivity wrote: > >> Sheng Yang wrote: >> >>> From 9cd9d5cde7341d5e9de41b1070cea7a98e7d8cc9 Mon Sep 17 00:00:00 2001 >>> From: Sheng Yang >>> Date: Thu, 15 Nov 2007 15:11:58 +0800 >>> Subject: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator >>> >>> For CR2 is unreliable and unavailable in many condition, this patch >>> completely decode memory operand instead of using CR2 in x86 emulator. >>> >> One of my innermost wishes... >> >> >>> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c >>> index aa6c3d8..85a0776 100644 >>> --- a/drivers/kvm/x86.c >>> +++ b/drivers/kvm/x86.c >>> @@ -1293,7 +1293,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu, >>> >>> vcpu->emulate_ctxt.vcpu = vcpu; >>> vcpu->emulate_ctxt.eflags = kvm_x86_ops->get_rflags(vcpu); >>> - vcpu->emulate_ctxt.cr2 = cr2; >>> + vcpu->emulate_ctxt.memop = 0; >>> >> We have c->modrm_ea which can be used for the memory operand. >> > > I don't think using the name modrm_ea is good for explicit encoding, so I add > this. I agree the name isn't good (we already use it for MemAbs decoding, too). We can rename it later. > thBut I am think of is it better to be in decode_cache? > > c-> is the decode cache. Maybe I misunderstood you? >>> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c >>> index c020010..95536a8 100644 >>> --- a/drivers/kvm/x86_emulate.c >>> +++ b/drivers/kvm/x86_emulate.c >>> @@ -880,6 +880,8 @@ done_prefixes: >>> break; >>> } >>> c->src.type = OP_MEM; >>> + ctxt->memop = insn_fetch(u32, c->src.bytes, c->eip); >>> + c->eip -= c->src.bytes; /* keep the page fault ip */ >>> >> I don't understand this. In the cases where the memory operand address >> is encoded in the instruction, we fetch it explicity. When it isn't, >> this is broken. >> > > But we mark implicit encoding instructions as "ImplicitOps", so only explicit > ones should get here. And my former patch deal with the implicit ones, and > modrm_ea has priority to memop, so I think it's OK. > I still don't understand. Which instruction benefits from this change? And shouldn't the be marked MemAbs instead? -- Any sufficiently difficult bug is indistinguishable from a feature. ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/