Re: [Xen-devel] [PATCH] xen/arm: p2m: Prevent deadlock when using memaccess

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 03/06/2018 12:37 PM, Julien Grall wrote:
> On 06/03/18 11:06, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hi Sergej,
>
>>
>> On 02/28/2018 04:25 PM, Julien Grall wrote:
>>> Commit 7d623b358a4 "arm/mem_access: Add long-descriptor based gpt"
>>> assumed the read-write lock can be taken recursively. However, this
>>> assumption is wrong and will lead to deadlock when the lock is
>>> contended.
>>>
>>> To avoid the nested lock, rework the locking in get_page_from_gva and
>>> p2m_mem_access_check_and_get_page. The latter will now be called
>>> without
>>> the p2m lock. The new locking in p2m_mem_accces_check_and_get_page will
>>> not cover the translation of the VA to an IPA.
>>>
>>> This is fine because we can't promise that the stage-1 page-table have
>>> changed behind our back (they are under guest control). Modification in
>>> the stage-2 page-table can now happen, but I can't issue any potential
>>> issue here except with the break-before-make sequence used when
>>> updating
>>> page-table. gva_to_ipa may fail if the sequence is executed at the same
>>> on another CPU. In that case we would fallback in the software lookup
>>> path.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>>
>> The patch looks good to me. However, I did not understand the following
>> sentence in the commit message: "... but I can't issue any potential
>> issue here except with the break-before-make sequence used when updating
>> page-table". What issue did you mean? Thank you.
>
> To avoid breaking the coherency when updating the page-table the Arm
> Arm recommends to follow a break-before-make sequence (see G4-4916 in
> ARM DDI 0487C.a). It a 4 steps approach:
>
> 1. Replace the old entry with an invalid entry
> 2. Invalidate the cached old entries with a broadcasting TLB
> invalidation instruction
> 3. Wait for the completion of the TLB instruction with a dsb followed
> by an isb
> 4. Write the new entry
>
> The P2M code is using this sequence, so an address translation walk
> using hardware instruction may fail if another CPU happen to update
> (e.g breaking a superpage) the P2M entry at the same time/ This is
> because there are a small window where the entry does not exist.

Makes sense. Thank you.

Cheers,
~Sergej

>
> If you are interested in more details, you can look at the talk I gave
> a couple of years ago (see [1]).
>
>>
>> Reviewed-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>
> Thank you.
>
> Cheers,
>
> [1] https://schd.ws/hosted_files/xensummit2016/49/slides.pdf
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel