Xen devel - Feb 2008 - FW: [PATCH][UPDATE]Remove lock on guest table walk

Is there any trouble with mailing list? I didn''t see my mail sent back
after almost 8hrs. :-(

Thanks,
Kevin 

-----Original Message-----
From: Tian, Kevin 
Sent: 2008年2月22日 10:33
To: ''Tim Deegan''
Cc: xen-devel@lists.xensource.com
Subject: [PATCH][UPDATE]Remove lock on guest table walk
>From: Tim Deegan
>Sent: 2008年2月21日 22:13
>Hi,
>
>So, the idea seems sound, and avoids the shadow lock altogether on a
>bunch more pagefaults, which is nice. 
>
>I think that since PV pagetables are guaranteed to be read-only above
>l1e, the guest_map_l1e and guest_get_eff_l1e functions can be 
>allowed to
>drop the shadow lock and call guest_walk_tables with shadow_op == 0.
>That would mean that there are no callers left setting shadow_op to 1,
>and then the shadow_op argument (and the whole mechanism for calling
>remove_write_access from guest_walk_tables) could be removed.
Thanks for clarification.
>
>I think that in guest_walk_tables, you need to add an rmb() after
>reading the version number to stop the compiler from hoisting the
>pagetable reads to before the version read.
Yes, my overlook and thanks for pointing out.
>
>Coding nits:
> - I''d like to see the "version" field called something
more
>   descriptive, and moved into the shadow-specific domain state, 
>   since HAP won''t be using it.
Done and renamed to gtable_dirty_version. Sorry if this is still not a
good name and please feel free help polish a better name. :-)
> - In shadow_check_gwalk, maybe return 1 at the top of the function if
>   the version number hasn''t changed, rather than putting most of
the
>   function inside an if()?
Agree.
> - You''ve added a second "not a shadow_fault" printout
without removing
>   the first.
The first one is branched with lock held, and some audit functions also
required lock held. To differentiate, I put a new label. Now I just removed
the new label since only few places use it.
> - We can remove the comment at the top of multi.c about reworking the
>   guest_walk TLB flush logic. :)
Done.

Then updated version attached, and please help check again.

Thanks,
Kevin


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

At Fri, 22 Feb 2008 17:29:29 +0800,
Tian, Kevin wrote:
> 
> Then updated version attached, and please help check again.
> 
Ehr, sorry. Another disturbing mail from me with a very minor fix: can
you remove the ASSERT(!shadow_op ...) line? It''s not needed anymore
and it breaks the debug build.

Thanks!
Gianluca

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

>From: Gianluca Guida [mailto:gianluca.guida@eu.citrix.com] 
>Sent: 2008年2月25日 0:58
>
>At Fri, 22 Feb 2008 17:29:29 +0800,
>Tian, Kevin wrote:
>> 
>> Then updated version attached, and please help check again.
>> 
>
>Ehr, sorry. Another disturbing mail from me with a very minor fix: can
>you remove the ASSERT(!shadow_op ...) line? It''s not needed anymore
>and it breaks the debug build.
>
>Thanks!
>Gianluca
>
Here it is. Thanks again for pointing it out. 

Thanks,
Kevin


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

At Mon, 25 Feb 2008 09:37:42 +0800,
Tian, Kevin wrote:
> Here it is. Thanks again for pointing it out. 
One last thing. At the moment we''re auditing the tables and the guest
walk when we''re not sure that the walk itself is consistent or
not. Inline patch fixes this.

Thanks,
Gianl.


diff -r 993a7e1d224d xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Thu Feb 28 13:21:54 2008 +0000
+++ b/xen/arch/x86/mm/shadow/multi.c	Thu Feb 28 13:24:11 2008 +0000
@@ -2952,8 +2952,6 @@ static int sh_page_fault(struct vcpu *v,
 #endif /* (SHADOW_OPTIMIZATIONS & SHOPT_VIRTUAL_TLB) */
 
     shadow_lock(d);
-    shadow_audit_tables(v);
-    sh_audit_gw(v, &gw);
 
     if ( gw_remove_write_accesses(v, va, &gw) )
     {
@@ -2971,6 +2969,9 @@ static int sh_page_fault(struct vcpu *v,
         shadow_unlock(d);
         return EXCRET_fault_fixed;
     }
+
+    shadow_audit_tables(v);
+    sh_audit_gw(v, &gw);
 
     /* Make sure there is enough free shadow memory to build a chain of
      * shadow tables. (We never allocate a top-level shadow on this path,

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