Re: [Xen-devel] [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.05.15 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, 
> vm_event_request_t *req)
>      return 1;
>  }
>  
> -static inline
> -void hvm_event_cr(uint32_t reason, unsigned long value,
> -                         unsigned long old, bool_t onchangeonly, bool_t sync)
> +void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  {
> -    if ( onchangeonly && value == old )
> +    struct arch_domain *currad = &current->domain->arch;
> +    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
> +
> +    if ( !(currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) )
>          return;
> -    else
> +
> +    if ( !(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
> +         value != old )
>      {

While this is now better than the previous if/else pair, I still don't
see why this can't be just a single if().

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2965,11 +2965,14 @@ out:
>  int hvm_handle_xsetbv(u32 index, u64 new_bv)
>  {
>      struct segment_register sreg;
> +    struct vcpu *curr = current;
>  
> -    hvm_get_segment_register(current, x86_seg_ss, &sreg);
> +    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
>      if ( sreg.attr.fields.dpl != 0 )
>          goto err;
>  
> +    hvm_event_cr(VM_EVENT_X86_XCR0, new_bv, curr->arch.xcr0);
> +
>      if ( handle_xsetbv(index, new_bv) )
>          goto err;

So this is a pre event now, contrary to all others. Is that really a
good idea? And is it a good idea in the first place to introduce new
events in a patch titled "cleanup"?

Also, for readability I wonder whether an identically named macro
wrapper around hvm_event_cr() wouldn't be a good idea, eliminating
the need to spell out VM_EVENT_X86_ at each use site.

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -67,59 +67,41 @@ int monitor_domctl(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>  
>      switch ( mop->event )
>      {
> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0:
> +    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
>      {
> -        bool_t status = ad->monitor.mov_to_cr0_enabled;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        ad->monitor.mov_to_cr0_sync = mop->u.mov_to_cr.sync;
> -        ad->monitor.mov_to_cr0_onchangeonly = mop->u.mov_to_cr.onchangeonly;
> -
> -        domain_pause(d);
> -        ad->monitor.mov_to_cr0_enabled = !status;
> -        domain_unpause(d);
> -        break;
> -    }
> -
> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3:
> -    {
> -        bool_t status = ad->monitor.mov_to_cr3_enabled;
> +        unsigned int ctrlreg_bitmask =
> +            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> +        bool_t status = ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask;

!!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask)

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -249,6 +249,8 @@ struct pv_domain
>      struct mapcache_domain mapcache;
>  };
>  
> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1 << ctrlreg_index)

(1U << (ctrlreg_index)) would seem better to me. Also - does this
need to be in this header (which gets included basically everywhere)?

> @@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
>       */
>      union {
>          struct {
> +            /* Which control register */
> +            uint8_t index;

Okay, 8 bits here (which is reasonable), but ...

> @@ -156,7 +158,8 @@ struct vm_event_mem_access {
>      uint32_t _pad;
>  };
>  
> -struct vm_event_mov_to_cr {
> +struct vm_event_write_ctrlreg {
> +    uint64_t index;
>      uint64_t new_value;
>      uint64_t old_value;
>  };

... a full 64 bits here? 32 bits surely would do (with 32 bits of padding),
allowing slightly better accessing code on both the consumer and
producer sides.

Jan

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