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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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