[PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Alexandru Isaila]

This patch is adding a way to enable/disable nested pagefault
events. It introduces the xc_monitor_nested_pagefault function
and adds the nested_pagefault_disabled in the monitor structure.
This is needed by the introspection so it will only get gla
faults and not get spammed with other faults.
In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
v->arch.sse_pg_dirty.gla are used to mark that this is the
second time a fault occurs and the dirty bit is set.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V1:
        - Rb V1
	- Add comment in domctl.h
---
 tools/libxc/include/xenctrl.h |  2 ++
 tools/libxc/xc_monitor.c      | 14 ++++++++++++++
 xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
 xen/arch/x86/monitor.c        | 13 +++++++++++++
 xen/include/asm-x86/domain.h  |  6 ++++++
 xen/include/asm-x86/monitor.h |  3 ++-
 xen/include/public/domctl.h   |  2 ++
 7 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 09e1363..112c974 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
                                  bool enable);
 int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
                              bool enable, bool sync, bool allow_userspace);
+int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
+                                bool disable);
 int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
                                 bool enable, bool sync);
 int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 0233b87..e96c56d 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
     return do_domctl(xch, &domctl);
 }
 
+int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
+                                bool disable)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
                                 bool enable)
 {
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c0cd017..07a334b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
     return violation;
 }
 
+static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
+{
+    struct hvm_hw_cpu ctxt;
+    uint32_t pfec = 0;
+
+    hvm_funcs.save_cpu_ctxt(v, &ctxt);
+
+    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
+         && ga == v->arch.pg_dirty.gla )
+        pfec = PFEC_write_access;
+
+    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
+
+    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
+    v->arch.pg_dirty.gla = ga;
+}
+
 bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
                           struct npfec npfec,
                           vm_event_request_t **req_ptr)
@@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         }
     }
 
