From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH v5 8/9] x86/vm_event: Add HVM debug exception vm_events Date: Fri, 3 Jun 2016 07:29:44 -0600 Message-ID: References: <1464907946-19242-1-git-send-email-tamas@tklengyel.com> <1464907946-19242-8-git-send-email-tamas@tklengyel.com> <57517CD702000078000F1855@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3900416009584076223==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b8pAa-0005pm-VO for xen-devel@lists.xenproject.org; Fri, 03 Jun 2016 13:29:49 +0000 Received: by mail-wm0-f65.google.com with SMTP id a20so12077995wma.3 for ; Fri, 03 Jun 2016 06:29:47 -0700 (PDT) Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com. [74.125.82.50]) by smtp.gmail.com with ESMTPSA id d4sm5780960wjk.11.2016.06.03.06.29.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 03 Jun 2016 06:29:45 -0700 (PDT) Received: by mail-wm0-f50.google.com with SMTP id a136so276464042wme.0 for ; Fri, 03 Jun 2016 06:29:44 -0700 (PDT) In-Reply-To: <57517CD702000078000F1855@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Kevin Tian , Wei Liu , Razvan Cojocaru , Andrew Cooper , Ian Jackson , Jun Nakajima , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============3900416009584076223== Content-Type: multipart/alternative; boundary=001a1148ea9643396205345fb731 --001a1148ea9643396205345fb731 Content-Type: text/plain; charset=UTF-8 On Jun 3, 2016 04:49, "Jan Beulich" wrote: > > >>> On 03.06.16 at 00:52, 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. > > + } > > 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. Tamas --001a1148ea9643396205345fb731 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


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_r= egs *regs)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 HVMTRACE_1D(TRAP_= DEBUG, exit_qualification);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 write_debugreg(6,= exit_qualification | DR_STATUS_RESERVED_ONE);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( !v->domai= n->debugger_attached )
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vmx_prop= agate_intr(intr_info);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned= long insn_length =3D 0;
>
> It's insn_len further down - please try to be consistent.
>
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int rc;<= br> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned= long trap_type =3D MASK_EXTR(intr_info,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 INTR_INFO_INTR_TYPE_MASK); > > +
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if( trap= _type >=3D X86_EVENTTYPE_SW_INTERRUPT )
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_length);
> > +
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D h= vm_monitor_debug(regs->eip,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0HVM_MONITOR_DEBUG_EXCEPTION,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0trap_type, insn_length);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( !rc= )
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 vmx_propagate_intr(intr_info);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 break;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else if = ( rc > 0 )
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 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&quo= t; in the previous
> if().
>

As the commit message explains in the other patch rc is 1 wh= en the vCPU is paused. This means a synchronous event where we are waiting = for the vm_event response thus work here is done.

> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dom= ain_pause_for_debugger();
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > +
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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:
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( unlikely(= rc < 0) )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = /* ... */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = goto exit_and_crash;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( !rc )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = 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 got= o the way I did is whats consistent with the already established handling o= f int3 events. I either go for consistency or reworking more code at other = spots too.

Tamas

--001a1148ea9643396205345fb731-- --===============3900416009584076223== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============3900416009584076223==--