From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [PATCH 2/8] KVM: x86: pop sreg accesses only 2 bytes
Date: Fri, 26 Dec 2014 09:54:30 +0800
Message-ID: <549CBFD6.5000604@intel.com>
References: <1419468743-23732-1-git-send-email-namit@cs.technion.ac.il> <1419468743-23732-3-git-send-email-namit@cs.technion.ac.il> <549BD49D.3060200@intel.com> <8941A675-6DA0-4815-AAB3-9D9F6DC71270@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252;
	format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm list <kvm@vger.kernel.org>
To: Nadav Amit <nadav.amit@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga11.intel.com ([192.55.52.93]:19289 "EHLO mga11.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750725AbaLZByf (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 25 Dec 2014 20:54:35 -0500
In-Reply-To: <8941A675-6DA0-4815-AAB3-9D9F6DC71270@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 2014/12/25 17:55, Nadav Amit wrote:
> Tiejun <tiejun.chen@intel.com> 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 exce=
ptions.
>>>
>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>> ---
>>>   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_c=
txt *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 =3D=3D 4) {
>> +               rsp_increment(ctxt, -2);
>> +               ctxt->op_bytes =3D 2;
>> +       }
>>         rc =3D emulate_pop(ctxt, &selector, ctxt->op_bytes);
>>         if (rc !=3D X86EMUL_CONTINUE)
>>                 return rc;
>>
>> Right?
> I don=92t think so. It seems the behaviour of push and pop is a bit d=
ifferent.
> For push: =93If the source operand is a segment register (16 bits) an=
d 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 p=
erform
> a 16-bit move, leaving the upper portion of the stack location unmodi=
fied.=94
>
> Therefore, for push in the case of op_bytes=3D=3D8 we push zero-exten=
ded value.
>
> For pop the behaviour is not well-documented, but experimentally it a=
ppears
> only the first two bytes are accessed. I cannot see why it would be

Maybe we can comment something here, like "/* Just force 2 byte=20
destination to already work well in most cases. */".

> different when opsize is 8, since it is not like the push case, where=
 the
> segment register value was zero extended.

Thanks for your explanation.

>
> If you feel strongly about it, I=92ll create a unit test.

Based on your description I think I can stand with you at this point.

Tiejun

>
> Nadav
>
>