Re: [PATCH] x86/HVM: prevent infinite VM entry retries

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/11/14 12:39, Jan Beulich wrote:
> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
> guest upon problems occurring in user mode") and gets SVM in line with
> the resulting VMX behavior. This is because Andrew validly says
>
> "A failed vmentry is overwhelmingly likely to be caused by corrupt
>  VMC[SB] state.  As a result, injecting a fault and retrying the the
>  vmentry is likely to fail in the same way."
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
> v2: Replace SVM change by what Tim suggested.
>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2371,8 +2371,8 @@ void svm_vmexit_handler(struct cpu_user_
>                  goto out;
>              case NESTEDHVM_VMEXIT_FATALERROR:
>                  gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
> -                goto exit_and_crash;
> -
> +                domain_crash(v->domain);
> +                goto out;
>              default:
>                  BUG();
>              case NESTEDHVM_VMEXIT_ERROR:
> @@ -2385,18 +2385,21 @@ void svm_vmexit_handler(struct cpu_user_
>          case NESTEDHVM_VMEXIT_FATALERROR:
>              gdprintk(XENLOG_ERR,
>                  "unexpected nestedsvm_check_intercepts() error\n");
> -            goto exit_and_crash;
> +            domain_crash(v->domain);
> +            goto out;
>          default:
>              gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
>                  nsret);
> -            goto exit_and_crash;
> +            domain_crash(v->domain);
> +            goto out;
>          }
>      }
>  
>      if ( unlikely(exit_reason == VMEXIT_INVALID) )
>      {

I think it would be good to retain the printk here from previous
versions of the patch, specifically identifies the below vmcb dump as
caused by a failed vmentry.  As it stands, it is a vmcb dump with no
other context.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>          svm_vmcb_dump(__func__, vmcb);
> -        goto exit_and_crash;
> +        domain_crash(v->domain);
> +        goto out;
>      }
>  
>      perfc_incra(svmexits, exit_reason);
> @@ -2431,13 +2434,13 @@ void svm_vmexit_handler(struct cpu_user_
>  
>      case VMEXIT_EXCEPTION_DB:
>          if ( !v->domain->debugger_attached )
> -            goto exit_and_crash;
> +            goto unexpected_exit_type;
>          domain_pause_for_debugger();
>          break;
>  
>      case VMEXIT_EXCEPTION_BP:
>          if ( !v->domain->debugger_attached )
> -            goto exit_and_crash;
> +            goto unexpected_exit_type;
>          /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
>          if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
>              break;
> @@ -2684,7 +2687,7 @@ void svm_vmexit_handler(struct cpu_user_
>          break;
>  
>      default:
> -    exit_and_crash:
> +    unexpected_exit_type:
>          gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -134,18 +134,6 @@ static void vmx_vcpu_destroy(struct vcpu
>      passive_domain_destroy(v);
>  }
>  
> -/* Only crash the guest if the problem originates in kernel mode. */
> -static void vmx_crash_or_fault(struct vcpu *v)
> -{
> -    struct segment_register ss;
> -
> -    vmx_get_segment_register(v, x86_seg_ss, &ss);
> -    if ( ss.attr.fields.dpl )
> -        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> -    else
> -        domain_crash(v->domain);
> -}
> -
>  static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
>  
>  static const u32 msr_index[] =
> @@ -2520,7 +2508,7 @@ static void vmx_failed_vmentry(unsigned 
>      vmcs_dump_vcpu(curr);
>      printk("**************************************\n");
>  
> -    vmx_crash_or_fault(curr);
> +    domain_crash(curr->domain);
>  }
>  
>  void vmx_enter_realmode(struct cpu_user_regs *regs)
> @@ -3173,8 +3161,19 @@ void vmx_vmexit_handler(struct cpu_user_
>      /* fall through */
>      default:
>      exit_and_crash:
> -        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
> -        vmx_crash_or_fault(v);
> +        {
> +            struct segment_register ss;
> +
> +            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
> +                     exit_reason);
> +
> +            vmx_get_segment_register(v, x86_seg_ss, &ss);
> +            if ( ss.attr.fields.dpl )
> +                hvm_inject_hw_exception(TRAP_invalid_op,
> +                                        HVM_DELIVER_NO_ERROR_CODE);
> +            else
> +                domain_crash(v->domain);
> +        }
>          break;
>      }
>  
>
>