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

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

On Wed, Jul 8, 2015 at 10:37 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 07.07.15 at 15:52, <tlengyel@xxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -42,10 +42,29 @@ int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
>      return 0;
>  }
>
> +static inline uint32_t get_capabilities(struct domain *d)
> +{
> +    uint32_t capabilities = 0;
> +
> +    if ( !is_hvm_domain(d) || !cpu_has_vmx )
> +        return capabilities;
> +
> +    capabilities = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> +                   (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> +                   (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> +
> +    /* Since we know this is on VMX, we can just call the hvm func */
> +    if ( hvm_funcs.is_singlestep_supported() )

You shouldn't use hvm_funcs directly here, but go through an inline
wrapper just like done for other such hooks.

Ack.
 

>  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>  {
>      int rc;
>      struct arch_domain *ad = &d->arch;
> +    uint32_t capabilities = get_capabilities(d);

The variable doesn't appear to be needed at all; the two use sites
can easily call the function individually. But it's your code, so I'll
leave it up to you whether to make that change.

I think it's cleaner this way - doing a bitmask with a function's return in an if statement.would be convoluted a bit.

Thanks,

Tamas

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