On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> 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.

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

Tamas