Re: [Xen-devel] [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.06.18 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1437,19 +1437,49 @@ static void svm_inject_event(const struct x86_event 
> *event)
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case TRAP_debug:
> -        if ( regs->eflags & X86_EFLAGS_TF )
> -        {
> -            __restore_debug_registers(vmcb, curr);
> -            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
> -        }
> +        /*
> +         * On AMD hardware, a #DB exception:
> +         *  1) Merges new status bits into %dr6
> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
> +         *
> +         * Item 1 is done by hardware before a #DB intercepted vmexit, but we
> +         * may end up here from emulation so have to repeat it ourselves.
> +         * Item 2 is done by hardware when injecting a #DB exception.
> +         */
> +        __restore_debug_registers(vmcb, curr);
> +        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
> +
>          /* fall through */
>      case TRAP_int3:
>          if ( curr->domain->debugger_attached )
>          {
>              /* Debug/Int3: Trap to debugger. */
> +            if ( _event.vector == TRAP_int3 )
> +            {
> +                /* N.B. Can't use __update_guest_eip() for risk of recusion. 
> */
> +                regs->rip += _event.insn_len;

Not all callers provide a non-zero insn length. Is that a potential
problem here (and in the equivalent VMX code)?

> +                regs->eflags &= ~X86_EFLAGS_RF;
> +                curr->arch.gdbsx_vcpu_event = TRAP_int3;
> +            }
> +
>              domain_pause_for_debugger();
>              return;
>          }
> +        else
> +        {

Considering the "return" above, could you avoid the "else" and
the extra level of indentation here? Same for VMX then again.

> +            int rc = hvm_monitor_debug(regs->rip,

"rc" can surely be declared in slightly wider a scope.

> @@ -2775,67 +2805,46 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>  
>      case VMEXIT_ICEBP:
>      case VMEXIT_EXCEPTION_DB:
> -        if ( !v->domain->debugger_attached )
> +    case VMEXIT_EXCEPTION_BP:
> +    {
> +        unsigned int vec, type, len, extra;
> +
> +        switch ( exit_reason )
>          {
> -            int rc;
> -            unsigned int trap_type;
> +        case VMEXIT_ICEBP:

I don't understand this structuring of the code: The outer switch()
has the exact same control expression, and there's no fall through
- why the nesting? Move the variable declarations to the beginning
of the outer switch() and drop the inner one? You may not even
need all of the variables if you replicated the little bit of code
currently shared by the three (immediately after the inner switch()).

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1778,15 +1778,21 @@ static void vmx_inject_event(const struct x86_event 
> *event)
>      unsigned long intr_info;
>      struct vcpu *curr = current;
>      struct x86_event _event = *event;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>  
>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>      {
>      case TRAP_debug:
> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> -        {
> -            __restore_debug_registers(curr);
> -            write_debugreg(6, read_debugreg(6) | DR_STEP);
> -        }
> +        /*
> +         * On Intel hardware, a #DB exception:
> +         *  1) Merges new status bits into %dr6
> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.LBR
> +         *
> +         * All actions are left up to the hypervisor to perform.
> +         */
> +        __restore_debug_registers(curr);
> +        write_debugreg(6, read_debugreg(6) | event->pending_dbg);

Same as for the PV case in the earlier patch: Don't you need to clear the
low four bits first?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel