Re: [Xen-devel] [PATCH] x86/monitor: add get_capabilities to monitor_op domctl

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

On Tue, Jul 7, 2015 at 4:22 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 07.07.15 at 02:43, <tlengyel@xxxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel