Xen devel - Jun 2010 - [PATCH] remove useless super page recomputing in ept_set_entry

VMX: remove useless super page recomputing in ept_set_entry.

Caller to ept_set_entry must give a valid order that hardware supports, see
set_p2m_entry in p2m.c.

Signed-off-by: Xin Li <xin.li@intel.com>

diff -r a6d5b76abdcb xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c Tue Jun 29 18:20:14 2010 +0100
+++ b/xen/arch/x86/mm/hap/p2m-ept.c Wed Jun 30 09:59:40 2010 +0800
@@ -307,18 +307,10 @@
     }
     else
     {
-        int num = order / EPT_TABLE_ORDER;
         int level;
         ept_entry_t *split_ept_entry;

-        if ( (num >= 2) && hvm_hap_has_1gb(d) )
-            num = 2;
-        else if ( (num >= 1) && hvm_hap_has_2mb(d) )
-            num = 1;
-        else
-            num = 0;
-
-        for ( level = split_level; level > num ; level-- )
+        for ( level = split_level; level > walk_level ; level-- )
         {
             rv = ept_split_large_page(d, &table, &index, gfn, level);
             if ( !rv )

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

Would it make sense to turn this check on in DEBUG mode?  It can''t be
that expensive if it''s currently on all the time.

It''s OK to code with assumptions about what some other part of the
code is doing, but it''s always good to add checks (in DEBUG mode) to
catch problems if the assumptions change.

 -George

On Tue, Jun 29, 2010 at 7:06 PM, Li, Xin <xin.li@intel.com>
wrote:
> VMX: remove useless super page recomputing in ept_set_entry.
>
> Caller to ept_set_entry must give a valid order that hardware supports, see
set_p2m_entry in p2m.c.
>
> Signed-off-by: Xin Li <xin.li@intel.com>
>
> diff -r a6d5b76abdcb xen/arch/x86/mm/hap/p2m-ept.c
> --- a/xen/arch/x86/mm/hap/p2m-ept.c Tue Jun 29 18:20:14 2010 +0100
> +++ b/xen/arch/x86/mm/hap/p2m-ept.c Wed Jun 30 09:59:40 2010 +0800
> @@ -307,18 +307,10 @@
>     }
>     else
>     {
> -        int num = order / EPT_TABLE_ORDER;
>         int level;
>         ept_entry_t *split_ept_entry;
>
> -        if ( (num >= 2) && hvm_hap_has_1gb(d) )
> -            num = 2;
> -        else if ( (num >= 1) && hvm_hap_has_2mb(d) )
> -            num = 1;
> -        else
> -            num = 0;
> -
> -        for ( level = split_level; level > num ; level-- )
> +        for ( level = split_level; level > walk_level ; level-- )
>         {
>             rv = ept_split_large_page(d, &table, &index, gfn,
level);
>             if ( !rv )
>
> _______________________________________________
> 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

> Would it make sense to turn this check on in DEBUG mode?  It can''t
be
> that expensive if it''s currently on all the time.
Put the check in debug mode is of course no problem, the problem to me is the
code
implies:

Say the original page is a super page, we want to change a page inside it.  When
we
get here we need to split the original super page, but somehow smaller super
page is
not supported, we need split the page further.

Although hardware spec doesn''t say smaller super page will also be
supported, but the
code will be much simpler can cleaner.

Actually it is the p2m layer managing guest memory allocation/free and mapping,
so it
is not reasonable to me to put part of the logic like splitting a page into EPT
layer.  How
current code manages the info when a super page is split?  Will the split page
be changed
to some newly allocated page or freed casually?

Anyway I prefer to keep such info in p2m layer.

Thanks!
-Xin

> 
> It''s OK to code with assumptions about what some other part of the
> code is doing, but it''s always good to add checks (in DEBUG mode)
to
> catch problems if the assumptions change.
> 
>  -George
> 
> On Tue, Jun 29, 2010 at 7:06 PM, Li, Xin <xin.li@intel.com> wrote:
> > VMX: remove useless super page recomputing in ept_set_entry.
> >
> > Caller to ept_set_entry must give a valid order that hardware
supports, see
> set_p2m_entry in p2m.c.
> >
> > Signed-off-by: Xin Li <xin.li@intel.com>
> >
> > diff -r a6d5b76abdcb xen/arch/x86/mm/hap/p2m-ept.c
> > --- a/xen/arch/x86/mm/hap/p2m-ept.c Tue Jun 29 18:20:14 2010 +0100
> > +++ b/xen/arch/x86/mm/hap/p2m-ept.c Wed Jun 30 09:59:40 2010 +0800
> > @@ -307,18 +307,10 @@
> >     }
> >     else
> >     {
> > -        int num = order / EPT_TABLE_ORDER;
> >         int level;
> >         ept_entry_t *split_ept_entry;
> >
> > -        if ( (num >= 2) && hvm_hap_has_1gb(d) )
> > -            num = 2;
> > -        else if ( (num >= 1) && hvm_hap_has_2mb(d) )
> > -            num = 1;
> > -        else
> > -            num = 0;
> > -
> > -        for ( level = split_level; level > num ; level-- )
> > +        for ( level = split_level; level > walk_level ; level-- )
> >         {
> >             rv = ept_split_large_page(d, &table, &index, gfn,
level);
> >             if ( !rv )
> >
> > _______________________________________________
> > 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

> > Would it make sense to turn this check on in DEBUG mode?  It
can''t be
> > that expensive if it''s currently on all the time.
But I agree it''s ok to add

ASSERT((walk_level == 2) && hvm_hap_has_1gb(d))
ASSERT((walk_level == 1) && hvm_hap_has_2mb(d))

to ept_set_entry.
Thanks!
-Xin

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