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 16:34, <tamas@xxxxxxxxxxxxx> wrote:
> 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.

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.

Jan


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