Skip to contentSkip to conversation

xenpaging: Check that EPT is enabled for target guest

PATCH New Patrick Colp 5 participants 15 messages Jul 27, 2010 – Sep 15, 2010

lore
Share & tools
lore

Patrick Colp

author
#0 lore raw

PATCH 1/1 xenpaging: Check that EPT is enabled for target guest

This series of patches addes chs checks to Xen and the xenpaging userspace tool for EPT. xenpaging is only supported for guests running on EPT and currently if an attempt is made to use it on an unsupported guest, it fails with a very uninformative error.

Patrick

author
#1 lore raw

PATCH 1/1 xenpaging: Add a check to Xen for EPT

# HG changeset patch
# User Patrick Colp <[email protected]>
# Date 1280265109 25200
# Node ID 6cb61775657f6ea362b3ff45ed22e67b00ad3ea5 # Parent ac7e4c6ec6c7494e4046da92aa8f62f6c1371438
xenpaging: Add a check to Xen for EPT.

There isn't seem to be a way to directly check for EPT, so instead check for HAP and an Intel processor. If EPT isn't enabled, then return an error to the tool.

Signed-off-by Patrick Colp <[email protected]>
xen/arch/x86/mm/mem_event.c +7 -0
diff -r ac7e4c6ec6c7 -r 6cb61775657f xen/arch/x86/mm/mem_event.c
@@ -21,6 +21,7 @@
  */
 
 
+#include <asm/domain.h>
 #include <xen/event.h>
 #include <asm/p2m.h>
 #include <asm/mem_event.h>

@@ -225,6 +226,12 @@
             mfn_t ring_mfn;
             mfn_t shared_mfn;
 
+            /* Currently only EPT is supported */
+            rc = -ENODEV;
+            if ( !(hap_enabled(d) &&
+                  (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) )
+                break;
+
             /* Get MFN of ring page */
             guest_get_eff_l1e(v, ring_addr, &l1e);
             gfn = l1e_get_pfn(l1e);
author
#6 lore raw

PATCH 1/1 xenpaging: Fix-up xenpaging tool code

In reply to Patrick Colp

# HG changeset patch
# User Patrick Colp <[email protected]>
# Date 1280265109 25200
# Node ID 5a5bfb95a437cd860ab2da71c6534a2bef8fa558 # Parent 6cb61775657f6ea362b3ff45ed22e67b00ad3ea5
xenpaging: Fix-up xenpaging tool code.

This isn't directly related to EPT checking, but does some general fix-ups to the xenpaging code (adds some extra frees, etc.)

Signed-off-by Patrick Colp <[email protected]>
tools/xenpaging/xenpaging.c +30 -7
diff -r 6cb61775657f -r 5a5bfb95a437 tools/xenpaging/xenpaging.c
@@ -73,8 +73,9 @@
     xc_interface *xch;
     int rc;
 
-    xch = xc_interface_open(0,0,0);
-    if ( !xch ) return NULL;
+    xch = xc_interface_open(NULL, NULL, 0);
+    if ( !xch )
+        goto err_iface;
 
     DPRINTF("xenpaging init\n");
     *xch_r = xch;

@@ -101,7 +102,7 @@
     paging->mem_event.ring_page = init_page();
     if ( paging->mem_event.ring_page == NULL )
     {
-        ERROR("Error initialising shared page");
+        ERROR("Error initialising ring page");
         goto err;
     }
 

@@ -199,13 +200,33 @@
     return paging;
 
  err:
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);
+
+        if ( paging->platform_info )
+            free(paging->platform_info);
+
+        if ( paging->domain_info )
+            free(paging->domain_info);
+
+        if ( paging->mem_event.shared_page )
+        {
+            munlock(paging->mem_event.shared_page, PAGE_SIZE);
+            free(paging->mem_event.shared_page);
+        }
+
+        if ( paging->mem_event.ring_page )
+        {
+            munlock(paging->mem_event.ring_page, PAGE_SIZE);
+            free(paging->mem_event.ring_page);
+        }
+
         free(paging);
+    }
 
+ err_iface: 
     return NULL;
 }
 

@@ -619,6 +640,8 @@
     if ( rc == 0 )
         rc = rc1;
 
+    xc_interface_close(xch);
+
     return rc;
 }
author
#13 lore raw