+    if ( vm_event_check_ring(d->vm_event_monitor) &&
+         d->arch.monitor.nested_pagefault_disabled &&
+         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
+    {
+        v->arch.vm_event->emulate_flags = 0;
+        p2m_set_ad_bits(v, gla);
+
+        return true;
+    }
+
     *req_ptr = NULL;
     req = xzalloc(vm_event_request_t);
     if ( req )
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index f229e69..e35b619 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
+    {
+        bool old_status = ad->monitor.nested_pagefault_disabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.nested_pagefault_disabled = requested_status;
+        domain_unpause(d);
+        break;
+    }
+
     case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
     {
         bool old_status = ad->monitor.descriptor_access_enabled;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4679d54..099af7c 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -412,6 +412,7 @@ struct arch_domain
         unsigned int descriptor_access_enabled                             : 1;
         unsigned int guest_request_userspace_enabled                       : 1;
         unsigned int emul_unimplemented_enabled                            : 1;
+        unsigned int nested_pagefault_disabled                             : 1;
         struct monitor_msr_bitmap *msr_bitmap;
         uint64_t write_ctrlreg_mask[4];
     } monitor;
@@ -579,6 +580,11 @@ struct arch_vcpu
     /* A secondary copy of the vcpu time info. */
     XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
 
+    struct {
+        unsigned long eip;
+        unsigned long gla;
+    } pg_dirty;
+
     struct arch_vm_event *vm_event;
 
     struct msr_vcpu_policy *msr;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index a0444d1..a7a0b56 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -84,7 +84,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
                    (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT);
 
     /* Since we know this is on VMX, we can just call the hvm func */
     if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 9ae72959..706c6ea 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1014,6 +1014,8 @@ struct xen_domctl_psr_cmt_op {
 #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
 #define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
 #define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED    10
+/* Enabled by default */
+#define XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT      11
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Alexandru Stefan ISAILA]

Any thoughts are appreciated.

> This patch is adding a way to enable/disable nested pagefault
> events. It introduces the xc_monitor_nested_pagefault function
> and adds the nested_pagefault_disabled in the monitor structure.
> This is needed by the introspection so it will only get gla
> faults and not get spammed with other faults.
> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
> v->arch.sse_pg_dirty.gla are used to mark that this is the
> second time a fault occurs and the dirty bit is set.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V1:
>         - Rb V1
> - Add comment in domctl.h
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>  xen/include/asm-x86/domain.h  |  6 ++++++
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  2 ++
>  7 files changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/include/xenctrl.h
> b/tools/libxc/include/xenctrl.h
> index 09e1363..112c974 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface
> *xch, uint32_t domain_id,
>                                   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>                               bool enable, bool sync, bool
> allow_userspace);
> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t
> domain_id,
> +                                bool disable);
>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t
> domain_id,
>                                  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool
> enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 0233b87..e96c56d 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch,
> uint32_t domain_id, bool enable,
>      return do_domctl(xch, &domctl);
>  }
>
> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t
> domain_id,
> +                                bool disable)
> +{
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
> +    domctl.u.monitor_op.event =
> XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
> +
> +    return do_domctl(xch, &domctl);
> +}
> +
>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t
> domain_id,
>                                  bool enable)
>  {
> diff --git a/xen/arch/x86/mm/mem_access.c
> b/xen/arch/x86/mm/mem_access.c
> index c0cd017..07a334b 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu
> *v,
>      return violation;
>  }
>
> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
> +{
> +    struct hvm_hw_cpu ctxt;
> +    uint32_t pfec = 0;
> +
> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
> +
> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
> +         && ga == v->arch.pg_dirty.gla )
> +        pfec = PFEC_write_access;
> +
> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
> +
> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
> +    v->arch.pg_dirty.gla = ga;
> +}
> +
>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>                            struct npfec npfec,
>                            vm_event_request_t **req_ptr)
> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned
> long gla,
>          }
>      }
>
> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
> +         d->arch.monitor.nested_pagefault_disabled &&
> +         npfec.kind != npfec_kind_with_gla ) /* don't send a
> mem_event */
> +    {
> +        v->arch.vm_event->emulate_flags = 0;
> +        p2m_set_ad_bits(v, gla);
> +
> +        return true;
> +    }
> +
>      *req_ptr = NULL;
>      req = xzalloc(vm_event_request_t);
>      if ( req )
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index f229e69..e35b619 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>          break;
>      }
>
> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
> +    {
> +        bool old_status = ad->monitor.nested_pagefault_disabled;
> +
> +        if ( unlikely(old_status == requested_status) )
> +            return -EEXIST;
> +
> +        domain_pause(d);
> +        ad->monitor.nested_pagefault_disabled = requested_status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>      {
>          bool old_status = ad->monitor.descriptor_access_enabled;
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-
> x86/domain.h
> index 4679d54..099af7c 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -412,6 +412,7 @@ struct arch_domain
>          unsigned int
> descriptor_access_enabled                             : 1;
>          unsigned int
> guest_request_userspace_enabled                       : 1;
>          unsigned int
> emul_unimplemented_enabled                            : 1;
> +        unsigned int
> nested_pagefault_disabled                             : 1;
>          struct monitor_msr_bitmap *msr_bitmap;
>          uint64_t write_ctrlreg_mask[4];
>      } monitor;
> @@ -579,6 +580,11 @@ struct arch_vcpu
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>
> +    struct {
> +        unsigned long eip;
> +        unsigned long gla;
> +    } pg_dirty;
> +
>      struct arch_vm_event *vm_event;
>
>      struct msr_vcpu_policy *msr;
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-
> x86/monitor.h
> index a0444d1..a7a0b56 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -84,7 +84,8 @@ static inline uint32_t
> arch_monitor_get_capabilities(struct domain *d)
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION)
> |
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
> -                   (1U <<
> XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
> +                   (1U <<
> XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED) |
> +                   (1U <<
> XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT);
>
>      /* Since we know this is on VMX, we can just call the hvm func
> */
>      if ( hvm_is_singlestep_supported() )
> diff --git a/xen/include/public/domctl.h
> b/xen/include/public/domctl.h
> index 9ae72959..706c6ea 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1014,6 +1014,8 @@ struct xen_domctl_psr_cmt_op {
>  #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
>  #define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
>  #define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED    10
> +/* Enabled by default */
> +#define XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT      11
>
>  struct xen_domctl_monitor_op {
>      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Wei Liu]

On Mon, Jan 08, 2018 at 02:49:44PM +0200, Alexandru Isaila wrote:
> This patch is adding a way to enable/disable nested pagefault
> events. It introduces the xc_monitor_nested_pagefault function
> and adds the nested_pagefault_disabled in the monitor structure.
> This is needed by the introspection so it will only get gla
> faults and not get spammed with other faults.
> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
> v->arch.sse_pg_dirty.gla are used to mark that this is the
> second time a fault occurs and the dirty bit is set.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V1:
>         - Rb V1
> 	- Add comment in domctl.h
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++

These changes look sensible to me.

>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>  xen/include/asm-x86/domain.h  |  6 ++++++
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  2 ++

You might want to bump domctl version number it is has been done already
in this release.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Jan Beulich]

>>> On 23.02.18 at 18:46, <wei.liu2@citrix.com> wrote:
> On Mon, Jan 08, 2018 at 02:49:44PM +0200, Alexandru Isaila wrote:
>> ---
>>  tools/libxc/include/xenctrl.h |  2 ++
>>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
> 
> These changes look sensible to me.
> 
>>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>>  xen/include/asm-x86/domain.h  |  6 ++++++
>>  xen/include/asm-x86/monitor.h |  3 ++-
>>  xen/include/public/domctl.h   |  2 ++
> 
> You might want to bump domctl version number it is has been done already
> in this release.

For one this sentence confuses me by its wording alone. And then
all the patch does is add a #define, which doesn't alter the
interface in an incompatible way (which is what the interface
version tries to express, even if - I agree with Andrew here -
limitations).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Tamas K Lengyel]

