Re: [Xen-devel] [PATCH v4 8/8] x86/vm_event: Add HVM debug exception vm_events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.05.16 at 00:37, <tamas@xxxxxxxxxxxxx> wrote:
> @@ -117,7 +133,11 @@ int hvm_monitor_breakpoint(unsigned long rip,
>  
>      req.vcpu_id = curr->vcpu_id;
>  
> -    return vm_event_monitor_traps(curr, 1, &req);
> +    rc = vm_event_monitor_traps(curr, sync, &req);
> +    if ( type == HVM_MONITOR_DEBUG_EXCEPTION && rc == 1 && !sync )
> +        rc = 0;
> +
> +    return rc;
>  }

To someone like me, not intimately familiar with the code, this added
logic looks pretty arbitrary. Please add a comment indicating why
under these special circumstances rc needs to be altered here, which
then will hopefully also clarify why that can't be done right in
vm_event_monitor_traps().

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3377,9 +3377,25 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>              if ( !v->domain->debugger_attached )
> -                vmx_propagate_intr(intr_info);
> +            {
> +                unsigned long insn_length = 0;
> +                int handled;

The variable name suggests it wants to be bool_t, but ...

> +                unsigned long trap_type = MASK_EXTR(intr_info,
> +                                                    
> INTR_INFO_INTR_TYPE_MASK);
> +
> +                if( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
> +
> +                handled = hvm_monitor_debug(regs->eip,
> +                                            HVM_MONITOR_DEBUG_EXCEPTION,
> +                                            trap_type, insn_length);
> +                if ( handled <= 0 )

... it clearly can't. Please use a better name (could by just "rc" or
"ret"). (Otoh I see that code you modify further down uses that
same naming for a similar purpose variable. Let's see what the VMX
maintainers say.)

> +                    vmx_propagate_intr(intr_info);
> +
> +            }
>              else
>                  domain_pause_for_debugger();
> +
>              break;
>          case TRAP_int3: 

If anywhere, this added blank line wants to go after the break.

> @@ -3393,8 +3409,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              }
>              else {
>                  int handled =
> -                    hvm_monitor_breakpoint(regs->eip,
> -                                           HVM_MONITOR_SOFTWARE_BREAKPOINT);
> +                        hvm_monitor_debug(regs->eip,
> +                                          HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                          X86_EVENTTYPE_SW_EXCEPTION, 1);

Please let's not add further mistakes like this, assuming INT3 can't
have any prefixes. It can, even if they're useless.

> @@ -3721,8 +3738,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          vmx_update_cpu_exec_control(v);
>          if ( v->arch.hvm_vcpu.single_step )
>          {
> -            hvm_monitor_breakpoint(regs->eip,
> -                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
> +            hvm_monitor_debug(regs->eip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, 
> 0, 0);

How come the 3rd argument is literal zero here?

Also you're creating a long line here.

Jan


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