PATCH 1/1 xenpaging: Add check to xenpaging tool for EPT error from Xen

In reply to Patrick Colp

# HG changeset patch
# User Patrick Colp <[email protected]>
# Date 1280265109 25200
# Node ID 4c37dd3811993b1ce173b3f5573be85cba1a53d9
# Parent 5a5bfb95a437cd860ab2da71c6534a2bef8fa558
xenpaging: Add check to xenpaging tool for EPT error from Xen.

Add a check in the xenpaging tool for the specific return code from Xen indicating that the target guest isn't using EPT. Return an appropriate error message so the user knows why xenpaging has failed.

Signed-off-by Patrick Colp <[email protected]>
tools/xenpaging/xenpaging.c +4 -1
diff -r 5a5bfb95a437 -r 4c37dd381199 tools/xenpaging/xenpaging.c
@@ -121,7 +121,10 @@
                              paging->mem_event.ring_page);
     if ( rc != 0 )
     {
-        ERROR("Error initialising shared page");
+        if ( errno == ENODEV )
+            ERROR("EPT not supported for this guest");
+        else
+            ERROR("Error initialising mem-event connection");
         goto err;
     }
#2 lore raw

Re: [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT

In reply to Patrick Colp

Patrick Colp writes ("[Xen-devel] [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT"):

Quoted 5 lines

xenpaging: Add a check to Xen for EPT.

There isn't seem to be a way to directly check for EPT, so instead check for HAP and an Intel processor. If EPT isn't enabled, then return an error to the tool.

I haven't tested this but this and the corresponding patch 2/3 to produce a better error message look sane. I do have one question: why ENODEV and not ENOSYS ? (I'm not particularly familiar with the hypervisor's errno conventions.)

Ian.

#3 lore raw

Re: [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT

Quoted 2 lines · I wrote:

I haven't tested this but this and the corresponding patch 2/3 to produce a better error message look sane. [...]

I meant 3/3. I have some comments on 2/3.

Ian.

#7 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

In reply to Patrick Colp

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):

tools/xenpaging/xenpaging.c line 206

Quoted 9 lines

err:

tools/xenpaging/xenpaging.c +3 -4
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);

While you're doing this, why not replace

Quoted 2 lines

-+ if ( paging->bitmap )
-+ free(paging->bitmap);

with

Quoted 1 line

++ free(paging->bitmap);

since free(0) is legal and a no-op ?

Ian.

author
#4 lore raw

Re: [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT

In reply to Ian Jackson

ENOSYS tends to be used to indicate an invalid/unsupported domctl. I chose ENODEV as an alternative so that it provides a unique error return code and seemed relatively sane (vs say something like E2BIG). I'm not particularly fussed about what the actually error code is, I just wanted it to be able to distinguish a lack-of-EPT-support error from other errors (the other error codes in use in the mem-event enable domctl are ENOSYS and EINVAL). If you have a suggestion for another one to use, I'm happy to hear it.

Patrick

Quoted 14 lines · Ian Jackson
Quoted 2 lines · I wrote:

I haven't tested this but this and the corresponding patch 2/3 to produce a better error message look sane.  [...]

I meant 3/3.  I have some comments on 2/3.

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#8 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

In reply to Ian Jackson
Quoted 13 lines · Ian Jackson

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):

Quoted 9 lines

  err:
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);

While you're doing this, why not replace

Quoted 7 lines
Quoted 2 lines

-+        if ( paging->bitmap )
-+            free(paging->bitmap);

with

Quoted 1 line

++        free(paging->bitmap);

since free(0) is legal and a no-op ?

Could do, but free(0) isn't exactly a no-op. free() does a check to see if the pointer passed was 0. So it doesn't really make much difference if I do the check or let it do the check. I can easily change the code to just do free(paging->bitmap) though, if that's the preferred way to do it. Actually, I would argue my way is better since in the case of a NULL pointer, the free function isn't called at all, which saves a bunch of cycles.

Patrick

#9 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

Quoted 20 lines · Patrick Colp
Quoted 19 lines · Ian Jackson

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):

tools/xenpaging/xenpaging.c line 206

Quoted 9 lines

err:

tools/xenpaging/xenpaging.c +3 -4
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);

While you're doing this, why not replace

Quoted 2 lines

-+ if ( paging->bitmap )
-+ free(paging->bitmap);

with

Quoted 1 line

++ free(paging->bitmap);

Quoted 7 lines
Quoted 1 line

since free(0) is legal and a no-op ?

Could do, but free(0) isn't exactly a no-op. free() does a check to see if the pointer passed was 0. So it doesn't really make much difference if I do the check or let it do the check. I can easily change the code to just do free(paging->bitmap) though, if that's the preferred way to do it.

It's just simpler and takes less screen space.

Quoted 3 lines

Actually, I would argue my way is better since
in the case of a NULL pointer, the free function isn't called at all, which saves a bunch of cycles.

At the expense of expanding the binary image with a few more instructions. Besides don't "optimize" what isn't a bottleneck.

Gianni

#12 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

In reply to Patrick Colp
Quoted 11 lines · Patrick Colp
Quoted 3 lines
Quoted 1 line

++        free(paging->bitmap);

since free(0) is legal and a no-op ?

Could do, but free(0) isn't exactly a no-op. free() does a check to see if the pointer passed was 0. So it doesn't really make much difference if I do the check or let it do the check. I can easily change the code to just do free(paging->bitmap) though, if that's the preferred way to do it. Actually, I would argue my way is better since in the case of a NULL pointer, the free function isn't called at all, which saves a bunch of cycles.

Avoiding the if is better. Everyone knows free(0) is legal, so it's idiomatic to unconditionally call free().

-- Keir

#5 lore raw

Re: [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT

In reply to Patrick Colp

Patrick Colp writes ("Re: [Xen-devel] [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT"):

Quoted 8 lines

ENOSYS tends to be used to indicate an invalid/unsupported domctl. I chose ENODEV as an alternative so that it provides a unique error return code and seemed relatively sane (vs say something like E2BIG). I'm not particularly fussed about what the actually error code is, I just wanted it to be able to distinguish a lack-of-EPT-support error from other errors (the other error codes in use in the mem-event enable domctl are ENOSYS and EINVAL). If you have a suggestion for another one to use, I'm happy to hear it.

No, thanks, your rationale makes sense to me. ENODEV is fine.

Ian.

author
#10 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

In reply to Gianni Tedesco
Quoted 29 lines · Gianni Tedesco
Quoted 20 lines · Patrick Colp
Quoted 19 lines · Ian Jackson

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):

Quoted 9 lines

  err:
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);

While you're doing this, why not replace

Quoted 2 lines

-+        if ( paging->bitmap )
-+            free(paging->bitmap);

with

Quoted 1 line

++        free(paging->bitmap);

Quoted 7 lines
Quoted 1 line

since free(0) is legal and a no-op ?

Could do, but free(0) isn't exactly a no-op. free() does a check to see if the pointer passed was 0. So it doesn't really make much difference if I do the check or let it do the check. I can easily change the code to just do free(paging->bitmap) though, if that's the preferred way to do it.

Quoted 8 lines

It's just simpler and takes less screen space.

Quoted 3 lines

Actually, I would argue my way is better since
in the case of a NULL pointer, the free function isn't called at all, which saves a bunch of cycles.

At the expense of expanding the binary image with a few more instructions. Besides don't "optimize" what isn't a bottleneck.

All good points. I'll fix up the patches and resubmit them.

Patrick

author
#11 lore raw

Re: [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code

Quoted 36 lines · Patrick Colp
Quoted 35 lines · Gianni Tedesco
Quoted 20 lines · Patrick Colp
Quoted 19 lines · Ian Jackson

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):

Quoted 9 lines

  err:
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->bitmap )
+            free(paging->bitmap);

While you're doing this, why not replace

Quoted 2 lines

-+        if ( paging->bitmap )
-+            free(paging->bitmap);

with

Quoted 1 line

++        free(paging->bitmap);

Quoted 7 lines
Quoted 1 line

since free(0) is legal and a no-op ?

Could do, but free(0) isn't exactly a no-op. free() does a check to see if the pointer passed was 0. So it doesn't really make much difference if I do the check or let it do the check. I can easily change the code to just do free(paging->bitmap) though, if that's the preferred way to do it.

It's just simpler and takes less screen space.

Quoted 3 lines

Actually, I would argue my way is better since
in the case of a NULL pointer, the free function isn't called at all, which saves a bunch of cycles.

Quoted 8 lines
Quoted 2 lines

