Re: [PATCH] arm/entry: set x0 before cache maintenance.

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/01/2021 03:25, Jianyong Wu wrote:
> There is a long standing issue in entry code of arm that x0 is not
> set before using it in clean_and_invalidate_dcache_range. This error
> can be caught by kvm and result in core dump.
> Here, x0 is set to the base address of dtb before cache maintain.
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> ---
>   plat/kvm/arm/entry64.S | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
> index c4de334..0526ae8 100644
> --- a/plat/kvm/arm/entry64.S
> +++ b/plat/kvm/arm/entry64.S
> @@ -92,6 +92,12 @@ ENTRY(_libkvmplat_entry)
>         add x27, x26, x17
>         add x27, x27, #__STACK_SIZE
>         sub x1, x27, x25
> +
> +       /*
> +        * set x0 to the location stores dtb as the base address of the
> +        * memory range to be cache maintained
> +        */
There is already a few lines above explaining what the clean is meant to 
do. I think it would be best to modify this comment (if you need to) 
rather than adding a new comment.
It would also make sense to introduce a new symbol (maybe start_binary) 
to avoid relying on _dtb to always be first.
> +       ldr x0, =_dtb
Unrelated to this patch, I am a bit confused with the code. The comment 
on top of __libkvmplat_entry suggests that the DTB in x0, so this 
implies the DTB can be dynamic.
However, this line will load the DTB from the image. Can you clarify the 
expected behavior?
>         bl clean_and_invalidate_dcache_range
>   
>   	/* Disable the MMU and D-Cache. */
Cheers,

--
Julien Grall