Re: [PATCH v3 05/13] x86/altp2m: basic data structures and support routines.

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Wed, Jul 1, 2015 at 7:09 PM, Ed White <edmund.h.white@intel.com> wrote:
> diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> index b4f035e..301ca59 100644
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -217,7 +217,7 @@ declare_mm_lock(nestedp2m)
>  #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
>  #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
>
> -/* P2M lock (per-p2m-table)
> +/* P2M lock (per-non-alt-p2m-table)
>   *
>   * This protects all queries and updates to the p2m table.
>   * Queries may be made under the read lock but all modifications
> @@ -225,10 +225,44 @@ declare_mm_lock(nestedp2m)
>   *
>   * The write lock is recursive as it is common for a code path to look
>   * up a gfn and later mutate it.
> + *
> + * Note that this lock shares its implementation with the altp2m
> + * lock (not the altp2m list lock), so the implementation
> + * is found there.
>   */
>
>  declare_mm_rwlock(p2m);
> -#define p2m_lock(p)           mm_write_lock(p2m, &(p)->lock);
> +
> +/* Alternate P2M list lock (per-domain)
> + *
> + * A per-domain lock that protects the list of alternate p2m's.
> + * Any operation that walks the list needs to acquire this lock.
> + * Additionally, before destroying an alternate p2m all VCPU's
> + * in the target domain must be paused.
> + */
> +
> +declare_mm_lock(altp2mlist)
> +#define altp2m_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_lock)
> +#define altp2m_unlock(d) mm_unlock(&(d)->arch.altp2m_lock)

This is unlocking an altp2mlist lock type; and the lock above is
protecting an altp2m list, not an altp2m.  Please use
"altp2m_list_lock", both for the lock itself and the macro that locks
it.

Also, even after reading Tim's comment and going through the whole
series, I still don't have a clear idea in what circumstances the
various locks (p2m, altp2mlist, and altp2m) will be acquired after one
another, such that this "sandwiched" lock structure is required.

Please include a comment brief description of what codepaths might
cause different pairs of locks to be acquired, so that someone coming
and looking at this code doesn't have to try to figure it out
themselves.

Everything else looks OK to me.  Thanks.

 -George