From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andre Przywara <andre.przywara@amd.com>
Subject: Re: [PATCH 2/2] add sysenter/syscall emulation for 32bit compat mode
Date: Fri, 5 Jun 2009 14:23:34 +0200
Message-ID: <4A290E46.8010107@amd.com>
References: <1243504592-5112-1-git-send-email-andre.przywara@amd.com> <1243504592-5112-2-git-send-email-andre.przywara@amd.com> <4A2246E7.7050102@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, Amit Shah <amit.shah@redhat.com>,
	Christoph Egger <christoph.egger@amd.com>
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from va3ehsobe005.messaging.microsoft.com ([216.32.180.15]:38813
	"EHLO VA3EHSOBE006.bigfish.com" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751264AbZFEMUn (ORCPT
	<rfc822;kvm@vger.kernel.org>); Fri, 5 Jun 2009 08:20:43 -0400
In-Reply-To: <4A2246E7.7050102@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Hi Avi,

>> --- a/arch/x86/kvm/x86_emulate.c
>> +++ b/arch/x86/kvm/x86_emulate.c
>> @@ -32,6 +32,8 @@
>>  #include <linux/module.h>
>>  #include <asm/kvm_x86_emulate.h>
>>  
>> +#include "mmu.h"
>> +
>>   
> 
> I think this is unneeded?
Seems so. Probably a left-over from debugging.
> 
>> @@ -1985,10 +1992,114 @@ twobyte_insn:
>>              goto cannot_emulate;
>>          }
>>          break;
>> +    case 0x05: { /* syscall */
>> +        unsigned long cr0 = ctxt->vcpu->arch.cr0;
>> +        struct kvm_segment cs, ss;
>> +
>> +        memset(&cs, 0, sizeof(struct kvm_segment));
>> +        memset(&ss, 0, sizeof(struct kvm_segment));
>> +
>> +        /* inject #UD if
>> +         * 1. we are in real mode
>> +         * 2. protected mode is not enabled
>> +         * 3. LOCK prefix is used
>> +         */
>> +        if ((ctxt->mode == X86EMUL_MODE_REAL)
>> +            || (!(cr0 & X86_CR0_PE))
>> +            || (c->lock_prefix)) {
>> +            /* we don't need to inject #UD here, because
>> +             * when emulate_instruction() returns something else
>> +             * than EMULATE_DONE, then svm.c:ud_interception()
>> +             * will do that for us.
>> +             */
>> +            goto cannot_emulate;
>>   
> 
> I prefer explicit injection, relying on the caller is tricky and may 
> change.
I don't agree. If this function cannot emulate an instruction, it 
returns -1 and lets the upper levels handle this. If we cannot rely on 
this, what else can we rely on? I could remove the comment in case this 
is confusing. The same functionality (return -1 to inject UD into the 
guest) is used in other places in this same file.

> 
>> +    case 0x07:        /* sysret */
>>   
> 
> Since we don't emulate sysret, it should be dropped here.
OK, will do.
> 
>> +            cs.limit = 0xffffffff;
>> +            ss.base = 0;
>> +            ss.limit = 0xffffffff;
>>   
> 
> Once is enough.
You are right about the ss.base assignment. But the limit goes from five 
f's to eight f's. On a first glance this should not matter (as the 
granularity bit is set), but exactly here are differences between VMX 
and SVM, so I'd like to leave it this way.
> 
> Please move the code out of the switch and into separate functions.

Ok, will do.

Thanks for the review!
Renewed patch will follow.

Regards,
Andre.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712