Re: [PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Kevin Wolf <kwolf@redhat.com>]

Am 24.01.2012 12:37, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 12:31:48PM +0100, Kevin Wolf wrote:
>> Am 24.01.2012 11:57, schrieb Gleb Natapov:
>>> On Mon, Jan 23, 2012 at 05:10:48PM +0100, Kevin Wolf wrote:
>>>> Task switches can switch between Protected Mode and VM86. The current
>>>> mode must be updated during the task switch emulation so that the new
>>>> segment selectors are interpreted correctly and privilege checks
>>>> succeed.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  arch/x86/include/asm/kvm_emulate.h |    1 +
>>>>  arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
>>>>  arch/x86/kvm/x86.c                 |    6 ++++++
>>>>  3 files changed, 24 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
>>>> index c8a9cf3..4a21c7d 100644
>>>> --- a/arch/x86/include/asm/kvm_emulate.h
>>>> +++ b/arch/x86/include/asm/kvm_emulate.h
>>>> @@ -176,6 +176,7 @@ struct x86_emulate_ops {
>>>>  	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
>>>>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
>>>>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
>>>> +	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
>>>>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
>>>>  	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
>>>>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
>>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>>> index 833969e..52fce89 100644
>>>> --- a/arch/x86/kvm/emulate.c
>>>> +++ b/arch/x86/kvm/emulate.c
>>>> @@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
>>>>  		return emulate_gp(ctxt, 0);
>>>>  	ctxt->_eip = tss->eip;
>>>>  	ctxt->eflags = tss->eflags | 2;
>>>> +
>>>> +	/*
>>>> +	 * If we're switching between Protected Mode and VM86, we need to make
>>>> +	 * sure to update the mode before loading the segment descriptors so
>>>> +	 * that the selectors are interpreted correctly.
>>>> +	 *
>>>> +	 * Need to get it to the vcpu struct immediately because it influences
>>>> +	 * the CPL which is checked when loading the segment descriptors.
>>> This is true only for VMX. SVM does not look at rflags. May be instead
>>> of adding new x86_emulate_ops callback we need to get rid of get_cpl()
>>> one and implement CPL checking using emulate.c:get_segment_selector().
>>
>> The selector isn't enough for VM86. In most cases
>> ctxt->ops->get_segment() would work (assuming that the dpl field is
>> valid there), but it doesn't in task switches before switching the code
>> segment, which already needs to have the new CPL applied to succeed.
>>
>> So the only other way I could think of is to pass a flag to
>> load_segment_descriptor() which says that it shouldn't check any privileges.
>>
> What I mean it to have similar logic to what we have in
> __vmx_get_cpl(). Something like this:
> 
> static int get_cpl(struct x86_emulate_ctxt *ctxt)
> {
> 	if (ctxt->mode == X86EMUL_MODE_REAL)
>                 return 0;
> 
>         if (ctxt->mode != X86EMUL_MODE_PROT64
>             && (ctx->eflags & EFLG_VM)) /* if virtual 8086 */
>                 return 3;
> 
>         return get_segment_selector(ctx, VCPU_SREG_CS) & 3;
> }

Ah, you mean just using ctxt->rflags instead of calling out into x86.c
to update it before the call. Yes, I think that would work.

Kevin