From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE Date: Tue, 7 Jul 2015 17:15:31 +0300 Message-ID: <559BDF03.6040808@bitdefender.com> References: <1436277267-20044-1-git-send-email-rcojocaru@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Lengyel, Tamas" Cc: George Dunlap , Andrew Cooper , keir@xen.org, Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org On 07/07/2015 05:08 PM, Lengyel, Tamas wrote: > > > On Tue, Jul 7, 2015 at 9:54 AM, Razvan Cojocaru > > wrote: > > By naming, placing and bit shift convention, it could be taken as > implied that MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE are > mem_access event specific flags (instead of being generally > applicable as vm_event flags). This patch renames them to > VM_EVENT_FLAG_EMULATE and VM_EVENT_FLAG_EMULATE_NOWRITE > respectively, and uses bit shifts following the rest of the > VM_EVENT_FLAG_ constants. > > Signed-off-by: Razvan Cojocaru > > --- > xen/arch/x86/mm/p2m.c | 4 ++-- > xen/include/public/vm_event.h | 26 +++++++++++++------------- > 2 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 6b39733..4c49369 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1418,7 +1418,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v, > const vm_event_response_t *rsp) > { > /* Mark vcpu for skipping one instruction upon rescheduling. */ > - if ( rsp->flags & MEM_ACCESS_EMULATE ) > + if ( rsp->flags & VM_EVENT_FLAG_EMULATE ) > { > xenmem_access_t access; > bool_t violation = 1; > @@ -1553,7 +1553,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, > unsigned long gla, > if ( v->arch.vm_event.emulate_flags ) > { > hvm_mem_access_emulate_one((v->arch.vm_event.emulate_flags & > - MEM_ACCESS_EMULATE_NOWRITE) != 0, > + VM_EVENT_FLAG_EMULATE_NOWRITE) > != 0, > > > Now looking at this more closely - VM_EVENT_FLAG_EMULATE_NOWRITE doesn't > get set on rsp->flags. As such, it would make sense to keep it as a > separate define (simply VM_EVENT_EMULATE_NOWRITE?). IMHO VM_EVENT_FLAG_* > defines should correspond to things set on the req/rsp->flags field. It does get set, it's just not very obvious, but please see line 1468 below: 1417 void p2m_mem_access_emulate_check(struct vcpu *v, 1418 const vm_event_response_t *rsp) 1419 { 1420 /* Mark vcpu for skipping one instruction upon rescheduling. */ 1421 if ( rsp->flags & MEM_ACCESS_EMULATE ) 1422 { 1423 xenmem_access_t access; 1424 bool_t violation = 1; 1425 const struct vm_event_mem_access *data = &rsp->u.mem_access; 1426 1427 if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 ) 1428 { 1429 switch ( access ) 1430 { 1431 case XENMEM_access_n: 1432 case XENMEM_access_n2rwx: 1433 default: 1434 violation = data->flags & MEM_ACCESS_RWX; 1435 break; [...] 1462 case XENMEM_access_rwx: 1463 violation = 0; 1464 break; 1465 } 1466 } 1467 1468 v->arch.vm_event.emulate_flags = violation ? rsp->flags : 0; 1469 } 1470 } With the right conditions, v->arch.vm_event.emulate_flags is assigned rsp->flags, which does have VM_EVENT_EMULATE_NOWRITE set quite a lot for us. :) Thanks, Razvan