On Sun, Aug 10, 2008 at 11:09 AM, Avi Kivity <[EMAIL PROTECTED]> 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)
>> +{
>> + u16 cs,ds,ss,es,fs,gs,tr,ldtr;
>> + u64 cs_limit, ds_limit, ss_limit, es_limit, fs_limit, gs_limit;
>> + u64 tr_limit, ldtr_limit;
>> + u32 cs_ar, ds_ar, ss_ar, es_ar, fs_ar, gs_ar, tr_ar, ldtr_ar;
>> +
>> + cs = vmcs_read16(GUEST_CS_SELECTOR);
>> + ds = vmcs_read16(GUEST_DS_SELECTOR);
>> + ss = vmcs_read16(GUEST_SS_SELECTOR);
>> + es = vmcs_read16(GUEST_ES_SELECTOR);
>> + fs = vmcs_read16(GUEST_FS_SELECTOR);
>> + gs = vmcs_read16(GUEST_GS_SELECTOR);
>> + tr = vmcs_read16(GUEST_TR_SELECTOR);
>> + ldtr = vmcs_read16(GUEST_LDTR_SELECTOR);
>> +
>> + cs_limit = vmcs_readl(GUEST_SS_LIMIT);
>> + ds_limit = vmcs_readl(GUEST_DS_LIMIT);
>> + ss_limit = vmcs_readl(GUEST_SS_LIMIT);
>> + es_limit = vmcs_readl(GUEST_ES_LIMIT);
>> + fs_limit = vmcs_readl(GUEST_FS_LIMIT);
>> + gs_limit = vmcs_readl(GUEST_GS_LIMIT);
>> + tr_limit = vmcs_readl(GUEST_TR_LIMIT);
>> +
>> + cs_ar = vmcs_read32(GUEST_CS_AR_BYTES);
>> + ds_ar = vmcs_read32(GUEST_DS_AR_BYTES);
>> + ss_ar = vmcs_read32(GUEST_SS_AR_BYTES);
>> + es_ar = vmcs_read32(GUEST_ES_AR_BYTES);
>> + fs_ar = vmcs_read32(GUEST_FS_AR_BYTES);
>> + gs_ar = vmcs_read32(GUEST_GS_AR_BYTES);
>> + tr_ar = vmcs_read32(GUEST_TR_AR_BYTES);
>> + ldtr_ar = vmcs_read32(GUEST_LDTR_AR_BYTES);
>> +
>> + if(tr & 0x02) /* TI = 1 */
>> + return false;
>> +
>> + if(ldtr & 0x02) /* TI = 1 */
>> + return false;
>> +
>> + /* 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?
>
>> + /* 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().
>
>> +
>> static void fix_pmode_dataseg(int seg, struct kvm_save_segment *save)
>> {
>> struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
>> @@ -1311,10 +1486,12 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
>> update_exception_bitmap(vcpu);
>> - fix_pmode_dataseg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
>> - fix_pmode_dataseg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
>> - fix_pmode_dataseg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
>> - fix_pmode_dataseg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
>> + if(!emulate_invalid_guest_state) {
>> + fix_pmode_dataseg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
>> + fix_pmode_dataseg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
>> + fix_pmode_dataseg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
>> + fix_pmode_dataseg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
>> + }
>> vmcs_write16(GUEST_SS_SELECTOR, 0);
>> vmcs_write32(GUEST_SS_AR_BYTES, 0x93);
>>
>
> These (and the other hacks below) need also to be qualified with
> emulate_invalid_guest_state.
>
I've already done that, I'll update my patch to take your suggestions
in consideration.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
