Re: [PATCH v4 3/3] x86/monitor: don't use hvm_funcs directly

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/09/2015 01:52 AM, Tamas K Lengyel wrote:
> A couple spots in x86/monitor used hvm_funcs directly. This patch adds an extra
> wrapper for enable_msr_exit_interception and changes monitor.c to use only the
> wrappers.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> ---
>  xen/arch/x86/monitor.c        |  7 ++-----
>  xen/include/asm-x86/hvm/hvm.h | 11 +++++++++++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 590a45d..44efcd0 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -130,7 +130,7 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 )
>              /* Latches new CR3 mask through CR0 code */
>              for_each_vcpu ( d, v )
> -                hvm_funcs.update_guest_cr(v, 0);
> +                hvm_update_guest_cr(v, 0);
>  
>          break;
>      }
> @@ -146,11 +146,8 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          if ( mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE &&
>               mop->u.mov_to_msr.extended_capture )
>          {
> -            if ( hvm_funcs.enable_msr_exit_interception )
> -            {
> +            if ( hvm_enable_msr_exit_interception(d) == 0 )
>                  ad->monitor.mov_to_msr_extended = 1;
> -                hvm_funcs.enable_msr_exit_interception(d);

I don't feel very strongly about it, so if you really prefer you can
keep the code as it is, however this looks somewhat counterintuitive to
me, especially when you compare the new condition to the old one, because
...

> -            }
>              else
>                  return -EOPNOTSUPP;
>          } else
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index aa8ab8d..9128dff 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -514,6 +514,17 @@ static inline enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>      return hvm_funcs.nhvm_intr_blocked(v);
>  }
>  
> +static inline int hvm_enable_msr_exit_interception(struct domain *d)
> +{
> +    if ( hvm_funcs.enable_msr_exit_interception )
> +    {
> +        hvm_funcs.enable_msr_exit_interception(d);
> +        return 0;
> +    }
> +
> +    return -ENOSYS;
> +}
> +

... I can see no reason why this function should not return bool_t, and
return 1 where it now returns 0, and 0 where it now returns -ENOSYS. But
perhaps you're anticipating future uses I don't see now, or somebody
else has a different opinion.


Cheers,
Razvan