Re: [Xen-devel] [PATCH] x86/vm_event: toggle singlestep from vm_event response

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

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

On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx)
> + *
> + * 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 <xen/sched.h>
> +#include <asm/hvm/hvm.h>
> +
> +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?

Tamas

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