Re: [PATCH 2/8] KVM: x86: pop sreg accesses only 2 bytes

["Chen, Tiejun" <tiejun.chen@intel.com>]

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

Maybe we can comment something here, like "/* Just force 2 byte 
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’ll create a unit test.

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

Tiejun

>
> Nadav
>
>