On Mon, Jun 29, 2015 at 10:09 AM, Lengyel, Tamas <tleng...@novetta.com>
wrote:
>
>
> 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.
>
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@lists.xen.org
http://lists.xen.org/xen-devel
