Re: [PATCH V5] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/26/2015 06:47 PM, Jan Beulich wrote:
>>>> On 25.05.15 at 10:33, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -19,6 +19,7 @@
>>  * Place - Suite 330, Boston, MA 02111-1307 USA.
>>  */
>>  
>> +#include <asm/monitor.h>
>>  #include <xen/vm_event.h>
>>  #include <xen/paging.h>
> 
> Just like almost everywhere else, please have asm/ includes follow
> xen/ ones.

Ack. I went the sorted-alphabetically route, obviously that's not the
Xen convention.

>> @@ -3349,7 +3354,7 @@ int hvm_set_cr4(unsigned long value)
>>      }
>>  
>>      hvm_update_cr(v, 4, value);
>> -    hvm_event_cr4(value, old_cr);
>> +    hvm_event_cr(CR4, value, old_cr);
> 
> Considering e.g. this one, ...
> 
>> @@ -2010,7 +2012,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);
> 
> ... does this build at all? Ah - you put the wrapping #define into
> xen/arch/x86/hvm/hvm.c instead of right alongside the function
> declaration. That's not how I would think this should be done -
> either use the wrapper everywhere, or nowhere.

Ack.

>> @@ -156,14 +158,15 @@ struct vm_event_mem_access {
>>      uint32_t _pad;
>>  };
>>  
>> -struct vm_event_mov_to_cr {
>> +struct vm_event_write_ctrlreg {
>> +    uint32_t index;
>>      uint64_t new_value;
>>      uint64_t old_value;
>> +    uint32_t _pad;
>>  };
> 
> Why would you want to add padding anywhere other than between
> index and new_value?

Ack, will move the _pad.


Thanks,
Razvan