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

[Jianyong Wu <Jianyong.Wu@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, February 2, 2021 1:24 AM
> 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.
>
>
>
> 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.
>
Ok, I will send another patch to fix it.

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

Sorry to have mistaken _libkvmplat_entry to _libkvmplat_start. The 
_libkvmplat_start is a "C" function, and _libkvmplat_entry is entry code with 
no caller.

> >
> >> 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?
>

In unikraft, DTB is provided by hypervisor like qemu. Qemu/virt puts the DTB at 
1G of physical address in guest.
The "_dtb" is initialized in link script as 1G. In the _libkvmplat_entry,
we treat _dtb as both the beginning of the normal memory in unikraft and the 
start address where stores DTB.
Maybe that confused you. So I can offer another symbol to represents the 
beginning of the normal memory to distinguish with _dtb.
Then we will only use the "_dtb" as the parameter of _libkvmplat_start.

If this can do your trick, I will send another patch to fix it.

Thanks
Jianyong

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