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

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

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:
>>>
>>> Hello Tamas,
>>>
>>> Thank you for taking care of this bug.
>>>
>>> On 02/08/2016 19:53, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> When mem_access settings change, the active vCPUs may still cause a
>>>> violation
>>>> until the TLB gets flushed. Instead of just reinjecting the violation to
>>>> the
>>>> guest, in this patch we direct the vCPU to retry the access where
>>>> appropriate or we crash the domain where the mem_access settings are
>>>> corrupted.
>>>>
>>>
>>> With this patch p2m_mem_access_check will always return false. So I am
>>> not
>>> sure why you want to return in p2m_mem_access_check.
>>
>>
>> That's not the case, it returns true if mem_access is not enabled on
>> the domain, which means whatever caused the trap wasn't mem_access and
>> thus we should fall back on the default behavior, which is injecting
>> the fault to the guest.
>>
>>>
>>>
>>>> Requested-by: Julien Grall <julien.grall@xxxxxxx>
>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>>> ---
>>>>  xen/arch/arm/p2m.c | 29 ++++++++++++++++++++++++++---
>>>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 40a0b80..a4b6b7b 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1657,8 +1657,26 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t
>>>> gla, const struct npfec npfec)
>>>>          return true;
>>>>
>>>>      rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
>>>> -    if ( rc )
>>>> -        return true;
>>>> +    switch (rc )
>>>> +    {
>>>> +    case -ESRCH:
>>>> +        /*
>>>> +         * If we can't find any mem_access setting for this page then
>>>> the
>>>> page
>>>> +         * might have just been removed and the event was triggered by
>>>> no
>>>> longer
>>>> +         * valid settings. The vCPU should just retry to get to the
>>>> proper error
>>>> +         * path.
>>>> +         */
>>>> +        return false;
>>>> +    case -ERANGE:
>>>> +        /*
>>>> +         * The mem_access settings are corrupted. Crashing the domain
>>>> is
>>>> the
>>>> +         * appropriate step in this case.
>>>> +         */
>>>> +        domain_crash(v->domain);
>>>> +        return false;
>>>> +    };
>>>> +
>>>> +    ASSERT(!rc);
>>>
>>>
>>>
>>> It would be easier to do:
>>>
>>> rc = p2m_mem_access_check(gpa, gva, npfec);
>>> if (!rc)
>>>   return;
>>>
>>> by
>>>
>>> p2m_mem_access_check(gpa, gva, npfec);
>>> return;
>>>
>>> in both do_trap_instr_abort_guest and do_trap_data_abort_guest.
>>>
>>> This would also helps to fallback on another permission check if in the
>>> future we decide to use permission for other reasons.
>>>
>>> Or is there any reason you may want to inject a data abort to the guest
>>> if
>>> memaccess has failed (i.e return true)?
>>>
>>
>> Yes, if the fault wasn't caused by mem_access (ie. it's not enabled on
>> the domain).
>
>
> 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.

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

> The right thing here is:
>         1) Try to handle memaccess
>         2) Re-execute the instruction
>
> The instruction will fault again if it was really a permission issue.
> Otherwise it will normally be executed.

And that's what this patch does. If mem_access is enabled it will try
to handle it, but if something doesn't add up it will opt to reexecute
the instruction. If mem_access is not in use at all, it falls back to
the default behavior.

Tamas

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