Re: [Xen-devel] [PATCH 1/2] x86/altp2m: use __get_gfn_type_access to avoid lock conflicts

[Tamas Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Aug 9, 2016 at 1:55 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 09.08.16 at 01:56, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> Use __get_gfn_type_access instead of get_gfn_type_access when checking
>> the hostp2m entries during altp2m mem_access setting and gfn remapping
>> to avoid a lock conflict which can make dom0 freeze.
>
> You fail to mention why the resulting code is nevertheless correct:
>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1787,8 +1787,8 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
>> p2m_domain *hp2m,
>>      if ( !mfn_valid(mfn) )
>>      {
>>
>> -        mfn = get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> -                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
>> +        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 
>> 0);
>
> In this case the respective p2m lock is already being held.
>
>> @@ -2563,8 +2563,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
>> int idx,
>>      /* Check host p2m if no valid entry in alternate */
>>      if ( !mfn_valid(mfn) )
>>      {
>> -        mfn = get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> -                                  P2M_ALLOC | P2M_UNSHARE, &page_order);
>> +        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 
>> 0);
>
> In this case, however, only ap2m's lock is being held, yet you query
> hp2m, so it's not immediately obvious whether the change is correct,
> and it would seem to me that you'd want to hold the gfn lock until
> after the subsequent ap2m->set_entry() (to make sure the two don't
> go out of sync). Since taking hp2m's lock can't nest inside any of its
> ap2m-s' ones, that'd be a slightly more intrusive adjustment.
>

True, we should lock the hp2m just before we lock the ap2m here,
similar to the mem_access path, as we can't do gfn_lock after the
altp2m is already locked.

Tamas

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