Re: [PATCH 2/2] KVM: x86 emulator: Discard CR2 in x86 emulator

[Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Friday 16 November 2007 14:41:50 Avi Kivity wrote:
> 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 <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> 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?

Yes, your are right. I found I made a wrong assumption. 

I will send the modified patch later, thx. 

-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
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/