On Mon, Jan 8, 2018 at 5:49 AM, Alexandru Isaila
<aisaila@bitdefender.com> wrote:
> This patch is adding a way to enable/disable nested pagefault
> events. It introduces the xc_monitor_nested_pagefault function
> and adds the nested_pagefault_disabled in the monitor structure.
> This is needed by the introspection so it will only get gla
> faults and not get spammed with other faults.
> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
> v->arch.sse_pg_dirty.gla are used to mark that this is the
> second time a fault occurs and the dirty bit is set.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V1:
>         - Rb V1
>         - Add comment in domctl.h
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>  xen/include/asm-x86/domain.h  |  6 ++++++
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  2 ++
>  7 files changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 09e1363..112c974 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
>                                   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>                               bool enable, bool sync, bool allow_userspace);
> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
> +                                bool disable);
>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
>                                  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 0233b87..e96c56d 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
>      return do_domctl(xch, &domctl);
>  }
>
> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
> +                                bool disable)
> +{
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
> +    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
> +
> +    return do_domctl(xch, &domctl);
> +}
> +
>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
>                                  bool enable)
>  {
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c0cd017..07a334b 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>      return violation;
>  }
>
> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
> +{
> +    struct hvm_hw_cpu ctxt;
> +    uint32_t pfec = 0;
> +
> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
> +
> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
> +         && ga == v->arch.pg_dirty.gla )
> +        pfec = PFEC_write_access;
> +
> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
> +
> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
> +    v->arch.pg_dirty.gla = ga;
> +}
> +
>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>                            struct npfec npfec,
>                            vm_event_request_t **req_ptr)
> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>          }
>      }
>
> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
> +         d->arch.monitor.nested_pagefault_disabled &&
> +         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
> +    {
> +        v->arch.vm_event->emulate_flags = 0;
> +        p2m_set_ad_bits(v, gla);
> +
> +        return true;
> +    }
> +
>      *req_ptr = NULL;
>      req = xzalloc(vm_event_request_t);
>      if ( req )
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index f229e69..e35b619 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>          break;
>      }
>
> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
> +    {
> +        bool old_status = ad->monitor.nested_pagefault_disabled;
> +
> +        if ( unlikely(old_status == requested_status) )
> +            return -EEXIST;
> +
> +        domain_pause(d);
> +        ad->monitor.nested_pagefault_disabled = requested_status;
> +        domain_unpause(d);
> +        break;
> +    }
> +
>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>      {
>          bool old_status = ad->monitor.descriptor_access_enabled;
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 4679d54..099af7c 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -412,6 +412,7 @@ struct arch_domain
>          unsigned int descriptor_access_enabled                             : 1;
>          unsigned int guest_request_userspace_enabled                       : 1;
>          unsigned int emul_unimplemented_enabled                            : 1;
> +        unsigned int nested_pagefault_disabled                             : 1;

All other options are "_enabled" here, so adding one that's flipped
just looks out of place. Any objections to making this match the rest?
Also, naming it "nested" just makes me think this is somehow would be
related to nested virtualization, but that's not the case. These would
be just regular pagefaults in the guest, so naming the monitor option
simply "pagefault" would look better to me in general.

>          struct monitor_msr_bitmap *msr_bitmap;
>          uint64_t write_ctrlreg_mask[4];
>      } monitor;
> @@ -579,6 +580,11 @@ struct arch_vcpu
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>
> +    struct {
> +        unsigned long eip;
> +        unsigned long gla;
> +    } pg_dirty;
> +
>      struct arch_vm_event *vm_event;
>
>      struct msr_vcpu_policy *msr;
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index a0444d1..a7a0b56 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -84,7 +84,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
>                     (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
> -                   (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED);
> +                   (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED) |
> +                   (1U << XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT);
>
>      /* Since we know this is on VMX, we can just call the hvm func */
>      if ( hvm_is_singlestep_supported() )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 9ae72959..706c6ea 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1014,6 +1014,8 @@ struct xen_domctl_psr_cmt_op {
>  #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
>  #define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
>  #define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED    10
> +/* Enabled by default */
> +#define XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT      11
>
>  struct xen_domctl_monitor_op {
>      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> --
> 2.7.4

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Razvan Cojocaru]

On 02/24/2018 12:06 AM, Tamas K Lengyel wrote:
> On Mon, Jan 8, 2018 at 5:49 AM, Alexandru Isaila
> <aisaila@bitdefender.com> wrote:
>> This patch is adding a way to enable/disable nested pagefault
>> events. It introduces the xc_monitor_nested_pagefault function
>> and adds the nested_pagefault_disabled in the monitor structure.
>> This is needed by the introspection so it will only get gla
>> faults and not get spammed with other faults.
>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>> second time a fault occurs and the dirty bit is set.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>> ---
>> Changes since V1:
>>         - Rb V1
>>         - Add comment in domctl.h
>> ---
>>  tools/libxc/include/xenctrl.h |  2 ++
>>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>>  xen/include/asm-x86/domain.h  |  6 ++++++
>>  xen/include/asm-x86/monitor.h |  3 ++-
>>  xen/include/public/domctl.h   |  2 ++
>>  7 files changed, 66 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 09e1363..112c974 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
>>                                   bool enable);
>>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>>                               bool enable, bool sync, bool allow_userspace);
>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>> +                                bool disable);
>>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
>>                                  bool enable, bool sync);
>>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>> index 0233b87..e96c56d 100644
>> --- a/tools/libxc/xc_monitor.c
>> +++ b/tools/libxc/xc_monitor.c
>> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
>>      return do_domctl(xch, &domctl);
>>  }
>>
>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>> +                                bool disable)
>> +{
>> +    DECLARE_DOMCTL;
>> +
>> +    domctl.cmd = XEN_DOMCTL_monitor_op;
>> +    domctl.domain = domain_id;
>> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
>> +    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
>> +
>> +    return do_domctl(xch, &domctl);
>> +}
>> +
>>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
>>                                  bool enable)
>>  {
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index c0cd017..07a334b 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>>      return violation;
>>  }
>>
>> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
>> +{
>> +    struct hvm_hw_cpu ctxt;
>> +    uint32_t pfec = 0;
>> +
>> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
>> +
>> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
>> +         && ga == v->arch.pg_dirty.gla )
>> +        pfec = PFEC_write_access;
>> +
>> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
>> +
>> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
>> +    v->arch.pg_dirty.gla = ga;
>> +}
>> +
>>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>                            struct npfec npfec,
>>                            vm_event_request_t **req_ptr)
>> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>          }
>>      }
>>
>> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
>> +         d->arch.monitor.nested_pagefault_disabled &&
>> +         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>> +    {
>> +        v->arch.vm_event->emulate_flags = 0;
>> +        p2m_set_ad_bits(v, gla);
>> +
>> +        return true;
>> +    }
>> +
>>      *req_ptr = NULL;
>>      req = xzalloc(vm_event_request_t);
>>      if ( req )
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index f229e69..e35b619 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>>          break;
>>      }
>>
>> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
>> +    {
>> +        bool old_status = ad->monitor.nested_pagefault_disabled;
>> +
>> +        if ( unlikely(old_status == requested_status) )
>> +            return -EEXIST;
>> +
>> +        domain_pause(d);
>> +        ad->monitor.nested_pagefault_disabled = requested_status;
>> +        domain_unpause(d);
>> +        break;
>> +    }
>> +
>>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>>      {
>>          bool old_status = ad->monitor.descriptor_access_enabled;
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index 4679d54..099af7c 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -412,6 +412,7 @@ struct arch_domain
>>          unsigned int descriptor_access_enabled                             : 1;
>>          unsigned int guest_request_userspace_enabled                       : 1;
>>          unsigned int emul_unimplemented_enabled                            : 1;
>> +        unsigned int nested_pagefault_disabled                             : 1;
> 
> All other options are "_enabled" here, so adding one that's flipped
> just looks out of place. Any objections to making this match the rest?
> Also, naming it "nested" just makes me think this is somehow would be
> related to nested virtualization, but that's not the case. These would
> be just regular pagefaults in the guest, so naming the monitor option
> simply "pagefault" would look better to me in general.
Hello Tamas,

Here's the thinking behind preferring "disabled" to "enabled": we want
to keep the default behaviour as it is currently, and the current
behaviour is to send out _all_ EPT fault vm_events (caused by page walks
or not).

Now, struct arch_domain is being zeroed out on init, so if we name this
"enabled", then that's the behaviour we're starting out with. We have no
problem with that, but it changes the current default behaviour.

So either we name this new field "disabled", or we rename it to
"enabled" (if we rename it, we either need to set it as a special case
on init, or modify the default behaviour to be _not_ sending out
page-walk-caused EPT events).

If you feel strongly about options 2.A or 2.C we don't have a problem
changing the code.

About "pagefault", it reads more confusing to me, since all EPT-related
vm_events are basically page faults. But maybe that's just me.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Tamas K Lengyel]

On Fri, Feb 23, 2018 at 3:25 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 02/24/2018 12:06 AM, Tamas K Lengyel wrote:
>> On Mon, Jan 8, 2018 at 5:49 AM, Alexandru Isaila
>> <aisaila@bitdefender.com> wrote:
>>> This patch is adding a way to enable/disable nested pagefault
>>> events. It introduces the xc_monitor_nested_pagefault function
>>> and adds the nested_pagefault_disabled in the monitor structure.
>>> This is needed by the introspection so it will only get gla
>>> faults and not get spammed with other faults.
>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>> second time a fault occurs and the dirty bit is set.
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>
>>> ---
>>> Changes since V1:
>>>         - Rb V1
>>>         - Add comment in domctl.h
>>> ---
>>>  tools/libxc/include/xenctrl.h |  2 ++
>>>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>>>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>>>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>>>  xen/include/asm-x86/domain.h  |  6 ++++++
>>>  xen/include/asm-x86/monitor.h |  3 ++-
>>>  xen/include/public/domctl.h   |  2 ++
>>>  7 files changed, 66 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 09e1363..112c974 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
>>>                                   bool enable);
>>>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>>>                               bool enable, bool sync, bool allow_userspace);
>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>> +                                bool disable);
>>>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
>>>                                  bool enable, bool sync);
>>>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
>>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>>> index 0233b87..e96c56d 100644
>>> --- a/tools/libxc/xc_monitor.c
>>> +++ b/tools/libxc/xc_monitor.c
>>> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
>>>      return do_domctl(xch, &domctl);
>>>  }
>>>
>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>> +                                bool disable)
>>> +{
>>> +    DECLARE_DOMCTL;
>>> +
>>> +    domctl.cmd = XEN_DOMCTL_monitor_op;
>>> +    domctl.domain = domain_id;
>>> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>>> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
>>> +    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
>>> +
>>> +    return do_domctl(xch, &domctl);
>>> +}
>>> +
>>>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
>>>                                  bool enable)
>>>  {
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..07a334b 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>>>      return violation;
>>>  }
>>>
>>> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
>>> +{
>>> +    struct hvm_hw_cpu ctxt;
>>> +    uint32_t pfec = 0;
>>> +
>>> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
>>> +
>>> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
>>> +         && ga == v->arch.pg_dirty.gla )
>>> +        pfec = PFEC_write_access;
>>> +
>>> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
>>> +
>>> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
>>> +    v->arch.pg_dirty.gla = ga;
>>> +}
>>> +
>>>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>                            struct npfec npfec,
>>>                            vm_event_request_t **req_ptr)
>>> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>          }
>>>      }
>>>
>>> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
>>> +         d->arch.monitor.nested_pagefault_disabled &&
>>> +         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>>> +    {
>>> +        v->arch.vm_event->emulate_flags = 0;
>>> +        p2m_set_ad_bits(v, gla);
>>> +
>>> +        return true;
>>> +    }
>>> +
>>>      *req_ptr = NULL;
>>>      req = xzalloc(vm_event_request_t);
>>>      if ( req )
>>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>>> index f229e69..e35b619 100644
>>> --- a/xen/arch/x86/monitor.c
>>> +++ b/xen/arch/x86/monitor.c
>>> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>>>          break;
>>>      }
>>>
>>> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
>>> +    {
>>> +        bool old_status = ad->monitor.nested_pagefault_disabled;
>>> +
>>> +        if ( unlikely(old_status == requested_status) )
>>> +            return -EEXIST;
>>> +
>>> +        domain_pause(d);
>>> +        ad->monitor.nested_pagefault_disabled = requested_status;
>>> +        domain_unpause(d);
>>> +        break;
>>> +    }
>>> +
>>>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>>>      {
>>>          bool old_status = ad->monitor.descriptor_access_enabled;
>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>> index 4679d54..099af7c 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -412,6 +412,7 @@ struct arch_domain
>>>          unsigned int descriptor_access_enabled                             : 1;
>>>          unsigned int guest_request_userspace_enabled                       : 1;
>>>          unsigned int emul_unimplemented_enabled                            : 1;
>>> +        unsigned int nested_pagefault_disabled                             : 1;
>>
>> All other options are "_enabled" here, so adding one that's flipped
>> just looks out of place. Any objections to making this match the rest?
>> Also, naming it "nested" just makes me think this is somehow would be
>> related to nested virtualization, but that's not the case. These would
>> be just regular pagefaults in the guest, so naming the monitor option
>> simply "pagefault" would look better to me in general.
> Hello Tamas,
>
> Here's the thinking behind preferring "disabled" to "enabled": we want
> to keep the default behaviour as it is currently, and the current
> behaviour is to send out _all_ EPT fault vm_events (caused by page walks
> or not).
>
> Now, struct arch_domain is being zeroed out on init, so if we name this
> "enabled", then that's the behaviour we're starting out with. We have no
> problem with that, but it changes the current default behaviour.

We can keep the "disabled" naming but then please add a comment to the field
saying that by default all events are sent, this is used to filter
pagefaults out.

>
> So either we name this new field "disabled", or we rename it to
> "enabled" (if we rename it, we either need to set it as a special case
> on init, or modify the default behaviour to be _not_ sending out
> page-walk-caused EPT events).
>
> If you feel strongly about options 2.A or 2.C we don't have a problem
> changing the code.
>
> About "pagefault", it reads more confusing to me, since all EPT-related
> vm_events are basically page faults. But maybe that's just me.

True. It's just confusing with "nested" also having multiple meanings.
Perhaps "inguest_pagefaults"?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Razvan Cojocaru]

