[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
  • Date: Fri, 14 Aug 2015 13:30:31 +0300
  • Cc: george.dunlap@xxxxxxxxxxxxx, andrew.cooper3@xxxxxxxxxx, tamas@xxxxxxxxxxxxx, keir@xxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Comment: DomainKeys? See http://domainkeys.sourceforge.net/
  • Delivery-date: Fri, 14 Aug 2015 10:29:08 +0000
  • Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=bitdefender.com; b=lXuctsIZ/jo/ONqxSNJEmRj/0P4ZOjkmg1DCDjvStLps+F3Y5TeZs0MKKxKLew8El5zDeYlN4bCrhKN2car8SwIPYj3DUDLGlR9tmZxDz62kukdzzLENsG9fk62QuoY1c1eNGDIkvfXDKXCwWKoGNwdYKY8lXmCjGiAuUawfQ3l/ElKNsdq7vD8p7iaZWn16SLV6EbgcOcI7w+r4ZG2CBocido6DZ0WsqUoI0WsKQvtYy4ROEYN/odmxKDWE9KxdRrydtWPuC/yDAktByqOoyWaYGITQ2GZaJ1CKHrwaCD4jFgJyBrVd5uynNI0ar0NEprGviVgt3DDwdxF4DHm0fg==; h=Received:Received:Received:Received:Received:From:Subject:To:References:Cc:Message-ID:Date:User-Agent:MIME-Version:In-Reply-To:Content-Type:Content-Transfer-Encoding:X-BitDefender-Scanner:X-BitDefender-Spam:X-BitDefender-SpamStamp:X-BitDefender-CF-Stamp;
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

On 08/14/2015 01:21 PM, Jan Beulich wrote:
>>>> On 14.08.15 at 11:54, <rcojocaru@xxxxxxxxxxxxxxx> 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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.