Re: [RFC][PATCH] VMX: Invalid guest state emulation

[Avi Kivity <avi@qumranet.com>]

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