Xen devel - Jul 2010 - [PATCH 0 of 3] 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

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

# 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);

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

# 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;
 }
 

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

# 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

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.

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

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

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.

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

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
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

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

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


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

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



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

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.

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

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

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

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


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

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel