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.

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

If we are aiming for consistency then I think it does. Lets hear from the
maintainers and will go from there. I rather not start reworking
preexisting stuff because it tends to snowball into a lot more patches.

Tamas