diff mbox

[v2,2/2] xen-access: write_ctrlreg_c4 test

Message ID 1496137567-6574-3-git-send-email-ppircalabu@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Petre Ovidiu PIRCALABU May 30, 2017, 9:46 a.m. UTC 
Add test for write_ctrlreg event handling.

Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
---
 tools/tests/xen-access/xen-access.c | 47 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

Comments

Tamas K Lengyel June 16, 2017, 2:32 p.m. UTC | #1 
On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu
<ppircalabu@bitdefender.com> wrote:
> Add test for write_ctrlreg event handling.
>
> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com>
> ---
>  tools/tests/xen-access/xen-access.c | 47 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
> index 238011e..29b60e8 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -57,6 +57,9 @@
>  #define X86_TRAP_DEBUG  1
>  #define X86_TRAP_INT3   3
>
> +/* From xen/include/asm-x86/x86-defns.h */
> +#define X86_CR4_PGE        0x00000080 /* enable global pages */
> +
>  typedef struct vm_event {
>      domid_t domain_id;
>      xenevtchn_handle *xce_handle;
> @@ -314,6 +317,22 @@ static void get_request(vm_event_t *vm_event, vm_event_request_t *req)
>  }
>
>  /*
> + * X86 control register names
> + */
> +static const char* get_x86_ctrl_reg_name(uint32_t index)
> +{
> +    static const char* names[] = {

I would prefer to see this being defined in the following form so that
it is clear where the indexes come from:
  [VM_EVENT_X86_CR0] = "CR0",
  ...

> +        "CR0",
> +        "CR3",
> +        "CR4",
> +        "XCR0",
> +    };

And this check to be index > VM_EVENT_X86_XCR0

> +    return (index > 3) ? "" : names[index];
> +}
> +
> +
> +/*
>   * Note that this function is not thread safe.
>   */
>  static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
> @@ -337,7 +356,7 @@ void usage(char* progname)
>  {
>      fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
>  #if defined(__i386__) || defined(__x86_64__)
> -            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
> +            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access|write_ctrlreg_cr4");
>  #elif defined(__arm__) || defined(__aarch64__)
>              fprintf(stderr, "|privcall");
>  #endif
> @@ -369,6 +388,7 @@ int main(int argc, char *argv[])
>      int debug = 0;
>      int cpuid = 0;
>      int desc_access = 0;
> +    int write_ctrlreg_cr4 = 1;
>      uint16_t altp2m_view_id = 0;
>
>      char* progname = argv[0];
> @@ -439,6 +459,10 @@ int main(int argc, char *argv[])
>      {
>          desc_access = 1;
>      }
> +    else if ( !strcmp(argv[0], "write_ctrlreg_cr4") )
> +    {
> +        write_ctrlreg_cr4 = 1;
> +    }
>  #elif defined(__arm__) || defined(__aarch64__)
>      else if ( !strcmp(argv[0], "privcall") )
>      {
> @@ -596,6 +620,18 @@ int main(int argc, char *argv[])
>          }
>      }
>
> +    if ( write_ctrlreg_cr4 )
> +    {
> +        /* Mask the CR4.PGE bit so no events will be generated for global TLB flushes. */
> +        rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR4, 1, 1,
> +                                      X86_CR4_PGE, 1);
> +        if ( rc < 0 )
> +        {
> +            ERROR("Error %d setting write control register trapping with vm_event\n", rc);
> +            goto exit;
> +        }
> +    }
> +
>      /* Wait for access */
>      for (;;)
>      {
> @@ -806,6 +842,15 @@ int main(int argc, char *argv[])
>                         req.u.desc_access.is_write);
>                  rsp.flags |= VM_EVENT_FLAG_EMULATE;
>                  break;
> +            case VM_EVENT_REASON_WRITE_CTRLREG:
> +                printf("Control register written: rip=%016"PRIx64", vcpu %d: "
> +                       "reg=%s, old_value=%016"PRIx64", new_value=%016"PRIx64"\n",
> +                       req.data.regs.x86.rip,
> +                       req.vcpu_id,
> +                       get_x86_ctrl_reg_name(req.u.write_ctrlreg.index),
> +                       req.u.write_ctrlreg.old_value,
> +                       req.u.write_ctrlreg.new_value);
> +                break;
>              default:
>                  fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
>              }
> --
> 2.7.4
Jan Beulich June 16, 2017, 3:12 p.m. UTC | #2 
>>> On 16.06.17 at 16:32, <tamas@tklengyel.com> wrote:
> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu <ppircalabu@bitdefender.com> wrote:
>> @@ -314,6 +317,22 @@ static void get_request(vm_event_t *vm_event, vm_event_request_t *req)
>>  }
>>
>>  /*
>> + * X86 control register names
>> + */
>> +static const char* get_x86_ctrl_reg_name(uint32_t index)
>> +{
>> +    static const char* names[] = {
> 
> I would prefer to see this being defined in the following form so that
> it is clear where the indexes come from:
>   [VM_EVENT_X86_CR0] = "CR0",
>   ...
> 
>> +        "CR0",
>> +        "CR3",
>> +        "CR4",
>> +        "XCR0",
>> +    };
> 
> And this check to be index > VM_EVENT_X86_XCR0

Or perhaps even better >= ARRAY_SIZE()?

>> +    return (index > 3) ? "" : names[index];
>> +}
>> +
>> +
>> +/*

I also notice there are two successive blank lines here, which we
generally try to avoid.

Jan
Tamas K Lengyel June 16, 2017, 3:24 p.m. UTC | #3 
On Fri, Jun 16, 2017 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.06.17 at 16:32, <tamas@tklengyel.com> wrote:
>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu <ppircalabu@bitdefender.com> wrote:
>>> @@ -314,6 +317,22 @@ static void get_request(vm_event_t *vm_event, vm_event_request_t *req)
>>>  }
>>>
>>>  /*
>>> + * X86 control register names
>>> + */
>>> +static const char* get_x86_ctrl_reg_name(uint32_t index)
>>> +{
>>> +    static const char* names[] = {
>>
>> I would prefer to see this being defined in the following form so that
>> it is clear where the indexes come from:
>>   [VM_EVENT_X86_CR0] = "CR0",
>>   ...
>>
>>> +        "CR0",
>>> +        "CR3",
>>> +        "CR4",
>>> +        "XCR0",
>>> +    };
>>
>> And this check to be index > VM_EVENT_X86_XCR0
>
> Or perhaps even better >= ARRAY_SIZE()?

Yeap, even better.

Tamas
diff mbox

Patch

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 238011e..29b60e8 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -57,6 +57,9 @@ 
 #define X86_TRAP_DEBUG  1
 #define X86_TRAP_INT3   3
 
+/* From xen/include/asm-x86/x86-defns.h */
+#define X86_CR4_PGE        0x00000080 /* enable global pages */
+
 typedef struct vm_event {
     domid_t domain_id;
     xenevtchn_handle *xce_handle;
@@ -314,6 +317,22 @@  static void get_request(vm_event_t *vm_event, vm_event_request_t *req)
 }
 
 /*
+ * X86 control register names
+ */
+static const char* get_x86_ctrl_reg_name(uint32_t index)
+{
+    static const char* names[] = {
+        "CR0",
+        "CR3",
+        "CR4",
+        "XCR0",
+    };
+
+    return (index > 3) ? "" : names[index];
+}
+
+
+/*
  * Note that this function is not thread safe.
  */
 static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
@@ -337,7 +356,7 @@  void usage(char* progname)
 {
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
-            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
+            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access|write_ctrlreg_cr4");
 #elif defined(__arm__) || defined(__aarch64__)
             fprintf(stderr, "|privcall");
 #endif
@@ -369,6 +388,7 @@  int main(int argc, char *argv[])
     int debug = 0;
     int cpuid = 0;
     int desc_access = 0;
+    int write_ctrlreg_cr4 = 1;
     uint16_t altp2m_view_id = 0;
 
     char* progname = argv[0];
@@ -439,6 +459,10 @@  int main(int argc, char *argv[])
     {
         desc_access = 1;
     }
+    else if ( !strcmp(argv[0], "write_ctrlreg_cr4") )
+    {
+        write_ctrlreg_cr4 = 1;
+    }
 #elif defined(__arm__) || defined(__aarch64__)
     else if ( !strcmp(argv[0], "privcall") )
     {
@@ -596,6 +620,18 @@  int main(int argc, char *argv[])
         }
     }
 
+    if ( write_ctrlreg_cr4 )
+    {
+        /* Mask the CR4.PGE bit so no events will be generated for global TLB flushes. */
+        rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR4, 1, 1,
+                                      X86_CR4_PGE, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d setting write control register trapping with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     /* Wait for access */
     for (;;)
     {
@@ -806,6 +842,15 @@  int main(int argc, char *argv[])
                        req.u.desc_access.is_write);
                 rsp.flags |= VM_EVENT_FLAG_EMULATE;
                 break;
+            case VM_EVENT_REASON_WRITE_CTRLREG:
+                printf("Control register written: rip=%016"PRIx64", vcpu %d: "
+                       "reg=%s, old_value=%016"PRIx64", new_value=%016"PRIx64"\n",
+                       req.data.regs.x86.rip,
+                       req.vcpu_id,
+                       get_x86_ctrl_reg_name(req.u.write_ctrlreg.index),
+                       req.u.write_ctrlreg.old_value,
+                       req.u.write_ctrlreg.new_value);
+                break;
             default:
                 fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
             }