Re: [RFC V0 PATCH 1/1] Replace handle_mmio calls in svm/vmx

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.08.14 at 03:15, <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1252,6 +1252,32 @@ void hvm_emulate_prepare(
>      hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
>  }
>  
> +void hvm_emulate(struct cpu_user_regs *regs)

This surely is too generic a name.

> +{
> +    int rc;
> +    struct hvm_emulate_ctxt ctxt;
> +    
> +    hvm_emulate_prepare(&ctxt, regs);
> +    rc = hvm_emulate_one(&ctxt);

As said in an earlier mail, I'm not sure this is the right thing to do
(namely continuing to use the full hvm_emulate_ops).

> +    case X86EMUL_EXCEPTION:
> +    {
> +        uint8_t vector = ctxt.exn_pending ? ctxt.exn_vector : TRAP_gp_fault;
> +        int32_t errcode = ctxt.exn_pending ? ctxt.exn_error_code : 0;
> +        hvm_inject_hw_exception(vector, errcode);

This looks ugly. Either do this with if/else, or put the conditional
operators right as function arguments. (And style-wise if it
remained the way it is it would at least need a blank line between
declarations and statements.)

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2475,16 +2475,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>              if ( handle_pio(port, bytes, dir) )
>                  __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
>          }
> -        else if ( !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else 
> +            hvm_emulate(regs);

As said in the earlier mail, I think there is a reason for this to call
handle_mmio().

>      case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
>      case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
>          if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
>              svm_vmexit_do_cr_access(vmcb, regs);
> -        else if ( !handle_mmio() ) 
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else
> +            hvm_emulate(regs);

While this one indeed doesn't need it, nor does it (as said above)
need the full hvm_emulate_ops.

> @@ -2493,8 +2493,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>              svm_invlpg_intercept(vmcb->exitinfo1);
>              __update_guest_eip(regs, vmcb->nextrip - vmcb->rip);
>          }
> -        else if ( !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else
> +            hvm_emulate(regs);

Same here.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3008,8 +3008,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case EXIT_REASON_APIC_ACCESS:
> -        if ( !vmx_handle_eoi_write() && !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        if ( !vmx_handle_eoi_write() )
> +            hvm_emulate(regs);

How can this not need handle_mmio()? The LAPIC page is an
(emulated) MMIO one after all.

> @@ -3026,11 +3026,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      case EXIT_REASON_IO_INSTRUCTION:
>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
>          if ( exit_qualification & 0x10 )
> -        {
> -            /* INS, OUTS */
> -            if ( !handle_mmio() )
> -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -        }
> +            hvm_emulate(regs);   /* INS, OUTS */

Same as for AMD's IOIO case.

Jan