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 08:51:43 -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> <5751AF1402000078000F1A15@prv-mh.provo.novell.com> <5751B43B02000078000F1A80@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3435467076809889456==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b8qRw-0006bW-Ge for xen-devel@lists.xenproject.org; Fri, 03 Jun 2016 14:51:48 +0000 Received: by mail-wm0-f66.google.com with SMTP id a136so24248709wme.0 for ; Fri, 03 Jun 2016 07:51:46 -0700 (PDT) Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com. [74.125.82.49]) by smtp.gmail.com with ESMTPSA id c185sm46311465wme.9.2016.06.03.07.51.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 03 Jun 2016 07:51:45 -0700 (PDT) Received: by mail-wm0-f49.google.com with SMTP id z87so111488069wmh.0 for ; Fri, 03 Jun 2016 07:51:44 -0700 (PDT) In-Reply-To: <5751B43B02000078000F1A80@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 --===============3435467076809889456== Content-Type: multipart/alternative; boundary=001a11444f127d2476053460dcf9 --001a11444f127d2476053460dcf9 Content-Type: text/plain; charset=UTF-8 On Jun 3, 2016 08:45, "Jan Beulich" wrote: > > >>> On 03.06.16 at 16:34, wrote: > > On Jun 3, 2016 08:23, "Jan Beulich" wrote: > >> > >> >>> On 03.06.16 at 15:29, wrote: > >> > 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. > >> > >> 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. > Ack. I have nothing against adding a comment here. Tamas --001a11444f127d2476053460dcf9 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Jun 3, 2016 08:45, "Jan Beulich" <JBeulich@suse.com> wrote:
>
> >>> On 03.06.16 at 16:34, <tamas@tklengyel.com> wrote:
> > 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)
> >> >> >=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->domain->debugger_attached )
> >> >> > -=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 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 unsigned long insn_length =3D 0;
> >> >>
> >> >> It's insn_len further down - please try to be co= nsistent.
> >> >>
> >> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 int rc;
> >> >> > +=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,
> >> >> > +
> >> > 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 hvm_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 r= eturn value
> >> >> adjustment from hvm_monitor_debug(), but this isn= 9;t any better:
> >> >> What does the return value being positive really mea= n? And btw.,
> >> >> no point using "else" after an uncondition= al "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 wait= ing 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 b= e 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 wa= y
> self-explanatory," to indicate that if that adjustment was done,<= br> > perhaps a comment may not even be needed.
>

Ack.=C2=A0 I have nothing against adding a comment here.

Tamas

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