From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 2/2] add sysenter/syscall emulation for 32bit compat mode
Date: Sun, 07 Jun 2009 10:09:26 +0300
Message-ID: <4A2B67A6.80206@redhat.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> <4A290E46.8010107@amd.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: Andre Przywara <andre.przywara@amd.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:44737 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754124AbZFGHJ3 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 7 Jun 2009 03:09:29 -0400
In-Reply-To: <4A290E46.8010107@amd.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Andre Przywara wrote:
>
>>
>>> @@ -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.

We return -1, but that doesn't mean we inject #UD.  For some cases (page 
table emulation), we unshadow the page and retry (letting the guest 
execute natively).

We only inject #UD from that specific call site.  If we somehow emulated 
from another call site, we'd get different behaviour.

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

I misread, I thought you were setting ss.limit twice.  Certainly the 
code is correct as is and should not be modified.  Sorry about the 
confusion.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.