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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 30 May 2016, Julien Grall wrote:
> 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."

What about adding a lengthy and detailed description of the erratum
elsewhere and just having a one liner at the call sites, such as:

/* Erratum #834220: check Stage1 translation does not generate faults first! */

so that developers can easily grep for #834220 through the code to have
the full explanation?

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