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

[Tamas K Lengyel <tamas@tklengyel.com>]

On Mon, May 30, 2016 at 8:16 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 30.05.16 at 00:37, <tamas@tklengyel.com> 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().

Yea, vm_event_monitor_traps is not the most straight forward function
in regards of what the return value means. I'll see if I can clean it
up a bit in another patch.

>
>> --- 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.

You mean the instruction length is not necessarily 1? Ultimately it
doesn't seem to matter because reinjecting it with xc_hvm_inject_trap
ignores this field. Instruction length is only required to be properly
set AFAICT for a subset of debug exceptions during reinjection.

>
>> @@ -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?

Instruction length is only meaningful for a subset of debug exceptions
that could be reinjected to the guest using xc_hvm_inject_trap.
Monitor trap flag events are external to the guest so there is nothing
to inject. The instruction length field won't even exist for this type
of singlestep events (we distinguish vm_event_singlestep and
vm_event_debug structs for this reason), so 0 here is arbitrary. We
could set it to ~0 to make it more obvious that it's just a
placeholder in this context.

> Also you're creating a long line here.

Ack.

Thanks,
Tamas

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