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

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

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. But I am think of is it better to be in decode_cache?

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

Maybe add  a judgment of modrm_ea here is better.

>
> >  		break;
> >  	case SrcImm:
> >  		c->src.type = OP_IMM;
> > @@ -918,14 +920,18 @@ done_prefixes:
> >  			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
> >  		break;
> >  	case DstMem:
> > +		c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>
> This seems unrelated; needs a separate patch.  Please check that movzx
> and movsx aren't affected by this.

I need this for following insn_fetch(). And movzx/movsx is marked as DstReg, 
so they won't get here.

>
> >  		/*
> >  		 * For instructions with a ModR/M byte, switch to register
> >  		 * access if Mod = 3.
> >  		 */
> > -		if ((c->d & ModRM) && c->modrm_mod == 3)
> > +		if ((c->d & ModRM) && c->modrm_mod == 3) {
> >  			c->dst.type = OP_REG;
> > -		else
> > -			c->dst.type = OP_MEM;
> > +			break;
> > +		}
> > +		c->dst.type = OP_MEM;
> > +		ctxt->memop = insn_fetch(u32, c->dst.bytes, c->eip);
> > +		c->eip -= c->dst.bytes; /* keep the page fault ip */
>
> Ditto.



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