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

[Julien Grall <julien.grall@arm.com>]

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@arm.com>
>> ---
>>  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@lists.xen.org
http://lists.xen.org/xen-devel