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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Jun 24, 2016 at 5:24 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 24.06.16 at 13:20, <kevin.tian@xxxxxxxxx> wrote:
>>>  From: Tamas K Lengyel [mailto:tamas@xxxxxxxxxxxxx]
>>> Sent: Friday, June 24, 2016 1:07 AM
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 03fcba7..4b69ca6 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3373,10 +3373,39 @@ 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_len = 0;
>>> +                int rc;
>>> +                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_len);
>>> +
>>> +                rc = hvm_monitor_debug(regs->eip,
>>> +                                       HVM_MONITOR_DEBUG_EXCEPTION,
>>> +                                       trap_type, insn_len);
>>> +
>>> +                /*
>>> +                 * !rc  continue normally
>>> +                 * rc > paused waiting for response, work here is done
>>
>> Did you mean "rc > 0, vcpu is paused and waiting for response..."? Please
>> make the comment clear to understand.

Yes, sure.

>>
>>> +                 * rc < error, fall-through to exit_and_crash
>>> +                 */
>>> +                if ( !rc )
>>> +                {
>>> +                    vmx_propagate_intr(intr_info);
>>> +                    break;
>>> +                }
>>> +                if ( rc > 0 )
>>> +                    break;
>>> +            }
>>>              else
>>> +            {
>>>                  domain_pause_for_debugger();
>>> -            break;
>>> +                break;
>>> +            }
>>> +
>>> +            goto exit_and_crash;
>>
>> Putting goto as the last line within a 'case' looks a bit strange. What
>> about putting goto directly under a "if ( rc < 0 )" check earlier?
>>
>>               if ( !rc )
>>                       ...
>>               if ( rc < 0 )
>>                       goto exit_and_crash;
>>       }
>>       else
>>               domain_pause_for_debugger();
>>       break;
>
> Thanks, Kevin - indeed that's exactly what I had asked for already
> on the previous iteration.
>

I'm OK with adding the rc < 0 case, it's just that the fall-through
style is already used for handling the int3 events for example. Should
I fix that too while I'm at it so the code is consistent?

Tamas

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