Re: [Xen-devel] [PATCH] arm/mem_access: properly handle traps caused by no-longer current settings

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Wed, Aug 3, 2016 at 5:43 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Tamas,
>
> On 02/08/16 23:47, Tamas K Lengyel wrote:
>>
>> On Tue, Aug 2, 2016 at 4:05 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>>>
>>> On 02/08/2016 22:34, Tamas K Lengyel wrote:
>>>>
>>>> On Tue, Aug 2, 2016 at 2:02 PM, Julien Grall <julien.grall@xxxxxxx>
>>>> wrote:
>>>>>
>>>>> On 02/08/2016 19:53, Tamas K Lengyel wrote:
>>>
>>> Well, the data abort can only be a permission fault if memaccess is inuse
>>> so
>>> far. Unless there is another race condition in the memaccess code and in
>>> this case this is not the fault of the guest. So sending a data abort to
>>> the
>>> guest will not really help to know what's going on.
>>
>>
>> From my perspective it doesn't matter whether the fault is injected
>> into the guest or not when mem_access is not in use. Since that's the
>> default behavior right now, my opinion is that it should get
>> reinjected but that's beyond the scope of mem_access itself so it's up
>> to you to decide. If you really prefer to have the mem_access check
>> just be a void function and not inject a fault to the guest, I'm
>> certainly fine with that.
>
>
> I would prefer to see the function p2m_mem_access_check unchanged. The
> current behavior of the function is to return either "the fault is not for
> me" or "I handled the fault".
>
> With this patch, this function could also return "this might be a race
> condition, try again".
>
> The function p2m_mem_access_check is returning a boolean. You cannot encode
> 3 states in a boolean. So you could not hand over the fault to another
> helper.
>
>>
>>> Also, you are assuming that it will never be possible in the future to
>>> have
>>> another usage of the permission fault. By returning false you say "I
>>> handled
>>> the fault, it is not necessary to hand over to someone else".
>>
>>
>> I only return false here if mem_access is enabled.
>
>
> But this code can never be reached when mem_access is not enabled. And once
> mem_access is turned on, it is never possible to turned off back.
>
> If you think that in the future mem_access can be turned off, then the code
> could be racy. IHMO it is already kind of racy because the code assumes that
> p2m->mem_access_enabled will be seen well before the P2M was changed.

Doing a p2m locking around setting it should be fine though.

>
>> If any other system
>> in the future is going to make use of these permissions then it needs
>> to be carefully evaluated what the handover setup should be when the
>> mem_access state doesn't seem to be the reason for the violation. I
>> can forsee some issues if one system would like the instruction to be
>> reexecuted while the other to do something else. As this all being
>> hypothetical at this point I don't really know what to do with this
>> right now.
>
>
> IHMO, right now, the best solution is to re-execute the instruction in any
> case and keep p2m_mem_access_check unchanged.
>

Sure, that works for me.

Tamas

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