At the expense of expanding the binary image with a few more instructions. Besides don't "optimize" what isn't a bottleneck.

All good points. I'll fix up the patches and resubmit them.

Patrick

How does this look?

Patrick

xenpaging: Fix-up xenpaging tool code.

This isn't directly related to EPT checking, but does some general fix-ups to the xenpaging code (adds some extra frees, etc.)

Signed-off-by Patrick Colp <[email protected]>
tools/xenpaging/xenpaging.c +24 -7
diff -r fb0b9af3ed2c tools/xenpaging/xenpaging.c
@@ -73,8 +73,9 @@
     xc_interface *xch;
     int rc;
 
-    xch = xc_interface_open(0,0,0);
-    if ( !xch ) return NULL;
+    xch = xc_interface_open(NULL, NULL, 0);
+    if ( !xch )
+        goto err_iface;
 
     DPRINTF("xenpaging init\n");
     *xch_r = xch;

@@ -101,7 +102,7 @@
     paging->mem_event.ring_page = init_page();
     if ( paging->mem_event.ring_page == NULL )
     {
-        ERROR("Error initialising shared page");
+        ERROR("Error initialising ring page");
         goto err;
     }
 

@@ -199,13 +200,27 @@
     return paging;
 
  err:
-    if ( paging->bitmap )
-        free(paging->bitmap);
-    if ( paging->platform_info )
-        free(paging->platform_info);
     if ( paging )
+    {
+        if ( paging->mem_event.shared_page )
+        {
+            munlock(paging->mem_event.shared_page, PAGE_SIZE);
+            free(paging->mem_event.shared_page);
+        }
+
+        if ( paging->mem_event.ring_page )
+        {
+            munlock(paging->mem_event.ring_page, PAGE_SIZE);
+            free(paging->mem_event.ring_page);
+        }
+
+        free(paging->bitmap);
+        free(paging->platform_info);
+        free(paging->domain_info);
         free(paging);
+    }
 
+ err_iface: 
     return NULL;
 }
 

@@ -619,6 +640,8 @@
     if ( rc == 0 )
         rc = rc1;
 
+    xc_interface_close(xch);
+
     return rc;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#14 lore raw

RE: [PATCH 3 of 3] xenpaging: Add check to xenpaging tool for EPT error from Xen

In reply to Patrick Colp

Patrick, Seems this patch is not in the upstream tree still?

Although this change is not that meaningful to user (user may have no idea of EPT at all), but at least it avoid the same error information ("Error initialising shared page") in two code, with totally different reason:

    /* Initialise shared page */
    paging->mem_event.shared_page = init_page();
    if ( paging->mem_event.shared_page == NULL )
    {
        ERROR("Error initialising shared page");
        goto err;
    }

and

   /* Initialise Xen */
    rc = xc_mem_event_enable(paging->xc_handle, paging->mem_event.domain_id,
                             paging->mem_event.shared_page,
                             paging->mem_event.ring_page);
    if ( rc != 0 )
    {
        ERROR("Error initialising shared page");
        goto err;
    }

Thanks
--jyh

Quoted 41 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Patrick Colp
Sent: Wednesday, July 28, 2010 5:21 AM
To: [email protected]
Subject: [Xen-devel] [PATCH 3 of 3] xenpaging: Add check to xenpaging tool for EPT error from Xen

# HG changeset patch
# User Patrick Colp <[email protected]>
# Date 1280265109 25200
# Node ID 4c37dd3811993b1ce173b3f5573be85cba1a53d9
# Parent 5a5bfb95a437cd860ab2da71c6534a2bef8fa558
xenpaging: Add check to xenpaging tool for EPT error from Xen.

Add a check in the xenpaging tool for the specific return code from Xen indicating that the target guest isn't using EPT. Return an appropriate error message so the user knows why xenpaging has failed.

Signed-off-by Patrick Colp <[email protected]>

diff -r 5a5bfb95a437 -r 4c37dd381199 tools/xenpaging/xenpaging.c

tools/xenpaging/xenpaging.c +4 -1
@@ -121,7 +121,10 @@
                             paging->mem_event.ring_page);
    if ( rc != 0 )
    {
-        ERROR("Error initialising shared page");
+        if ( errno == ENODEV )
+            ERROR("EPT not supported for this guest");
+        else
+            ERROR("Error initialising mem-event connection");
        goto err;
    }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel