From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h Date: Fri, 14 Aug 2015 13:30:31 +0300 Message-ID: <55CDC347.7090400@bitdefender.com> References: <1439546073-13836-1-git-send-email-rcojocaru@bitdefender.com> <55CDDD5F020000780009AF78@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZQCEQ-0001QY-6K for xen-devel@lists.xenproject.org; Fri, 14 Aug 2015 10:29:02 +0000 Received: from smtp01.buh.bitdefender.com (unknown [10.17.80.75]) by mx-sr.buh.bitdefender.com (Postfix) with ESMTP id 0E87480513 for ; Fri, 14 Aug 2015 13:28:58 +0300 (EEST) In-Reply-To: <55CDDD5F020000780009AF78@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, tamas@tklengyel.com, keir@xen.org, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 08/14/2015 01:21 PM, Jan Beulich wrote: >>>> On 14.08.15 at 11:54, 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 >> #include >> #include >> +#include > > 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