Re: [Xen-devel] [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.06.16 at 16:12, <czuzu@xxxxxxxxxxxxxxx> wrote:
> Prepare for ARM implementation of control-register write vm-events by moving
> X86-specific hvm_event_cr to the common-side.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/event.c        | 30 ------------------------------
>  xen/arch/x86/hvm/hvm.c          |  2 +-
>  xen/arch/x86/monitor.c          | 37 -------------------------------------
>  xen/arch/x86/vm_event.c         |  2 +-
>  xen/common/monitor.c            | 40 ++++++++++++++++++++++++++++++++++++++++
>  xen/common/vm_event.c           | 31 +++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/event.h | 13 ++++---------
>  xen/include/asm-x86/monitor.h   |  2 --
>  xen/include/xen/monitor.h       |  4 ++++
>  xen/include/xen/vm_event.h      | 10 ++++++++++
>  10 files changed, 91 insertions(+), 80 deletions(-)

Considering that there's no ARM file getting altered here at all,
mentioning ARM in the subject is a little odd.

> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -62,6 +62,46 @@ int monitor_domctl(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>  
>      switch ( mop->event )
>      {
> +#if CONFIG_X86

#ifdef please.

> +    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
> +    {
> +        struct arch_domain *ad = &d->arch;

Peeking into the next patch I see that this stays there. Common code,
however, shouldn't access ->arch sub-structures - respective fields
should be moved out.

And looking at all the uses of this variable I get the impression that
you really want a shorthand for &d->arch.monitor (if any such
helper variable is worthwhile to have here in the first place).

> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -24,8 +24,6 @@
>  
>  #include <xen/sched.h>
>  
> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
> -
>  static inline
>  int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op 
> *mop)
>  {
> --- a/xen/include/xen/monitor.h
> +++ b/xen/include/xen/monitor.h
> @@ -25,6 +25,10 @@
>  struct domain;
>  struct xen_domctl_monitor_op;
>  
> +#if CONFIG_X86
> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
> +#endif

What's the point in removing this from the x86 header if then it
needs to be put in such a conditional? If the conditional gets
dropped in the next patch, then I think you have two options:
Leave it where it is here, and move it there. Or move it here,
but omit the conditional right away - I can't see this definition
being present to harm the ARM build in any way.

> --- a/xen/include/xen/vm_event.h
> +++ b/xen/include/xen/vm_event.h
> @@ -96,6 +96,16 @@ void vm_event_vcpu_unpause(struct vcpu *v);
>  int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
>                             vm_event_request_t *req);
>  
> +#if CONFIG_X86
> +/*
> + * Called for the current vCPU on control-register changes by guest.
> + * The event might not fire if the client has subscribed to it in 
> onchangeonly
> + * mode, hence the bool_t return type for control register write events.
> + */
> +bool_t vm_event_monitor_cr(unsigned int index, unsigned long value,
> +                           unsigned long old);
> +#endif

Same goes for the declaration here.

Jan


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