From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: [RFC][PATCH] VMX: Invalid guest state emulation
Date: Mon, 11 Aug 2008 11:53:30 +0300
Message-ID: <489FFE0A.2020001@qumranet.com>
References: <20080803020826.GA20831@mohd-laptop>	 <489EA246.4000105@qumranet.com> <52d4a3890808101145m6316994dx2b171677b647917b@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, riel@surriel.com, andrea@qumranet.com,
	guillaume.thouvenin@ext.bull.net
To: Mohammed Gamal <m.gamal005@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from il.qumranet.com ([212.179.150.194]:56408 "EHLO il.qumranet.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751301AbYHKIxc (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 11 Aug 2008 04:53:32 -0400
In-Reply-To: <52d4a3890808101145m6316994dx2b171677b647917b@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Mohammed Gamal wrote:
> On Sun, Aug 10, 2008 at 11:09 AM, Avi Kivity <avi@qumranet.com> wrote:
>   
>> Mohammed Gamal wrote:
>>     
>>> This patch aims to allow emulation whenever guest state is not valid for
>>> VMX operation. This usually happens in mode switches with guests such as
>>> older versions of gfxboot and FreeDOS with HIMEM.
>>> The patch aims to address this issue, it introduces the following:
>>>
>>> - A function that invokes the x86 emulator when the guest state is not
>>> valid (borrowed from Guillaume Thouvenin's real mode patches)
>>> - A function that checks that guest register state is VMX compliant
>>> - A module parameter that enables these operations. It is disabled by
>>> default, in order not to intervene with KVM's normal operation
>>>
>>>       
>>>  +/*
>>> + * Check if guest state is valid. Returns true if valid, false if
>>> + * not.
>>> + * We assume that registers are always usable
>>> + */
>>> +static bool guest_state_valid(struct kvm_vcpu *vcpu)
>>> +{
>>>       

...

>>> +       /* vm86 mode guest state checks */
>>> +       if(vcpu->arch.rmode.active) {
>>>
>>>       
>> Better to check cr0 here.
>>     
>
> Why? when cr0.PE bit is cleared, enter_rmode() is called and
> vcpu->arch.rmode.active is set, or do you mean it should be checked in
> addition?
>
>   

No, instead.  If we check cr0 then the function only depends on guest 
state and is therefore easier to understand.

Eventually rmode.active will go away.

>>> +               /* Check segment limits */
>>> +               if( (cs_limit != 0xffff) || (ds_limit != 0xffff) ||
>>> +                   (ss_limit != 0xffff) || (es_limit != 0xffff) ||
>>> +                   (fs_limit != 0xffff) || (gs_limit != 0xffff) )
>>> +                       return false;
>>>
>>>       
>> Would be nice to get code reuse, here and below (i.e. data_segment_valid()
>> for ds,es..gs.).  Also, vmx_get_segment() will make the code tidier.
>>     
>
> I think the function can be further broken down into smaller functions
> each checking for a certain segment parameter (e.g.
> segment_limit_valid(), segment_ar_valid(), rip_valid() ...etc.) and
> then call all these functions in guest_state_valid().
>
>   

Sure.


-- 
error compiling committee.c: too many arguments to function