Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Shanker Donthineni <shankerd@codeaurora.org>]

On 08/17/2016 06:11 AM, Julien Grall wrote:
> On 17/08/16 03:19, Shanker Donthineni wrote:
>> Hi Julien,
>
> Hello Shanker,
>
>> On 07/27/2016 12:09 PM, Julien Grall wrote:
>>> Translating a VA to a IPA is expensive. Currently, Xen is assuming that
>>> HPFAR_EL2 is only valid when the stage-2 data/instruction abort 
>>> happened
>>> during a translation table walk of a first stage translation (i.e S1PTW
>>> is set).
>>>
>>> However, based on the ARM ARM (D7.2.34 in DDI 0487A.j), the register is
>>> also valid when the data/instruction abort occured for a translation
>>> fault.
>>>
>>> With this change, the VA -> IPA translation will only happen for
>>> permission faults that are not related to a translation table of a
>>> first stage translation.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Use fsc in the switch in do_trap_data_abort_guest
>>> ---
>>>   xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index ea105f2..83a30fa 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -2382,13 +2382,28 @@ static inline paddr_t get_faulting_ipa(vaddr_t
>>> gva)
>>>       return ipa;
>>>   }
>>>
>>> +static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
>>> +{
>>> +    /*
>>> +     * HPFAR is valid if one of the following cases are true:
>>> +     *  1. the stage 2 fault happen during a stage 1 page table walk
>>> +     *  (the bit ESR_EL2.S1PTW is set)
>>> +     *  2. the fault was due to a translation fault
>>> +     *
>>> +     * Note that technically HPFAR is valid for other cases, but they
>>> +     * are currently not supported by Xen.
>>> +     */
>>> +    return s1ptw || (fsc == FSC_FLT_TRANS);
>>
>> Yes, XEN is not supporting the stage 2 access flag but we should handle
>> a stage 2 address size fault.
>
> The function hpfar_is_valid indicates whether the register HPFAR is 
> valid. If the function returns false, Xen will use the hardware do the 
> translation.
>
> It will only lead to a waste of cycle but this is fine as the address 
> size fault is not a hot path for now.
>
>> I think we should do some thing like to below to match ARM ARM.
>>
>> return s1ptw || (fsc != FSC_FLT_PERM);
>
> This does not match the ARM ARM, with this check you consider that 
> HPFAR will be valid for all the fault but permission ones which is not 
> true.
>
> I purposefully choose a white list because it is safer to use the 
> hardware to do the translation more often than the invert.
>
> So I don't see why we should handle stage 2 access flag with the 
> current Xen. If you still disagree, please explain why with a concrete 
> example.
>

Agree with you, I have suggested the above change because I saw the same 
check in Linux KVM.
As per ARM ARM, it should be 'return s1ptw || (fsc == FSC_FLT_TRANS) || 
(fsc == FSC_FLT_ACCESS) || (fsc == 0x00)';

> Regards,
>

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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