[PATCH] xen: Disable emulate.c REP optimization if introspection is active

[PATCH] xen: Disable emulate.c REP optimization if introspection is active

[Razvan Cojocaru]

Emulation for REP instructions is optimized to perform a single
write for all repeats in the current page if possible. However,
this interferes with a memory introspection application's ability
to detect suspect behaviour, since it will cause only one
mem_event to be sent per page touched.
This patch disables the optimization, gated on introspection
being active for the domain.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index c0f47d2..e53f390 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -395,6 +395,7 @@ static int hvmemul_virtual_to_linear(
 {
     struct segment_register *reg;
     int okay;
+    struct domain *currd = current->domain;
 
     if ( seg == x86_seg_none )
     {
@@ -402,12 +403,15 @@ static int hvmemul_virtual_to_linear(
         return X86EMUL_OKAY;
     }
 
-    /*
-     * Clip repetitions to avoid overflow when multiplying by @bytes_per_rep.
-     * The chosen maximum is very conservative but it's what we use in
-     * hvmemul_linear_to_phys() so there is no point in using a larger value.
-     */
-    *reps = min_t(unsigned long, *reps, 4096);
+    if ( currd->arch.hvm_domain.introspection_enabled )
+        *reps = 1;
+    else
+        /*
+         * Clip repetitions to avoid overflow when multiplying by @bytes_per_rep.
+         * The chosen maximum is very conservative but it's what we use in
+         * hvmemul_linear_to_phys() so there is no point in using a larger value.
+         */
+        *reps = min_t(unsigned long, *reps, 4096);
 
     reg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
 
-- 
1.7.9.5

Re: [PATCH] xen: Disable emulate.c REP optimization if introspection is active

[Jan Beulich]

>>> On 03.11.14 at 10:39, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -395,6 +395,7 @@ static int hvmemul_virtual_to_linear(
>  {
>      struct segment_register *reg;
>      int okay;
> +    struct domain *currd = current->domain;

This is being used just once and hence not really worthwhile.

> @@ -402,12 +403,15 @@ static int hvmemul_virtual_to_linear(
>          return X86EMUL_OKAY;
>      }
>  
> -    /*
> -     * Clip repetitions to avoid overflow when multiplying by @bytes_per_rep.
> -     * The chosen maximum is very conservative but it's what we use in
> -     * hvmemul_linear_to_phys() so there is no point in using a larger value.
> -     */
> -    *reps = min_t(unsigned long, *reps, 4096);
> +    if ( currd->arch.hvm_domain.introspection_enabled )
> +        *reps = 1;
> +    else
> +        /*
> +         * Clip repetitions to avoid overflow when multiplying by @bytes_per_rep.
> +         * The chosen maximum is very conservative but it's what we use in
> +         * hvmemul_linear_to_phys() so there is no point in using a larger value.
> +         */
> +        *reps = min_t(unsigned long, *reps, 4096);

With introspection_enabled supposedly being the unusual case (at
least for the majority of current users), a likely()/unlikely() annotation
should be used here. Further, with it being unclear (at least from
patch description and context alone) whether *reps could legally be
zero when getting here, I'd prefer

    *reps = min_t(unsigned long, *reps,
                  unlikely(current->domain->arch.hvm_domain.introspection_enabled)
                           ? 1 : 4096);

Perhaps with an adjustment to the comment explaining why.

Jan