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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.06.16 at 15:29, <tamas@xxxxxxxxxxxxx> wrote:
> On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>
>> >>> On 03.06.16 at 00:52, <tamas@xxxxxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -3377,10 +3377,33 @@ 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;
>>
>> It's insn_len further down - please try to be consistent.
>>
>> > +                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_length);
>> > +
>> > +                rc = hvm_monitor_debug(regs->eip,
>> > +                                       HVM_MONITOR_DEBUG_EXCEPTION,
>> > +                                       trap_type, insn_length);
>> > +                if ( !rc )
>> > +                {
>> > +                    vmx_propagate_intr(intr_info);
>> > +                    break;
>> > +                }
>> > +                else if ( rc > 0 )
>> > +                    break;
>>
>> So you've removed the odd / hard to understand return value
>> adjustment from hvm_monitor_debug(), but this isn't any better:
>> What does the return value being positive really mean? And btw.,
>> no point using "else" after an unconditional "break" in the previous
>> if().
> 
> As the commit message explains in the other patch rc is 1 when the vCPU is
> paused. This means a synchronous event where we are waiting for the
> vm_event response thus work here is done.

The commit message of _another_ patch doesn't help at all a future
reader to understand what's going on here.

>> > +            }
>> >              else
>> > +            {
>> >                  domain_pause_for_debugger();
>> > -            break;
>> > +                break;
>> > +            }
>> > +
>> > +            goto exit_and_crash;
>>
>> There was no such goto before, i.e. you introduce this. I'm rather
>> hesitant to see such getting added without a good reason, and
>> that good reason should be stated in a comment. Also it looks like
>> the change would be easier to grok if you didn't alter the code
>> down here, but instead inverted the earlier if:
>>
>>                 if ( unlikely(rc < 0) )
>>                     /* ... */
>>                     goto exit_and_crash;
>>                 if ( !rc )
>>                     vmx_propagate_intr(intr_info);
>>
>> Which imo would get us closer to code being at least half way
>> self-explanatory.
> 
> I agree it may be more intuitive that way but adding the goto the way I did
> is whats consistent with the already established handling of int3 events. I
> either go for consistency or reworking more code at other spots too.

Well, as always I'll leave it to the maintainers to decide, but I think
my suggestion would make this code better readable, and doesn't
require immediate re-work elsewhere.

Jan


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