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

[Julien Grall <julien@xxxxxxx>]

On 01/02/2021 15:09, Jianyong Wu wrote:
> Hi Julien,
Hi Jianyong,

> -----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?
I meant introducing a new symbol 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?
I couldn't find the caller to _libkvmplat_entry. Would you mind to give 
a pointer to the code calling it?
> > 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?
On a Linux setup, the device-tree is most of the time not embedded in 
the kernel. Instead, it is provided by the firmware (or possibly the 
toolstack in a virtualized environment).
In those setup, the device tree physical address will be provided in x0.

_libkvmplat_entry has the following comment on top:

/*
 * The registers used by _libkvmplat_start:
 * x0 - FDT pointer
 */

So I thought you were following the same behavior as Linux. But from 
what you wrote, the DTB will instead be embeded in Unikraft. Is that 
correct?
Cheers,

--
Julien Grall