Re: [PATCH 2/2] VMX: Reinject real mode exception

[Avi Kivity <avi@qumranet.com>]

Jan Kiszka wrote:
> As we execute real mode guests in VM86 mode, exception have to be
> reinjected appropriately when the guest triggered them. For this purpose
> the patch adopts the real-mode injection pattern used in vmx_inject_irq
> to vmx_queue_exception, additionally taking care that the IP is set
> correctly for #BP exceptions. Furthermore it extends
> handle_rmode_exception to reinject all those exceptions that can be
> raised in real mode.
>
> This fixes the execution of himem.exe from FreeDOS and also makes its
> debug.com work properly.
>
> Note that guest debugging in real mode is broken now. This has to be
> fixed by the scheduled debugging infrastructure rework (will be done
> once base patches for QEMU have been accepted).
>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>  arch/x86/kvm/vmx.c         |   40 ++++++++++++++++++++++++++++++++++++++--
>  include/asm-x86/kvm_host.h |    2 ++
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> Index: b/arch/x86/kvm/vmx.c
> ===================================================================
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -735,12 +735,30 @@ static void skip_emulated_instruction(st
>  static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>  				bool has_error_code, u32 error_code)
>  {
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> +	if (has_error_code)
> +		vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
> +
> +	if (vcpu->arch.rmode.active) {
> +		vmx->rmode.irq.pending = true;
> +		vmx->rmode.irq.vector = nr;
> +		vmx->rmode.irq.rip = kvm_rip_read(vcpu);
> +		if (nr == BP_VECTOR)
> +			vmx->rmode.irq.rip++;
> +		vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
> +			     nr | INTR_TYPE_SOFT_INTR
> +			     | (has_error_code ? INTR_INFO_DELIVER_CODE_MASK : 0)
> +			     | INTR_INFO_VALID_MASK);
> +		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
> +		kvm_rip_write(vcpu, vmx->rmode.irq.rip - 1);
> +		return;
> +	}
> +
>   

It's a little sad that this duplicates vmx_inject_irq.  But we can live 
with it.

>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
>  		     nr | INTR_TYPE_EXCEPTION
>  		     | (has_error_code ? INTR_INFO_DELIVER_CODE_MASK : 0)
>  		     | INTR_INFO_VALID_MASK);
> -	if (has_error_code)
> -		vmcs_write32(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
>  }
>  
>  static bool vmx_exception_injected(struct kvm_vcpu *vcpu)
> @@ -2234,6 +2252,24 @@ static int handle_rmode_exception(struct
>  	if (((vec == GP_VECTOR) || (vec == SS_VECTOR)) && err_code == 0)
>  		if (emulate_instruction(vcpu, NULL, 0, 0, 0) == EMULATE_DONE)
>  			return 1;
> +	/*
> +	 * Forward all other exceptions that are valid in real mode.
> +	 * FIXME: Breaks guest debugging in real mode, need to be fixed with
> +	 *        the required debugging infrastructure rework.
> +	 */
> +	switch (vec) {
> +	case DF_VECTOR:
> +	case SS_VECTOR:
> +	case GP_VECTOR:
> +		kvm_queue_exception_e(vcpu, vec, err_code);
>   

These don't actually queue an error code in real mode.  Compare the 
'Real-Address Mode Exceptions' section with the 'Protected Mode 
Exceptions' section  for most instructions in the Intel manual.

> +		return 1;
> +		break;
> +	case DE_VECTOR ... NM_VECTOR:
>   

Please spell out explicitly.  Also, NM_VECTOR is used by the host for 
managing the guest fpu, so some NM_VECTORs should not be reflected.  I 
suggest simply dropping NM_VECTOR until this is worked out, as many 
real-mode operating systems don't implement lazy fpu switching.

Also, NMI shouldn't be handled here.

> +	case MF_VECTOR:
> +	case MC_VECTOR:
>   

I don't think #MC should be handled here.  Machine checks have to be 
handled by the host.

> +		kvm_queue_exception(vcpu, vec);
> +		return 1;
> +	}
>  	return 0;
>  }
>  

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