Re: [edk2-devel] [PATCH v8 14/37] UefiCpuPkg: Add CpuMmuLib to UefiCpuPkg

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Fri, Jan 26, 2024 at 6:29 AM Chao Li <lichao@loongson.cn> wrote:
>
> Add a new library named CpuMmuLib and add a LoongArch64 instance with in
> the library.
> It provides two-stage MMU libraryinstances, PEI and DXE.
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584
>[...]
> diff --git a/UefiCpuPkg/Library/CpuMmuLib/LoongArch64/CommonMmuLib.c b/UefiCpuPkg/Library/CpuMmuLib/LoongArch64/CommonMmuLib.c
> new file mode 100644
> index 0000000000..2e852c3371
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuMmuLib/LoongArch64/CommonMmuLib.c
> @@ -0,0 +1,988 @@
> +/** @file
> +
> +  CPU Memory Map Unit Handler Library common functions.
> +
> +  Copyright (c) 2024 Loongson Technology Corporation Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Glossary:
> +    - Pgd or Pgd or PGD    - Page Global Directory
> +    - Pud or Pud or PUD    - Page Upper Directory
> +    - Pmd or Pmd or PMD    - Page Middle Directory
> +    - Pte or pte or PTE    - Page Table Entry
> +    - Val or VAL or val    - Value
> +    - Dir    - Directory
> +**/

Wow, this is one of the most bizarre patches I've seen in EDK2. You
took essentially Linux code (and the Linux way of doing paging) and
EDK2-ify'd it. Congrats, this is cursed.
You even include the way they botched what a "PTE" means back in Linux 1.0.

> +#include <Uefi.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/CpuMmuLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Register/LoongArch64/Csr.h>
> +#include "Tlb.h"
> +#include "Page.h"
> +
> +#define SWAP_PAGE_DIR              CsrRead(LOONGARCH_CSR_PGDL)

swapper_pg_dir? :P

You should seriously unscrew this patch's cursedness: disambiguate
pte_t and pteval_t (I would suggest having a PTABLE (for the lower
level page table) and a PTE for their entries), and... rename
swapper_pd_dir which makes no sense in this EFI firmware context,
where swapper is a really old, traditional UNIX name.

> +BOOLEAN  mMmuInited = FALSE;

Should probably be a STATIC, you also don't need to explicitly init it
to FALSE, as it's a static duration variable.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114877): https://edk2.groups.io/g/devel/message/114877
Mute This Topic: https://groups.io/mt/103971653/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-