On Tue, Jul 7, 2015 at 4:22 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 07.07.15 at 02:43, <tleng...@novetta.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -6431,6 +6431,11 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
> > return rc;
> > }
> >
> > +bool_t hvm_is_singlestep_supported(void)
> > +{
> > + return cpu_has_monitor_trap_flag;
> > +}
>
> This being a VMX flag, it should be tested in VMX code, i.e. you'd
> want to add a new hook to hvm_funcs.
>
Ack.
>
> > --- a/xen/arch/x86/monitor.c
> > +++ b/xen/arch/x86/monitor.c
> > @@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop,
> bool_t status)
> > return 0;
> > }
> >
> > +static inline
> > +void get_capabilities(struct domain *d, struct xen_domctl_monitor_op
> *mop)
> > +{
> > + mop->event = 0;
> > +
> > + if ( !is_hvm_domain(d) || !cpu_has_vmx )
> > + return;
> > +
> > + mop->event = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> > + (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> > + (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> > +
> > + if ( hvm_is_singlestep_supported() )
> > + mop->event |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> > +}
>
> And with this in mind I suppose the hook then would return a bit
> mask to be or-ed in here, instead of just a boolean flag.
>
I think the boolean flag return is a lot more generic. I rather keep it as
is - otherwise I have to mask the bitfield again in the toggle patch too.
>
> > @@ -51,6 +67,12 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> > if ( rc )
> > return rc;
> >
> > + if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES )
> > + {
> > + get_capabilities(d, mop);
> > + return 0;
> > + }
> > +
> > /*
> > * At the moment only Intel HVM domains are supported. However,
> event
> > * delivery could be extended to AMD and PV domains.
>
> Considering that capabilities could be domain specific (as is also
> expressed by d being passed to get_capabilities()), I think the
> conditional above should actually move past the one following this
> comment.
>
Sure.
>
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -1167,6 +1167,7 @@ long
> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > break;
> >
> > ret = monitor_domctl(d, &op->u.monitor_op);
> > + copyback = 1;
> > break;
>
> Conditional upon ret == 0?
>
Yeap, wasn't sure about if we should gate the copyback. My reasoning was in
doesn't hurt to copyback for enable/disable, it's just unnecessary. But
gating it is probably better.
> > @@ -1015,7 +1019,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
> >
> > struct xen_domctl_monitor_op {
> > uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> > - uint32_t event; /* XEN_DOMCTL_MONITOR_EVENT_* */
> > +
> > + /*
> > + * When used with ENABLE/DISABLE this has be set to
>
> ... has to be ... ?
>
Yeap.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
