[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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;
Cc: Justin He <Justin.He@xxxxxxx>
Subject: Re: [PATCH] arm/entry: set x0 before cache maintenance.


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 

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?


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.