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

Re: [Minios-devel] [UNIKRAFT PATCHv4 18/43] plat/include: Define address offsets of boot stack and pagetable



Hi all,

On 09.07.2018 12:10, Wei Chen wrote:
Hi Julien,

-----Original Message-----
From: Julien Grall <julien.grall@xxxxxxx>
Sent: 2018年7月9日 4:19
To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
simon.kuenzer@xxxxxxxxx
Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 18/43] plat/include: Define
address offsets of boot stack and pagetable

Hi Wei,

On 07/06/2018 10:03 AM, Wei Chen wrote:
If we place the boot stack and pagetable in BSS section. These
areas are not easy to be reused after changing to newstack. So

s/newstack/a new stack/ or "the new stack".



Ok

in Arm64, we want to place the pagetable and boot stack behind

s/behind/after/

Got it.


the end of image.
In this case, once we change to newstack or we have new pagetable,
these two areas can be reclaimed very easy.

I am wondering whether it would be worth to introduce an "init" section.
This would make easier to reclaim the region and avoid hardcoded offset
below.

If we have a lot of such init functions or data, it would be good to have
a init section. If not, the freed small init section it's not easy managed.
Because VA and PA is 1:1 mapped.



Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
---
   plat/common/include/arm/arm64/cpu_defs.h | 90 ++++++++++++++++++++++++
   plat/common/include/arm/cpu_defs.h       | 47 +++++++++++++
   2 files changed, 137 insertions(+)
   create mode 100644 plat/common/include/arm/arm64/cpu_defs.h
   create mode 100644 plat/common/include/arm/cpu_defs.h

diff --git a/plat/common/include/arm/arm64/cpu_defs.h
b/plat/common/include/arm/arm64/cpu_defs.h
new file mode 100644
index 0000000..b7eba93
--- /dev/null
+++ b/plat/common/include/arm/arm64/cpu_defs.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: BSD-3-Clause */

IIRC, the goal of SPDX is to avoid to copy the full header afterwards.
Can we please do one or the other but not both?


Actually, the copyright header confused me a lot. I think I need a guide
@Simon Kuenzer (simon.kuenzer@xxxxxxxxx) : (


Hum, good question. We actually treated the SPDX header as optional addition and require you to provide at least the License text. If a file has none of both, the project license applies as fall-back. With SPDX we originally wanted to enable automated tooling that uses those headers to check for licenses compatibilities. But we did not try it yet and SPDX might be incomplete for some files. But in general I have my concerns that SPDX alone is enough for defining a license for the code (please not note that I am not a lawyer). This is the reason why we ended up with both for all files.

+/*
+ * Authors: Wei Chen <wei.chen@xxxxxxx>
+ *
+ * Copyright (c) 2018, Arm Ltd. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#ifndef __CPU_ARM_64_DEFS_H__
+#define __CPU_ARM_64_DEFS_H__
+
+#ifndef _BITUL
+
+#ifdef __ASSEMBLY__
+
+/* Linkage for ARM */
+#define __ALIGN .align 2
+#define __ALIGN_STR ".align 2"
+
+#define ALIGN __ALIGN
+#define ALIGN_STR __ALIGN_STR
+
+#define ENTRY(name)    \
+.globl name;           \
+ALIGN;                 \
+name:
+
+#define GLOBAL(name)   \
+.globl name;           \
+name:
+
+#define END(name)      \
+.size name, .-name
+
+#define ENDPROC(name)  \
+.type name, %function; \
+END(name)
+
+#define _AC(X,Y)    X
+#define _AT(T,X)    X
+
+#else
+#define __AC(X,Y)   (X##Y)
+#define _AC(X,Y)    __AC(X,Y)
+#define _AT(T,X)    ((T)(X))
+#endif
+
+#define _BITUL(x)   (_AC(1,UL) << (x))
+#define _BITULL(x)  (_AC(1,ULL) << (x))

None of the code above seem to belong to this patch.


Ok, I will merge them to other patches.

+
+#endif
+
+/* Define the address offset of boot stack and pagetable */
+#define PAGE_SIZE      __PAGE_SIZE
+#define PAGE_SHIFT     __PAGE_SHIFT
+#define STACK_SIZE     __STACK_SIZE
+#define PGD_PAGE_OFFSET         0
+#define PUD_PAGE_OFFSET         (PGD_PAGE_OFFSET + PAGE_SIZE)
+#define PMD_PAGE_OFFSET         (PUD_PAGE_OFFSET + PAGE_SIZE * 2)
+#define PTE_PAGE_OFFSET         (PMD_PAGE_OFFSET + PAGE_SIZE)

PGD, PUD, PMD are linuxism that does not make sense without any
documentation. Could we just name them L0, L1, L2...?


Em, OK.

+#define PAGE_TABLE_SIZE         (PAGE_SIZE * 5)

You probably want to document where the 5 comes from and also the
page-table area setup.

Yes, that makes sense.

+#define STACK_TOP_OFFSET (PAGE_TABLE_SIZE + STACK_SIZE)
+
+#endif /* __CPU_ARM_64_DEFS_H__ */
diff --git a/plat/common/include/arm/cpu_defs.h
b/plat/common/include/arm/cpu_defs.h
new file mode 100644
index 0000000..cd5a436
--- /dev/null
+++ b/plat/common/include/arm/cpu_defs.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Wei Chen <wei.chen@xxxxxxx>
+ *
+ * Copyright (c) 2018, Arm Ltd. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#ifndef __PLAT_CMN_ARM_CPU_DEFS_H__
+#define __PLAT_CMN_ARM_CPU_DEFS_H__
+
+#if defined(__ARM_32__)
+#include "arm/cpu_defs.h"

Likely this belongs to a previous patch.


Ok

+#elif defined(__ARM_64__)
+#include "arm64/cpu_defs.h"
+#else
+#error "Add cpu_defs.h for current architecture."
+#endif
+
+
+#endif /* __PLAT_CMN_ARM_CPU_DEFS_H__ */


Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

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