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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/22/2015 10:36 AM, Jan Beulich wrote:
>>>> On 21.05.15 at 15:00, <rcojocaru@bitdefender.com> 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().

Ack, will change it to 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"?

While it is indeed different from the rest of the CR events in that it
is a pre-write, it is not however different from the MSR event, which is
currently pre-write.

As for introducing the new event, it's a trivial one-line change if we
don't count the VM_EVENT_X86_XCR0 constant and the extra bit in the
enabled, sync and onchangeonly fields.

Should I drop the introduction of the XR0 event from this patch and come
back to it in the following iteration of the series?

> 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.

#define hvm_event_cr0(new, old) hvm_event_cr(VM_EVENT_X86_XCR0, new,
old)? Sure.

>> --- 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)

Ack.

>> --- 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)?

Ack. As for the header, that's where the fields are
(write_ctrlreg_enabled, sync and onchangeonly), and since several .c
files use the fields it seemed the natural place to put the macros.

>> @@ -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.

Ack, will modify it. I set it to 64 bits there because I thought I'd get
free padding, but you're right, it's better to be explicit.


Thanks,
Razvan