On Wed, Jul 8, 2015 at 10:37 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 07.07.15 at 15:52, <tleng...@novetta.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
