Re: [Xen-devel] [V0 PATCH 3/6] AMD-PVH: call hvm_emulate_one instead of handle_mmio

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.14 at 03:53, <mukesh.rathor@xxxxxxxxxx> wrote:
> Certain IOIO instructions and CR access instructions like
> lmsw/clts etc need to be emulated. handle_mmio is incorrectly called to
> accomplish this. Create svm_emulate() to call hvm_emulate_one which is more
> appropriate, and works for pvh as well.

Is that really correct without slight adjustments to the function? I'm
particularly worried about (some of) the uses of vio there...

> handle_mmio call is forbidden for pvh.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 4ff4a96..dac16f4 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2209,6 +2209,18 @@ static struct hvm_function_table __initdata 
> svm_function_table = {
>      .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
>  };
>  
> +static void svm_emulate(struct cpu_user_regs *regs)
> +{
> +    int rc;
> +    struct hvm_emulate_ctxt ctxt;
> +
> +    hvm_emulate_prepare(&ctxt, regs);
> +    rc = hvm_emulate_one(&ctxt);
> +
> +    if ( rc != X86EMUL_OKAY )
> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);

I think this is too simplistic - when ctxt.exn_pending is set, you
ought to be delivering _that_ exception instead of #GP.

> @@ -2470,16 +2482,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
> +            svm_emulate(regs);
>          break;
>  
>      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
> +            svm_emulate(regs);
>          break;
>  
>      case VMEXIT_INVLPG:

There's another use of handle_mmio() right below here - shouldn't
that get replaced too (even if not strictly required by PVH)?

Also - how come at least the use of the function in VMX's
EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
but SVM's VMEXIT_IOIO one is?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel