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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: White, Edmund H
>Sent: Tuesday, July 07, 2015 9:20 AM
>
>On 07/07/2015 08:22 AM, Tim Deegan wrote:
>> At 16:04 +0100 on 07 Jul (1436285059), George Dunlap wrote:
>>> On 07/01/2015 07:09 PM, Ed White 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)
>>>> +
>>>> +/* P2M lock (per-altp2m-table)
>>>> + *
>>>> + * This protects all queries and updates to the p2m table.
>>>> + * Queries may be made under the read lock but all modifications
>>>> + * need the main (write) lock.
>>>> + *
>>>> + * The write lock is recursive as it is common for a code path to
>>>> +look
>>>> + * up a gfn and later mutate it.
>>>> + */
>>>> +
>>>> +declare_mm_rwlock(altp2m);
>>>> +#define p2m_lock(p)                         \
>>>> +{                                           \
>>>> +    if ( p2m_is_altp2m(p) )                 \
>>>> +        mm_write_lock(altp2m, &(p)->lock);  \
>>>> +    else                                    \
>>>> +        mm_write_lock(p2m, &(p)->lock);     \
>>>> +}
>>>>  #define p2m_unlock(p)         mm_write_unlock(&(p)->lock);
>>>>  #define gfn_lock(p,g,o)       p2m_lock(p)
>>>>  #define gfn_unlock(p,g,o)     p2m_unlock(p)
>>>
>>> I've just taken on the role of mm maintainer, and so this late in a
>>> series, having Tim's approval and also having Andy's reviewed-by, I'd
>>> normally just skim through and Ack it as a matter of course.  But I
>>> just wouldn't feel right giving this my maintainer's ack without
>>> understanding the locking a bit better.  So please bear with me here.
>>>
>>> I see in the cover letter that you "sandwiched" the altp2mlist lock
>>> between p2m and altp2m at Tim's suggestion.  But I can't find the
>>> discussion where that was suggested (it doesn't seem to be in
>>> response to v1 patch 5),
>>
>> I suggested changing the locking order here:
>> http://lists.xenproject.org/archives/html/xen-devel/2015-01/msg01824.h
>> tml
>>
>> Cheers,
>>
>> Tim.
>>
>
>And Tim, Andrew and I subsequently discussed this specific approach in a
>phone meeting.
>
>Ed

Here is a brief description of the approach that was taken - 

The altp2m design implements a set of p2m's that are derived from the host p2m, 
by lazy copy, and can then have specific modifications applied. If a change is 
made to the host p2m when the domain is in altp2m mode, that change must be 
propagated to any altp2m that contains a valid copy of the old host p2m entry, 
primarily to guarantee that no altp2m maps a mfn that is not mapped by the host 
p2m, as that would compromise Xen. Tim suggested that the only safe way to 
perform that propagation would be in the code that writes the new EPTE, but by 
that point there is invariably a lock on the host p2m, and that lock was 
frequently acquired some way higher in the call stack. It is not safe to walk 
the list of altp2m's without holding the list lock, so in that situation the 
acquisition order is hostp2m->altp2mlist->altp2m. Although Ed did the 
implementation, the idea to split the p2m lock type using the macros in 
mm-locks.h was suggested by Tim and Andrew in a phone meeting attended by Tim, 
Andrew, Ravi, and Ed in early May.

Cheers,
Ravi


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