Xen devel - Feb 2013 - Re: [Xen-staging] [xen] x86/mm: Take the p2m lock even in shadow mode.

Ian, 

I suppose this came from the commit bot on the xen.git tree? Could we
get the branch added to the Subject (e.g. [xen staging] / [xen
staging/4.2] etc). If it could include the committer as well as the
author in the metadata that might be useful too.

Cheers,
Ian.

On Thu, 2013-02-21 at 19:12 +0000, patchbot@xen.org
wrote:
> commit a15d87475ed95840dba693ab0a56d0b48a215cbc
> Author: Tim Deegan <tim@xen.org>
> Date:   Thu Feb 21 14:07:19 2013 +0000
> 
>     x86/mm: Take the p2m lock even in shadow mode.
>     
>     The reworking of p2m lookups to use get_gfn()/put_gfn() left the
>     shadow code not taking the p2m lock, even in cases where the p2m would
>     be updated (i.e. PoD).
>     
>     In many cases, shadow code doesn''t need the exclusion that
>     get_gfn()/put_gfn() provides, as it has its own interlocks against p2m
>     updates, but this is taking things too far, and can lead to crashes in
>     the PoD code.
>     
>     Now that most shadow-code p2m lookups are done with explicitly
>     unlocked accessors, or with the get_page_from_gfn() accessor, which is
>     often lock-free, we can just turn this locking on.
>     
>     The remaining locked lookups are in sh_page_fault() (in a path
that''s
>     almost always already serializing on the paging lock), and in
>     emulate_map_dest() (which can probably be updated to use
>     get_page_from_gfn()).  They''re not addressed here but may be
in a
>     follow-up patch.
>     
>     Signed-off-by: Tim Deegan <tim@xen.org>
>     Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
>  xen/arch/x86/mm/p2m.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index de1dd82..2db73c9 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -218,8 +218,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m,
unsigned long gfn,
>          return _mfn(gfn);
>      }
>  
> -    /* For now only perform locking on hap domains */
> -    if ( locked && (hap_enabled(p2m->domain)) )
> +    if ( locked )
>          /* Grab the lock here, don''t release until put_gfn */
>          gfn_lock(p2m, gfn, 0);
>  
> @@ -248,8 +247,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m,
unsigned long gfn,
>  
>  void __put_gfn(struct p2m_domain *p2m, unsigned long gfn)
>  {
> -    if ( !p2m || !paging_mode_translate(p2m->domain) 
> -              || !hap_enabled(p2m->domain) )
> +    if ( !p2m || !paging_mode_translate(p2m->domain) )
>          /* Nothing to do in this case */
>          return;
>  
> --
> generated by git-patchbot for /home/xen/git/xen.git#staging
> 
> _______________________________________________
> Xen-staging mailing list
> Xen-staging@lists.xen.org
> http://lists.xensource.com/xen-staging

Ian Campbell writes ("Re: [Xen-staging] [xen] x86/mm: Take the p2m lock
even in shadow mode."):
> I suppose this came from the commit bot on the xen.git tree? Could we
> get the branch added to the Subject (e.g. [xen staging] / [xen
> staging/4.2] etc). If it could include the committer as well as the
> author in the metadata that might be useful too.
Done.

Ian.

On Fri, 2013-02-22 at 12:05 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-staging] [xen] x86/mm: Take the p2m
lock even in shadow mode."):
> > I suppose this came from the commit bot on the xen.git tree? Could we
> > get the branch added to the Subject (e.g. [xen staging] / [xen
> > staging/4.2] etc). If it could include the committer as well as the
> > author in the metadata that might be useful too.
> 
> Done.
Thanks!