Re: [Xen-devel] [PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn

[Tim Deegan <tim@xxxxxxx>]

At 15:34 -0400 on 24 Apr (1335281651), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/mm/hap/guest_walk.c |  6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> 
> We don't need to hold the p2m lock for the duration of the guest walk. We need
> to ensure livenes of the top level page.

I'm not sure I want to start taking these s/gfn-lock/get-page/ changes
piecemeal without a clear understanding of why the gfn-locking is not
needed.  My understanding was that

 - get_page protects against the page being freed and reused.
 - gfn-lock protects against the GFN being reused, or the page 
   being reused within the VM. 

Are there some cases where we only care about the first of these and
some where we care about both?  In other words, can we replace the
gfn-locking everywhere with get_page/put_page (i.e. get rid of put_gfn)?

Also:

> @@ -85,13 +86,16 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>  
>      /* Map the top-level table and call the tree-walker */
>      ASSERT(mfn_valid(mfn_x(top_mfn)));
> +    top_page = mfn_to_page(mfn_x(top_mfn));
> +    ASSERT(get_page(top_page, p2m->domain));

ASSERT(foo) is compiled out on non-debug builds, so that's definitely
not what you want. 

I personally dislike this idiom with BUG_ON() as well, because even
though BUG_ON(some side-effecting statement) may be correct, my eye
tends to skip over it when skimming code.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel