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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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.
Regards,

--
Julien Grall

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