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 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().

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

Jan


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