[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 8/9] x86/vm_event: Add HVM debug exception vm_events
On Jun 3, 2016 08:23, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
