From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/vm_event: toggle singlestep from vm_event response Date: Mon, 29 Jun 2015 15:40:18 +0100 Message-ID: <559158D2.9@citrix.com> References: <1435582737-12053-1-git-send-email-tlengyel@novetta.com> <559149A5.7060008@citrix.com> <55914E42.6010503@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6768872989949600767==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Lengyel, Tamas" Cc: keir@xen.org, Ian Campbell , Razvan Cojocaru , Xen-devel , stefano.stabellini@citrix.com, Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============6768872989949600767== Content-Type: multipart/alternative; boundary="------------040801090807070904050607" --------------040801090807070904050607 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit On 29/06/15 15:17, Lengyel, Tamas wrote: > > > On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas > wrote: > > > > On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper > > wrote: > > On 29/06/15 14:42, Lengyel, Tamas wrote: >> >> >> > --- a/xen/arch/x86/hvm/hvm.c >> > +++ b/xen/arch/x86/hvm/hvm.c >> > @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, >> int32_t op) >> > return rc; >> > } >> > >> > +void hvm_toggle_singlestep(struct vcpu *v) >> > +{ >> > + if ( !cpu_has_monitor_trap_flag ) >> >> monitor_trap_flag is a VMX feature. This will never be >> true on AMD >> systems. (its use in hvm_debug_op() is also dubious) >> >> >> Yes, this feature is only for Intel cpus. Reworking the use >> of the flag though is a bit out-of-scope for this patch itself. > > In which case you must make it very clear in the commit > message that this is for Intel only and needs fixing up for > AMD. Best also to have a comment to the same effect in this > function. > > > Sure. > > > > >> >> >> >> > + return; >> > + >> > + v->arch.hvm_vcpu.single_step = >> !v->arch.hvm_vcpu.single_step; >> > +} >> > + >> > int nhvm_vcpu_hostrestore(struct vcpu *v, struct >> cpu_user_regs *regs) >> > { >> > if (hvm_funcs.nhvm_vcpu_hostrestore) >> > diff --git a/xen/arch/x86/vm_event.c >> b/xen/arch/x86/vm_event.c >> > new file mode 100644 >> > index 0000000..95b30ad >> > --- /dev/null >> > +++ b/xen/arch/x86/vm_event.c >> > @@ -0,0 +1,41 @@ >> > +/* >> > + * arch/x86/vm_event.c >> > + * >> > + * Architecture-specific vm_event handling routines >> > + * >> > + * Copyright (c) 2015 Tamas K Lengyel >> (tamas@tklengyel.com ) >> > + * >> > + * This program is free software; you can redistribute >> it and/or >> > + * modify it under the terms of the GNU General Public >> > + * License v2 as published by the Free Software >> Foundation. >> > + * >> > + * This program is distributed in the hope that it >> will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied >> warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR >> PURPOSE. See the GNU >> > + * General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU General >> Public >> > + * License along with this program; if not, write to the >> > + * Free Software Foundation, Inc., 59 Temple Place - >> Suite 330, >> > + * Boston, MA 021110-1307, USA. >> > + */ >> > + >> > +#include >> > +#include >> > + >> > +void vm_event_toggle_singlestep(struct domain *d, >> struct vcpu *v) >> > +{ >> > + if ( (v == current) || !is_hvm_domain(d) ) >> >> Why is 'current' excluded? >> >> >> Only to be consistent with the sanity check applied for >> XEN_DOMCTL_debug_op. > > XEN_DOMCTL_debug_op needs the check because of vcpu_pause(). > It is always run in the context of the hypercaller domain, and > must never end up calling singlestep on itself. > > However in this case, we can only legitimately use this on an > already-paused vcpu, which guarentees that Xen is not > currently in the context of the target vcpu. > > It would probably be better to have a check against the vcpu > pause count here (with early return), and > hvm_toggle_singlestep() assert so. > > > I guess that's a fair sanity check to add in case the vm_event > listener is buggy. > > > I was thinking ASSERT(v->vm_event_pause_count.counter); but that locks > the function to only be usable through the vm_event response method. > What do you think? You want to use atomic_read() instead of peeking into .counter. vm_event_toggle_singlestep() can legitimately gate on v->vm_event_pause_count, whereas hvm_toggle_singlestep() can get away with looking at v->pause_count. See vmx_domain_emable_pml() as a similar example. ~Andrew --------------040801090807070904050607 Content-Type: text/html; charset="windows-1252" Content-Length: 15512 Content-Transfer-Encoding: quoted-printable
On 29/06/15 15:17, Lengyel, Tamas wrote:


On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas <tlengyel@novetta.com> wrote:


On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
On 29/06/15 14:42, Lengyel, Tamas wrote:


> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6431,6 +6431,14 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>=A0 =A0 =A0 return rc;
>=A0 }
>
> +void hvm_toggle_singlestep(struct vcpu *v)
> +{
> +=A0 =A0 if ( !cpu_has_monitor_trap_flag )

monitor_trap_flag is a VMX feature.=A0 This will never be true on AMD
systems.=A0 (its use in hvm_debug_op() is also dubious)

Yes, this feature is only for Intel cpus. Reworking the use of the flag though is a bit out-of-scope for this patch itself.

 In which case you must make it very clear in the commit message that this is for Intel only and needs fixing up for AMD.=A0 Best also to have a comment to the same effect in this function.
Sure.
=A0


=A0

> +=A0 =A0 =A0 =A0 return;
> +
> +=A0 =A0 v->arch.hvm_vcpu.single_step =3D !v->arch.hvm_vcpu.single_step;
> +}
> +
>=A0 int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>=A0 {
>=A0 =A0 =A0 if (hvm_funcs.nhvm_vcpu_hostrestore)
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> new file mode 100644
> index 0000000..95b30ad
> --- /dev/null
> +++ b/xen/arch/x86/vm_event.c
> @@ -0,0 +1,41 @@
> +/*
> + * arch/x86/vm_event.c
> + *
> + * Architecture-specific vm_event handling routines
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.=A0 See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; if not, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#include <xen/sched.h>
> +#include <asm/hvm/hvm.h>
> +
> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
> +{
> +=A0 =A0 if ( (v =3D=3D current) || !is_hvm_domain(d) )

Why is 'current' excluded=3F

Only to be consistent with the sanity check applied for XEN_DOMCTL_debug_op.

XEN_DOMCTL_debug_op needs the check because of vcpu_pause().=A0 It is always run in the context of the hypercaller domain, and must never end up calling singlestep on itself.

However in this case, we can only legitimately use this on an already-paused vcpu, which guarentees that Xen is not currently in the context of the target vcpu.

It would probably be better to have a check against the vcpu pause count here (with early return), and hvm_toggle_singlestep() assert so.

I guess that's a fair sanity check to add in case the vm_event listener is buggy.

I was thinking ASSERT(v->vm_event_pause_count.counter); but that locks the function to only be usable through the vm_event response method. What do you think=3F

You want to use atomic_read() instead of peeking into .counter.

vm_event_toggle_singlestep() can legitimately gate on v->vm_event_pause_count, whereas hvm_toggle_singlestep() can get away with looking at v->pause_count.

See vmx_domain_emable_pml() as a similar example.

~Andrew --------------040801090807070904050607-- --===============6768872989949600767== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6768872989949600767==--