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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> wrote:
>> The CR0, CR3 and CR4 events are
>> now pre-write vm_events.
> 
> I didn't get the impression you and Tamas had already settled on
> whether this is the way to go forward.

I took Tamas' previous comment ("IMHO there is not much point in having
two distinct event types (PRE/POST).") to mean that he agrees that if we
aim for consistency, we need to have the CR and MSR events behave the
same way.

Tamas did indeed question whether or not post-write (vs. pre-write) CR
events matter from the guest perspective, and I've explained why this
does matter to the monitoring application, from both an architectural
and efficiency perspective.

Of course, if there is more to discuss, by all means. We should all
agree on how to go forward. Tamas, did I misunderstand your position?

>> --- 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 short index, unsigned long value, unsigned long old)
> 
> What's the point of using "unsigned short" here?

As opposed to an uint8_t? This would allow the hvm_event_cr() signature
to remain unmodified longer if more than 4 control register events are
added in the future, and it is the least wasteful of the
bigger-than-char-sized integers.

>>  {
>> -    if ( onchangeonly && value == old )
>> +    struct arch_domain *currad = &current->domain->arch;
>> +
>> +    if ( !(currad->monitor.write_ctrlreg_enabled & index) )
>> +        return;
>> +
>> +    if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == old )
>>          return;
>>      else
>>      {
> 
> Considering that nothing follows this else block, please invert the
> condition and avoid both return and else.

Ack.

>> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>>          goto gpf;
>>      }
>>  
>> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
>> +
>>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>>      {
>>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
> 
> If the monitor's response isn't being checked (i.e. "deny" not [yet]
> being honored), what's the point of moving the generation of the
> event in this patch (which would be involved enough without these
> extra adjustments)?

Consistency. Since the patch concerns itself with cleaning up the
control register events a bit, it didn't seem too far-fetched to assume
that having the control register write vm_events sent at the same place
as the MSR events fits the concept.

But I am happy to do this in the DENY patch in the next iteration of the
series if so requested.

>> @@ -3287,7 +3291,9 @@ int hvm_set_cr3(unsigned long value)
>>  {
>>      struct vcpu *v = current;
>>      struct page_info *page;
>> -    unsigned long old;
>> +    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
>> +
>> +    hvm_event_cr(VM_EVENT_X86_CR3, value, old);
> 
> I don't think the local variable is warranted anymore with the moved
> event generation point.

Ack.

>> @@ -2010,7 +2010,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
>>          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
>>          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>>          vmx_update_guest_cr(curr, 0);
>> -        hvm_event_cr0(curr->arch.hvm_vcpu.guest_cr[0], old);
>> +        hvm_event_cr(VM_EVENT_X86_CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
> 
> Why is this not becoming a pre event?

It should, I've missed that. Thank you for noticing!
Will fix it.

>> +/* Supported values for the vm_event_write_ctrlreg index. */
>> +#define VM_EVENT_X86_CR0     (1 << 0)
>> +#define VM_EVENT_X86_CR3     (1 << 1)
>> +#define VM_EVENT_X86_CR4     (1 << 2)
>> +#define VM_EVENT_X86_XCR0    (1 << 3)
> 
> Why bit masks rather than an enumeration like thing?

Ack, will change it to an enum. That would have been my first preference
too, but the header just seemed to be more #define-oriented and I tried
to follow suit.

>> @@ -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;
> 
> In particular here - what meaning would it have if there was more
> than one bit set?

Ack, will change the constants to members of an enum.


Thanks,
Razvan