[PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[Razvan Cojocaru]

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>
---
 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,
                                    TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
 
         v->arch.vm_event.emulate_flags = 0;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 577e971..aa22052 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -47,6 +47,19 @@
 #define VM_EVENT_FLAG_VCPU_PAUSED     (1 << 0)
 /* Flags to aid debugging mem_event */
 #define VM_EVENT_FLAG_FOREIGN         (1 << 1)
+/*
+ * The following flags can be set in response to a mem_access event.
+ *
+ * Emulate the fault-causing instruction (if set in the event response flags).
+ * This will allow the guest to continue execution without lifting the page
+ * access restrictions.
+ */
+#define VM_EVENT_FLAG_EMULATE         (1 << 2)
+/*
+ * Same as MEM_ACCESS_EMULATE, but with write operations or operations
+ * potentially having side effects (like memory mapped or port I/O) disabled.
+ */
+#define VM_EVENT_FLAG_EMULATE_NOWRITE (1 << 3)
 
 /*
  * Reasons for the vm event request
@@ -136,19 +149,6 @@ struct vm_event_regs_x86 {
 #define MEM_ACCESS_GLA_VALID            (1 << 3)
 #define MEM_ACCESS_FAULT_WITH_GLA       (1 << 4)
 #define MEM_ACCESS_FAULT_IN_GPT         (1 << 5)
-/*
- * The following flags can be set in the response.
- *
- * Emulate the fault-causing instruction (if set in the event response flags).
- * This will allow the guest to continue execution without lifting the page
- * access restrictions.
- */
-#define MEM_ACCESS_EMULATE              (1 << 6)
-/*
- * Same as MEM_ACCESS_EMULATE, but with write operations or operations
- * potentially having side effects (like memory mapped or port I/O) disabled.
- */
-#define MEM_ACCESS_EMULATE_NOWRITE      (1 << 7)
 
 struct vm_event_mem_access {
     uint64_t gfn;
-- 
1.7.9.5

Re: [PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]

On Tue, Jul 7, 2015 at 9:54 AM, Razvan Cojocaru <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>
> ---
>  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.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2486 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[Razvan Cojocaru]

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

Re: [PATCH] vm_event: Rename MEM_ACCESS_EMULATE and MEM_ACCESS_EMULATE_NOWRITE

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1792 bytes --]

> > 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. :)


Ah OK. Maybe we don't need to copy the entire rsp->flags, only the emulate
related bits? But that's beyond the scope of this patch so:

Acked-by: Tamas K Lengyel <tlengyel@novetta.com>

[-- Attachment #1.2: Type: text/html, Size: 2342 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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