[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 17/08/16 21:08, Shanker Donthineni wrote:


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@xxxxxxx>

---
     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)';

Feel free to send a patch for this explaining why we would need to have the full check. But I don't think it is worth to test every case and therefore increasing the number of cycles of this function for faults we don't care so far.

Regards,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.