On 02/24/2018 12:31 AM, Tamas K Lengyel wrote:
> On Fri, Feb 23, 2018 at 3:25 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 02/24/2018 12:06 AM, Tamas K Lengyel wrote:
>>> On Mon, Jan 8, 2018 at 5:49 AM, Alexandru Isaila
>>> <aisaila@bitdefender.com> wrote:
>>>> This patch is adding a way to enable/disable nested pagefault
>>>> events. It introduces the xc_monitor_nested_pagefault function
>>>> and adds the nested_pagefault_disabled in the monitor structure.
>>>> This is needed by the introspection so it will only get gla
>>>> faults and not get spammed with other faults.
>>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>>> second time a fault occurs and the dirty bit is set.
>>>>
>>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>>
>>>> ---
>>>> Changes since V1:
>>>>         - Rb V1
>>>>         - Add comment in domctl.h
>>>> ---
>>>>  tools/libxc/include/xenctrl.h |  2 ++
>>>>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>>>>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>>>>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>>>>  xen/include/asm-x86/domain.h  |  6 ++++++
>>>>  xen/include/asm-x86/monitor.h |  3 ++-
>>>>  xen/include/public/domctl.h   |  2 ++
>>>>  7 files changed, 66 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>>> index 09e1363..112c974 100644
>>>> --- a/tools/libxc/include/xenctrl.h
>>>> +++ b/tools/libxc/include/xenctrl.h
>>>> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
>>>>                                   bool enable);
>>>>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>>>>                               bool enable, bool sync, bool allow_userspace);
>>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>>> +                                bool disable);
>>>>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
>>>>                                  bool enable, bool sync);
>>>>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
>>>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>>>> index 0233b87..e96c56d 100644
>>>> --- a/tools/libxc/xc_monitor.c
>>>> +++ b/tools/libxc/xc_monitor.c
>>>> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
>>>>      return do_domctl(xch, &domctl);
>>>>  }
>>>>
>>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>>> +                                bool disable)
>>>> +{
>>>> +    DECLARE_DOMCTL;
>>>> +
>>>> +    domctl.cmd = XEN_DOMCTL_monitor_op;
>>>> +    domctl.domain = domain_id;
>>>> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>>>> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
>>>> +    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
>>>> +
>>>> +    return do_domctl(xch, &domctl);
>>>> +}
>>>> +
>>>>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
>>>>                                  bool enable)
>>>>  {
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index c0cd017..07a334b 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>>>>      return violation;
>>>>  }
>>>>
>>>> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
>>>> +{
>>>> +    struct hvm_hw_cpu ctxt;
>>>> +    uint32_t pfec = 0;
>>>> +
>>>> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
>>>> +
>>>> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
>>>> +         && ga == v->arch.pg_dirty.gla )
>>>> +        pfec = PFEC_write_access;
>>>> +
>>>> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
>>>> +
>>>> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
>>>> +    v->arch.pg_dirty.gla = ga;
>>>> +}
>>>> +
>>>>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>>                            struct npfec npfec,
>>>>                            vm_event_request_t **req_ptr)
>>>> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>>          }
>>>>      }
>>>>
>>>> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
>>>> +         d->arch.monitor.nested_pagefault_disabled &&
>>>> +         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>>>> +    {
>>>> +        v->arch.vm_event->emulate_flags = 0;
>>>> +        p2m_set_ad_bits(v, gla);
>>>> +
>>>> +        return true;
>>>> +    }
>>>> +
>>>>      *req_ptr = NULL;
>>>>      req = xzalloc(vm_event_request_t);
>>>>      if ( req )
>>>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>>>> index f229e69..e35b619 100644
>>>> --- a/xen/arch/x86/monitor.c
>>>> +++ b/xen/arch/x86/monitor.c
>>>> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>>>>          break;
>>>>      }
>>>>
>>>> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
>>>> +    {
>>>> +        bool old_status = ad->monitor.nested_pagefault_disabled;
>>>> +
>>>> +        if ( unlikely(old_status == requested_status) )
>>>> +            return -EEXIST;
>>>> +
>>>> +        domain_pause(d);
>>>> +        ad->monitor.nested_pagefault_disabled = requested_status;
>>>> +        domain_unpause(d);
>>>> +        break;
>>>> +    }
>>>> +
>>>>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>>>>      {
>>>>          bool old_status = ad->monitor.descriptor_access_enabled;
>>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>>> index 4679d54..099af7c 100644
>>>> --- a/xen/include/asm-x86/domain.h
>>>> +++ b/xen/include/asm-x86/domain.h
>>>> @@ -412,6 +412,7 @@ struct arch_domain
>>>>          unsigned int descriptor_access_enabled                             : 1;
>>>>          unsigned int guest_request_userspace_enabled                       : 1;
>>>>          unsigned int emul_unimplemented_enabled                            : 1;
>>>> +        unsigned int nested_pagefault_disabled                             : 1;
>>>
>>> All other options are "_enabled" here, so adding one that's flipped
>>> just looks out of place. Any objections to making this match the rest?
>>> Also, naming it "nested" just makes me think this is somehow would be
>>> related to nested virtualization, but that's not the case. These would
>>> be just regular pagefaults in the guest, so naming the monitor option
>>> simply "pagefault" would look better to me in general.
>> Hello Tamas,
>>
>> Here's the thinking behind preferring "disabled" to "enabled": we want
>> to keep the default behaviour as it is currently, and the current
>> behaviour is to send out _all_ EPT fault vm_events (caused by page walks
>> or not).
>>
>> Now, struct arch_domain is being zeroed out on init, so if we name this
>> "enabled", then that's the behaviour we're starting out with. We have no
>> problem with that, but it changes the current default behaviour.
> 
> We can keep the "disabled" naming but then please add a comment to the field
> saying that by default all events are sent, this is used to filter
> pagefaults out.

Will do, no problem.

>> So either we name this new field "disabled", or we rename it to
>> "enabled" (if we rename it, we either need to set it as a special case
>> on init, or modify the default behaviour to be _not_ sending out
>> page-walk-caused EPT events).
>>
>> If you feel strongly about options 2.A or 2.C we don't have a problem
>> changing the code.
>>
>> About "pagefault", it reads more confusing to me, since all EPT-related
>> vm_events are basically page faults. But maybe that's just me.
> 
> True. It's just confusing with "nested" also having multiple meanings.
> Perhaps "inguest_pagefaults"?

Sure, if nobody else objects, we'll change "nested" to "inguest".


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel