Re: [kvm-unit-tests PATCH v2 17/18] arm64: efi: Switch to our own stack

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

On 27/02/2024 19:21, Andrew Jones wrote:
> We don't want to map EFI_BOOT_SERVICES_DATA regions, so move the
> stack from its EFI_BOOT_SERVICES_DATA region to EFI_LOADER_CODE,
> which we always map. We'll still map the stack as R/W instead of
> R/X because we split EFI_LOADER_CODE regions on the _etext boundary
> and map addresses before _etext as R/X and the rest as R/W.
>
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Thanks,

Nikos

> ---
>   arm/efi/crt0-efi-aarch64.S | 22 +++++++++++++++++-----
>   lib/arm/setup.c            |  4 ----
>   lib/memregions.c           |  6 ------
>   3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/arm/efi/crt0-efi-aarch64.S b/arm/efi/crt0-efi-aarch64.S
> index 5fd3dc94dae8..71ce2794f059 100644
> --- a/arm/efi/crt0-efi-aarch64.S
> +++ b/arm/efi/crt0-efi-aarch64.S
> @@ -125,12 +125,18 @@ _start:
>
>       ldp             x0, x1, [sp, #16]
>
> -     /* Align sp; this is necessary due to way we store cpu0's thread_info */
> +     /*
> +      * Switch to our own stack and align sp; this is necessary due
> +      * to way we store cpu0's thread_info
> +      */
> +     adrp            x2, stacktop
> +     add             x2, x2, :lo12:stacktop
> +     and             x2, x2, #THREAD_MASK
> +     mov             x3, sp
> +     mov             sp, x2
> +     stp             xzr, xzr, [sp, #-16]!
>       mov             x29, sp
> -     mov             x30, sp
> -     and             x30, x30, #THREAD_MASK
> -     mov             sp, x30
> -     str             x29, [sp, #-16]!
> +     str             x3, [sp, #-16]!
>
>       bl              efi_main
>
> @@ -140,3 +146,9 @@ _start:
>
>   0:  ldp             x29, x30, [sp], #32
>       ret
> +
> +     .section        .data
> +
> +.balign 65536
> +.space 65536
> +stacktop:
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 08658b9a222b..d535cec88709 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -340,10 +340,6 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
>   {
>       efi_status_t status;
>
> -     struct thread_info *ti = current_thread_info();
> -
> -     memset(ti, 0, sizeof(*ti));
> -
>       exceptions_init();
>
>       memregions_init(arm_mem_regions, NR_MEM_REGIONS);
> diff --git a/lib/memregions.c b/lib/memregions.c
> index 3c6f751eb4f2..53fc0c7cfc58 100644
> --- a/lib/memregions.c
> +++ b/lib/memregions.c
> @@ -114,12 +114,6 @@ void memregions_efi_init(struct efi_boot_memmap *mem_map,
>                       break;
>               case EFI_LOADER_DATA:
>                       break;
> -             case EFI_BOOT_SERVICES_DATA:
> -                     /*
> -                      * FIXME: This would ideally be MR_F_RESERVED, but the
> -                      * primary stack is in a region of this EFI type.
> -                      */
> -                     break;
>               case EFI_PERSISTENT_MEMORY:
>                       r.flags = MR_F_PERSISTENT;
>                       break;
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.