[PATCH 0 of 3] xenpaging: Check that EPT is enabled for target guest

[PATCH 0 of 3] xenpaging: Check that EPT is enabled for target guest

[Patrick Colp]

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

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

[Patrick Colp]

# HG changeset patch
# User Patrick Colp <pjcolp@cs.ubc.ca>
# 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 <pjcolp@cs.ubc.ca>

diff -r ac7e4c6ec6c7 -r 6cb61775657f xen/arch/x86/mm/mem_event.c
--- a/xen/arch/x86/mm/mem_event.c	Fri Jul 23 19:23:49 2010 +0100
+++ b/xen/arch/x86/mm/mem_event.c	Tue Jul 27 14:11:49 2010 -0700
@@ -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);

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

[Ian Jackson]

Patrick Colp writes ("[Xen-devel] [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT"):
> 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.

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

[Ian Jackson]

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.

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

[Patrick Colp]

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


On 28 July 2010 09:59, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> 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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>

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

[Ian Jackson]

Patrick Colp writes ("Re: [Xen-devel] [PATCH 1 of 3] xenpaging: Add a check to Xen for EPT"):
> 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.

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

[Patrick Colp]

# HG changeset patch
# User Patrick Colp <pjcolp@cs.ubc.ca>
# 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 <pjcolp@cs.ubc.ca>

diff -r 6cb61775657f -r 5a5bfb95a437 tools/xenpaging/xenpaging.c
--- a/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
+++ b/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
@@ -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;
 }

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

[Ian Jackson]

Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):
>   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

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

>++        free(paging->bitmap);

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

Ian.

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

[Patrick Colp]

On 28 July 2010 10:00, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):
>>   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
>
>>-+        if ( paging->bitmap )
>>-+            free(paging->bitmap);
> with
>
>>++        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

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

[Gianni Tedesco]

On Wed, 2010-07-28 at 15:57 +0100, Patrick Colp wrote:
> On 28 July 2010 10:00, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):
> >>   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
> >
> >>-+        if ( paging->bitmap )
> >>-+            free(paging->bitmap);
> > with
> >
> >>++        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.

It's just simpler and takes less screen space.

> 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

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

[Patrick Colp]

On 28 July 2010 11:01, Gianni Tedesco <gianni.tedesco@citrix.com> wrote:
> On Wed, 2010-07-28 at 15:57 +0100, Patrick Colp wrote:
>> On 28 July 2010 10:00, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>> > Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):
>> >>   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
>> >
>> >>-+        if ( paging->bitmap )
>> >>-+            free(paging->bitmap);
>> > with
>> >
>> >>++        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.
>
> It's just simpler and takes less screen space.
>
>> 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

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

[Patrick Colp]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On 28 July 2010 11:28, Patrick Colp <pjcolp@cs.ubc.ca> wrote:
> On 28 July 2010 11:01, Gianni Tedesco <gianni.tedesco@citrix.com> wrote:
>> On Wed, 2010-07-28 at 15:57 +0100, Patrick Colp wrote:
>>> On 28 July 2010 10:00, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>>> > Patrick Colp writes ("[Xen-devel] [PATCH 2 of 3] xenpaging: Fix-up xenpaging tool code"):
>>> >>   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
>>> >
>>> >>-+        if ( paging->bitmap )
>>> >>-+            free(paging->bitmap);
>>> > with
>>> >
>>> >>++        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.
>>
>> It's just simpler and takes less screen space.
>>
>>> 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
>

How does this look?


Patrick

[-- Attachment #2: tools_xenpaging_cleanup.patch --]
[-- Type: text/x-patch, Size: 1767 bytes --]

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 <pjcolp@cs.ubc.ca>

diff -r fb0b9af3ed2c tools/xenpaging/xenpaging.c
--- a/tools/xenpaging/xenpaging.c	Tue Jul 27 17:04:24 2010 -0400
+++ b/tools/xenpaging/xenpaging.c	Tue Jul 27 17:04:30 2010 -0400
@@ -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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

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

[Keir Fraser]

On 28/07/2010 15:57, "Patrick Colp" <pjcolp@cs.ubc.ca> wrote:

>>> ++        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

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

[Patrick Colp]

# HG changeset patch
# User Patrick Colp <pjcolp@cs.ubc.ca>
# 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 <pjcolp@cs.ubc.ca>

diff -r 5a5bfb95a437 -r 4c37dd381199 tools/xenpaging/xenpaging.c
--- a/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
+++ b/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
@@ -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;
     }

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

[Jiang, Yunhong]

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


>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Patrick Colp
>Sent: Wednesday, July 28, 2010 5:21 AM
>To: xen-devel@lists.xensource.com
>Subject: [Xen-devel] [PATCH 3 of 3] xenpaging: Add check to xenpaging tool for EPT
>error from Xen
>
># HG changeset patch
># User Patrick Colp <pjcolp@cs.ubc.ca>
># 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 <pjcolp@cs.ubc.ca>
>
>diff -r 5a5bfb95a437 -r 4c37dd381199 tools/xenpaging/xenpaging.c
>--- a/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
>+++ b/tools/xenpaging/xenpaging.c	Tue Jul 27 14:11:49 2010 -0700
>@@ -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
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel