Re: [PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/07/2015 05:08 PM, Lengyel, Tamas wrote:
> 
> 
> On Tue, Jul 7, 2015 at 9:54 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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 <rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>
>     ---
>      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