On Mon, Jun 29, 2015 at 9:55 AM, Andrew Cooper <andrew.coop...@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 (ta...@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.
>
> ~Andrew
>
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
