| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7cc5e26c-42fc-a700-ae19-608920cafe44@amd.com>
Date: Mon, 18 Nov 2024 09:19:04 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Baoquan He <bhe@...hat.com>, linux-kernel@...r.kernel.org,
mingo@...nel.org, bp@...en8.de
Cc: x86@...nel.org
Subject: Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement
xxx_is_setup_data()
On 11/17/24 19:08, Baoquan He wrote:
> Functions memremap_is_setup_data() and early_memremap_is_setup_data()
> share completely the same process and handling, except of the
> different memremap/unmap invocations.
>
> So add helper __memremap_is_setup_data() to extract the common part,
> parameter 'early' is used to decide what kind of memremap/unmap
> APIs are called. This simplifies codes a lot by removing the duplicated
> codes, and also removes the similar code comment above them.
>
> And '__ref' is added to __memremap_is_setup_data() to suppress below
> section mismatch warning:
>
> ARNING: modpost: vmlinux: section mismatch in reference: __memremap_is_setup_data+0x5f (section: .text) ->
> early_memunmap (section: .init.text)
>
> Signed-off-by: Baoquan He <bhe@...hat.com>
> ---
> arch/x86/mm/ioremap.c | 108 +++++++++++++++---------------------------
> 1 file changed, 38 insertions(+), 70 deletions(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 8d29163568a7..68d78e2b1203 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -628,12 +628,13 @@ static bool memremap_is_efi_data(resource_size_t phys_addr,
> return false;
> }
>
> +#define SD_SIZE sizeof(struct setup_data)
Nit, I still think you should use "sizeof(*data)" in the code instead of
creating a #define.
> /*
> * Examine the physical address to determine if it is boot data by checking
> * it against the boot params setup_data chain.
> */
> -static bool memremap_is_setup_data(resource_size_t phys_addr,
> - unsigned long size)
> +static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
Oh, I see why the __ref is needed now, because this calls an __init
function based on the early bool.
While this nicely consolidates the checking, I'll let the x86
maintainers decide whether they like that an __init function is calling
a non __init function.
> + bool early)
> {
> struct setup_indirect *indirect;
> struct setup_data *data;
> @@ -641,31 +642,45 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>
> paddr = boot_params.hdr.setup_data;
> while (paddr) {
> - unsigned int len;
> + unsigned int len, size;
>
> if (phys_addr == paddr)
> return true;
>
> - data = memremap(paddr, sizeof(*data),
> - MEMREMAP_WB | MEMREMAP_DEC);
> + if (early)
> + data = early_memremap_decrypted(paddr, SD_SIZE);
> + else
> + data = memremap(paddr, SD_SIZE,
> + MEMREMAP_WB | MEMREMAP_DEC);
> if (!data) {
> pr_warn("failed to memremap setup_data entry\n");
> return false;
> }
>
> + size = SD_SIZE;
> +
> paddr_next = data->next;
> len = data->len;
>
> if ((phys_addr > paddr) &&
> - (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> - memunmap(data);
> + (phys_addr < (paddr + SD_SIZE + len))) {
> + if (early)
> + early_memunmap(data, SD_SIZE);
> + else
> + memunmap(data);
> return true;
> }
>
> if (data->type == SETUP_INDIRECT) {
> - memunmap(data);
> - data = memremap(paddr, sizeof(*data) + len,
> - MEMREMAP_WB | MEMREMAP_DEC);
> + size += len;
> + if (early) {
> + early_memunmap(data, SD_SIZE);
> + data = early_memremap_decrypted(paddr, size);
> + } else {
> + memunmap(data);
> + data = memremap(paddr, size,
> + MEMREMAP_WB | MEMREMAP_DEC);
> + }
> if (!data) {
> pr_warn("failed to memremap indirect setup_data\n");
> return false;
> @@ -679,7 +694,10 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
> }
> }
>
> - memunmap(data);
> + if (early)
> + early_memunmap(data, size);
> + else
> + memunmap(data);
>
> if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
> return true;
> @@ -689,68 +707,18 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>
> return false;
> }
> +#undef SD_SIZE
>
> -/*
> - * Examine the physical address to determine if it is boot data by checking
> - * it against the boot params setup_data chain (early boot version).
> - */
> -static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
> - unsigned long size)
> +static bool memremap_is_setup_data(resource_size_t phys_addr,
> + unsigned long size)
> {
> - struct setup_indirect *indirect;
> - struct setup_data *data;
> - u64 paddr, paddr_next;
> -
> - paddr = boot_params.hdr.setup_data;
> - while (paddr) {
> - unsigned int len, size;
> -
> - if (phys_addr == paddr)
> - return true;
> -
> - data = early_memremap_decrypted(paddr, sizeof(*data));
> - if (!data) {
> - pr_warn("failed to early memremap setup_data entry\n");
> - return false;
> - }
> -
> - size = sizeof(*data);
> -
> - paddr_next = data->next;
> - len = data->len;
> -
> - if ((phys_addr > paddr) &&
> - (phys_addr < (paddr + sizeof(struct setup_data) + len))) {
> - early_memunmap(data, sizeof(*data));
> - return true;
> - }
> -
> - if (data->type == SETUP_INDIRECT) {
> - size += len;
> - early_memunmap(data, sizeof(*data));
> - data = early_memremap_decrypted(paddr, size);
> - if (!data) {
> - pr_warn("failed to early memremap indirect setup_data\n");
> - return false;
> - }
> -
> - indirect = (struct setup_indirect *)data->data;
> -
> - if (indirect->type != SETUP_INDIRECT) {
> - paddr = indirect->addr;
> - len = indirect->len;
> - }
> - }
> -
> - early_memunmap(data, size);
> -
> - if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
> - return true;
> -
> - paddr = paddr_next;
> - }
> + return __memremap_is_setup_data(phys_addr, false);
> +}
>
> - return false;
> +static bool early_memremap_is_setup_data(resource_size_t phys_addr,
This should retain the original __init reference.
Thanks,
Tom
> + unsigned long size)
> +{
> + return __memremap_is_setup_data(phys_addr, true);
> }
>
> /*
Powered by blists - more mailing lists