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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4906 bytes --]

On 29/06/15 15:17, Lengyel, Tamas wrote:
>
>
> On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas <tlengyel@novetta.com
> <mailto:tlengyel@novetta.com>> wrote:
>
>
>
>     On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper
>     <andrew.cooper3@citrix.com <mailto: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)
>>             >      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 <mailto: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 <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?

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

[-- Attachment #1.2: Type: text/html, Size: 15342 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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