Re: [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/14/2015 01:21 PM, Jan Beulich wrote:
>>>> On 14.08.15 at 11:54, <rcojocaru@bitdefender.com> wrote:
>> @@ -571,7 +571,7 @@ void hvm_do_resume(struct vcpu *v)
>>      }
>>  
>>      /* Inject pending hw/sw trap */
>> -    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) 
>> +    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
> 
> Stray whitespace change to unrelated code.

Understood, thought I'd remove the trailing whitespace there, but I will
leave the unrelated code alone.

>> @@ -3371,13 +3371,13 @@ int hvm_set_cr0(unsigned long value, bool_t 
>> may_defer)
>>                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) &&
>>           value != old_value )
>>      {
>> -        ASSERT(currad->event_write_data != NULL);
>> +        ASSERT(v->arch.vm_event != NULL);
> 
> May I recommend dropping the redundant != NULL namely in ASSERT()
> expressions (the only effect they have is produce larger string literals
> in debug builds).

Of course, I'll change it.

>>          if ( hvm_event_crX(CR0, value, old_value) )
>>          {
>>              /* The actual write will occur in hvm_do_resume(), if permitted. */
>> -            currad->event_write_data[v->vcpu_id].do_write.cr0 = 1;
>> -            currad->event_write_data[v->vcpu_id].cr0 = value;
>> +            v->arch.vm_event->write_data.do_write.cr0 = 1;
>> +            v->arch.vm_event->write_data.cr0 = value;
> 
> Looks like this leaves just a single use of currad in the function, in
> which case I'd like to see the variable go away.

I'll remove it.

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -8,6 +8,7 @@
>>  #include <asm/hvm/domain.h>
>>  #include <asm/e820.h>
>>  #include <asm/mce.h>
>> +#include <public/vm_event.h>
> 
> It looks like both this and ...
> 
>> @@ -460,6 +459,18 @@ typedef enum __packed {
>>      SMAP_CHECK_DISABLED,        /* disable the check */
>>  } smap_check_policy_t;
>>  
>> +/*
>> + * Should we emulate the next matching instruction on VCPU resume
>> + * after a vm_event?
>> + */
>> +struct vm_event {
>> +    uint32_t emulate_flags;
>> +    unsigned long gpa;
>> +    unsigned long eip;
>> +    struct vm_event_emul_read_data emul_read_data;
>> +    struct monitor_write_data write_data;
>> +};
> 
> ... this would better go into asm-x86/vm_event.h (despite it meaning
> that the file will then be included by basically everything).

Ack. Thank you for the prompt review!


Thanks,
Razvan