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

[Jianyong Wu <Jianyong.Wu@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Friday, January 29, 2021 9:50 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; sharan.santhanam@xxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Justin He <Justin.He@xxxxxxx>
> Subject: Re: [PATCH] arm/entry: set x0 before cache maintenance.
>
> 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.
>
Ok, I can move this comment up.

> It would also make sense to introduce a new symbol (maybe start_binary) to
> avoid relying on _dtb to always be first.
>
Sounds good, but I'm not sure how to do that. As _dtb is assigned in link 
script, where should we assigned the new symbol? In the entry.S or in the link 
script?

> > +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.
>

I don't think the dtb address can be dynamic.  The   __libkvmplat_entry is a 
"C" function and the "_dtb" will be its parameter.
Maybe we can use "const" to bond the parameter of __libkvmplat_entry. Does that 
do your trick?

> However, this line will load the DTB from the image. Can you clarify the
> expected behavior?

This load ops only pass the dtb address to the __libkvmplat_entry.  And I'm not 
sure why it's not clear about that. Can you give more info?

Thanks
Jianyong

>
> >   bl clean_and_invalidate_dcache_range
> >
> >   /* Disable the MMU and D-Cache. */
> >
>
> Cheers,
>
> --
> Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.