Re: [PATCH v2 1/8] KVM: Fix load_guest_segment_descriptor() to inject page fault

[Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>]

Marcelo Tosatti wrote:
> On Wed, Feb 10, 2010 at 06:25:42PM +0200, Avi Kivity wrote:
>> On 02/10/2010 03:50 AM, Takuya Yoshikawa wrote:
>>> This patch injects page fault when reading descriptor in
>>> load_guest_segment_descriptor() fails with FAULT.
>>>
>>> Effects of this injection: This function is used by
>>> kvm_load_segment_descriptor() which is necessary for the
>>> following instructions.
>>>  - mov seg,r/m16
>>>  - jmp far
>>>  - pop ?s
>>> This patch makes it possible to emulate the page faults
>>> generated by these instructions. But be sure that unless
>>> we change the kvm_load_segment_descriptor()'s ret value
>>> propagation this patch has no effect.
>>>
>>>
>>> @@ -4655,6 +4655,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  {
>>>  	struct descriptor_table dtable;
>>>  	u16 index = selector>>  3;
>>> +	int ret;
>>>
>>>  	get_segment_descriptor_dtable(vcpu, selector,&dtable);
>>>
>>> @@ -4662,7 +4663,11 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>>>  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector&  0xfffc);
>>>  		return X86EMUL_PROPAGATE_FAULT;
>>>  	}
>>> -	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	ret = kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>>> +	if (ret == X86EMUL_PROPAGATE_FAULT)
>>> +		kvm_inject_page_fault(vcpu, dtable.base + index*8, 0);
>>> +
>>> +	return ret;
>>>  }
>> If the descriptor table is not aligned, and a descriptor spans two
>> pages, then we might need to inject a page fault at some other
>> address.
>>
>> Also, the injection should be done in kvm_read_guest_virt() to avoid
>> duplicating code.
>>
>> These instructions however are only emulated in big real mode.
>> Where did you encounter the need to inject page faults during their
>> emulation?
> 
> Task switch exits also use them, so it should inject faults as
> documented.

Yes, though I did not touch the task switch parts, we will have to do.