> -----Original Message----- > From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On > Behalf Of Nadav Amit > Sent: Thursday, December 25, 2014 5:55 PM > To: Chen, Tiejun > Cc: Paolo Bonzini; kvm list > Subject: Re: [PATCH 2/8] KVM: x86: pop sreg accesses only 2 bytes > > Tiejun <tiejun.chen@xxxxxxxxx> wrote: > > > On 2014/12/25 8:52, Nadav Amit wrote: > >> Although pop sreg updates RSP according to the operand size, only 2 bytes > are > >> read. The current behavior may result in incorrect #GP or #PF exceptions. > >> > >> Signed-off-by: Nadav Amit <namit@xxxxxxxxxxxxxxxxx> > >> --- > >> arch/x86/kvm/emulate.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > >> index e5a84be..702da5e 100644 > >> --- a/arch/x86/kvm/emulate.c > >> +++ b/arch/x86/kvm/emulate.c > >> @@ -1830,12 +1830,14 @@ static int em_pop_sreg(struct > x86_emulate_ctxt *ctxt) > >> unsigned long selector; > >> int rc; > > > > Looks we just should do similar thing to em_push_sreg(), > > > > unsigned long selector; > > int rc; > > > > + if (ctxt->op_bytes == 4) { > > + rsp_increment(ctxt, -2); > > + ctxt->op_bytes = 2; > > + } > > rc = emulate_pop(ctxt, &selector, ctxt->op_bytes); > > if (rc != X86EMUL_CONTINUE) > > return rc; > > > > Right? > I don't think so. It seems the behaviour of push and pop is a bit different. > For push: "If the source operand is a segment register (16 bits) and the > operand size is 64-bits, a zero-extended value is pushed on the stack; if > the operand size is 32-bits ... all recent Core and Atom processors perform > a 16-bit move, leaving the upper portion of the stack location unmodified." > > Therefore, for push in the case of op_bytes==8 we push zero-extended value. > > For pop the behaviour is not well-documented, but experimentally it appears > only the first two bytes are accessed. I cannot see why it would be > different when opsize is 8, since it is not like the push case, where the > segment register value was zero extended. Let's take 64-bits operand size as an example. When pushing a segment register, it uses zero-extended value, so 8 bytes will be pushed on the stack. When popping it, the current code return the top 8 bytes in the stack, and it only uses the lowest 2 bytes for load_segment_descriptor(). what is the issue here? Thanks, Feng > > If you feel strongly about it, I'll create a unit test. > > Nadav > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
