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

[Julien Grall <julien.grall@xxxxxxx>]

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

> in Arm64, we want to place the pagetable and boot stack behind
s/behind/after/

> 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.
> 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?
> +/*
> + * 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.

> +
> +#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...?
> +#define PAGE_TABLE_SIZE         (PAGE_SIZE * 5)
You probably want to document where the 5 comes from and also the 
page-table area setup.
> +#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.

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