On Jun 3, 2016 08:45, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 03.06.16 at 16:34, <tamas@tklengyel.com> wrote:
> > On Jun 3, 2016 08:23, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>
> >> >>> On 03.06.16 at 15:29, <tamas@tklengyel.com> wrote:
> >> > 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.
> >>
> >> The commit message of _another_ patch doesn't help at all a future
> >> reader to understand what's going on here.
> >
> > This is already used elsewhere in similar fashion so I don't see why we
> > would need to treat this case any differently. Its not like I'm
introducing
> > a totally new way of doing this. So IMHO adding a comment would be an OK
> > middle ground but my goal is really not to rework everything.
>
> Nothing but a comment was what I was hoping for. And then later,
> in the remark regarding the odd code structure further down, I did
> say "Which imo would get us closer to code being at least half way
> self-explanatory," to indicate that if that adjustment was done,
> perhaps a comment may not even be needed.
>

Ack.  I have nothing against adding a comment here.

Tamas