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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 04, 2018 at 02:59:12PM +0100, Andrew Cooper wrote:
> Currently, there is a lot of functionality in the #DB intercepts, and some
> repeated functionality in the *_inject_event() logic.
> 
> The gdbsx code is implemented at both levels (albeit differently for #BP,
> which is presumably due to the fact that the old emulator behaviour used to be
> to move %rip forwards for traps), while the monitor behaviour only exists at
> the intercept level.
> 
> Updating of %dr6 is implemented (buggily) at both levels, but having it at
> both levels is problematic to implement correctly.
> 
> Rearrange the logic to have nothing interesting at the intercept level, and
> everything implemented at the injection level.  Amongst other things, this
> means that the monitor subsystem will pick up debug actions from emulated
> events.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> @@ -1797,16 +1803,39 @@ static void vmx_inject_event(const struct x86_event 
> *event)
>              __vmread(GUEST_IA32_DEBUGCTL, &val);
>              __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
>          }
> -        if ( cpu_has_monitor_trap_flag )
> -            break;
> +
>          /* 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;
> +                regs->eflags &= ~X86_EFLAGS_RF;
> +                curr->arch.gdbsx_vcpu_event = TRAP_int3;
> +            }
> +
>              domain_pause_for_debugger();
>              return;
>          }
> +        else
> +        {
> +            int rc = hvm_monitor_debug(regs->rip,
> +                                       _event.vector == TRAP_debug
> +                                       ? HVM_MONITOR_DEBUG_EXCEPTION
> +                                       : HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                       _event.type, _event.insn_len);
> +            if ( rc < 0 )
> +            {
> +                gprintk(XENLOG_ERR, "Monitor debug error %d\n", rc);
> +                domain_crash(curr->domain);
> +                return;
> +            }
> +            if ( rc )
> +                return; /* VCPU paused.  Wait for monitor. */
> +        }
>          break;

This look fairly similar to the svm_inject_event code, I wonder if
those could be merged somehow (or certain part of it shared).

Roger.

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