Re: [Xen-devel] [PATCH v2 15/15] xen/arm: arm64: Document Cortex-A57 erratum 834220

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 30/05/2016 16:11, Stefano Stabellini wrote:
> On Mon, 23 May 2016, Julien Grall wrote:
> > The ARM erratum applies to certain revisions of Cortex-A57. The
> > processor may report a Stage 2 translation fault as the result of
> > Stage 1 fault for load crossing a page boundary when there is a
> > permission fault or device memory fault at stage 1 and a translation
> > fault at Stage 2.
> >
> > So Xen needs to check that Stage 1 translation does not generate a fault
> > before handling the Stage 2 fault. If it is a Stage 1 translation fault,
> > return to the guest to let the processor injecting the correct fault.
> >
> > Only document it as this is already the behavior of the fault handlers.
> > Note that some optimization could be done to avoid unecessary translation
> > fault. This is because HPFAR_EL2 is valid for more use case. For the moment,
> > the code is left unmodified.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >  docs/misc/arm/silicon-errata.txt |  1 +
> >  xen/arch/arm/traps.c             | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/docs/misc/arm/silicon-errata.txt b/docs/misc/arm/silicon-errata.txt
> > index ab2e5bc..1ac365d 100644
> > --- a/docs/misc/arm/silicon-errata.txt
> > +++ b/docs/misc/arm/silicon-errata.txt
> > @@ -47,3 +47,4 @@ stable hypervisors.
> >  | ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    
> > |
> >  | ARM            | Cortex-A57      | #852523         | N/A                     
> > |
> >  | ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    
> > |
> > +| ARM            | Cortex-A57      | #834220         | N/A                     
> > |
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 3acdba0..bbd5309 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -2396,6 +2396,21 @@ static void do_trap_instr_abort_guest(struct 
> > cpu_user_regs *regs,
> >              .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> >          };
> >
> > +        /*
> > +         * Erratum #834220: The processor may report a Stage 2
> > +         * translation fault as the result of Stage 1 fault for load
> > +         * crossing a page boundary when there is a permission fault or
> > +         * device memory alignment fault at Stage 1 and a translation
> > +         * fault at Stage 2.
> > +         *
> > +         * So Xen needs to check that the Stage 1 translation does not
> > +         * generate a fault before handling stage 2 fault. If it is a Stage
> > +         * 1 translation fault, return to the guest to let the processor
> > +         * injecting the correct fault.
> > +         *
> > +         * XXX: This can be optimized to avoid some unecessary
> > +         * translation.
> > +         */
> >          if ( hsr.iabt.s1ptw )
> >              gpa = get_faulting_ipa();
> >          else
> Please write the comment only once, maybe in do_trap_hypervisor before
> the calls to do_trap_instr_abort_guest and do_trap_data_abort_guest.
I wrote this comment at these two specific places because developers and 
reviewers can spot directly that the code has been written in a specific 
way to avoid an erratum.
If we happen to fix one place and not the other, the comment would still 
be there. do_trap_hypervisor would be the wrong place for this comment 
because it is "far away" in the code and will be less likely read.
I can shrink down the message. What about:

"Erratum #834220: Xen needs to check that the Stage 1 translation does 
not generate a fault before handling Stage 2 fault. If it is a stage 1 
translation fault, return to the guest to let the project injecting the 
correct fault.
XXX: This can be optimized to avoid some unnecessary translation."

Regards,

--
Julien Grall

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