From: Chao Fan <fanc.fnst@cn.fujitsu.com>
To: Borislav Petkov <bp@alien8.de>
Cc: <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
<linux-efi@vger.kernel.org>, <linux-acpi@vger.kernel.org>,
<tglx@linutronix.de>, <mingo@redhat.com>, <hpa@zytor.com>,
<keescook@chromium.org>, <bhe@redhat.com>,
<msys.mizuma@gmail.com>, <indou.takao@jp.fujitsu.com>,
<caoj.fnst@cn.fujitsu.com>
Subject: Re: [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
Date: Tue, 13 Nov 2018 09:57:30 +0800 [thread overview]
Message-ID: <20181113015730.GB7453@localhost.localdomain> (raw)
In-Reply-To: <20181112145433.GF8167@zn.tnic>
On Mon, Nov 12, 2018 at 03:54:33PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:41PM +0800, Chao Fan wrote:
>> In order to parse SRAT table and get memory information, RSDP pointer
>> should be found. In kernel, there are three methods to get RSDP:
>> EFI condition, BIOS condition and KEXEC condition. The first works
>> for EFI condition.
>
>"condition"?
>
>Also, please explain shortly what all those abbreviations mean: think
>of a person reading your commit message who doesn't have any clue from
>ACPI.
I will try to improve in next version.
>
>> Imitate ACPI code and EFI code to dig RSDP pointer from EFI tables.
>> Process: boot_param->systab->efi_config_table->RSDP.
>> Based on efi_init(), efi_config_init(), efi_config_parse_tables().
>>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>> arch/x86/boot/compressed/acpitb.c | 96 +++++++++++++++++++++++++++++++
>> 1 file changed, 96 insertions(+)
>> create mode 100644 arch/x86/boot/compressed/acpitb.c
>>
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> new file mode 100644
>> index 000000000000..56b54b0e0889
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -0,0 +1,96 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define BOOT_CTYPE_H
>> +#include "misc.h"
>> +#include "error.h"
>> +
>> +#include <linux/efi.h>
>> +#include <asm/efi.h>
>> +#include <linux/numa.h>
>> +#include <linux/acpi.h>
>> +
>> +/* Search EFI table for RSDP table. */
>> +static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>
>This is just silly: the function returns void and has a single parameter
>which is an *output* parameter?!
>
>Why isn't the signature
>
>static acpi_physical_address *efi_get_rsdp_addr(void)
>
>instead?
I will change the function style.
>
>> +{
>> +#ifdef CONFIG_EFI
>> + efi_system_table_t *systab;
>> + bool efi_64 = false;
>
>You're setting it below already, why here too?
I will drop.
>
>> + void *config_tables;
>> + struct efi_info *e;
>> + char *sig;
>> + int size;
>> + int i;
>> +
>> + e = &boot_params->efi_info;
>> + sig = (char *)&e->efi_loader_signature;
>> +
>> + if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4))
>> + efi_64 = true;
>> + else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
>> + efi_64 = false;
>> + else {
>> + debug_putstr("Wrong EFI loader signature.\n");
>> + return;
>> + }
>> +
>> + /* Get systab from boot params. Based on efi_init(). */
>> +#ifdef CONFIG_X86_64
>> + systab = (efi_system_table_t *)(
>> + e->efi_systab | ((__u64)e->efi_systab_hi<<32));
>
>No ugly line breaks with open braces trailing like that, pls - just let
>it stick out.
Thanks for this suggestion.
>
>> +#else
>> + if (e->efi_systab_hi || e->efi_memmap_hi) {
>> + debug_putstr("Table located above 4GB. EFI should be disabled.\n");
>
>You need to say here what really happens here:
>
> debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
>
>The same below.
I will change all the all the same message.
>
>> + return;
>> + }
>> + systab = (efi_system_table_t *)e->efi_systab;
>> +#endif
>> +
>> + if (!systab)
>> + return;
>> +
>> + /*
>> + * Get EFI tables from systab. Based on efi_config_init() and
>> + * efi_config_parse_tables(). Only dig out the config_table.
>> + */
>> + size = efi_64 ? sizeof(efi_config_table_64_t) :
>> + sizeof(efi_config_table_32_t);
>> +
>> + for (i = 0; i < systab->nr_tables; i++) {
>> + efi_guid_t guid;
>> + unsigned long table;
>
>Put the void *config_tables declaration here.
OK.
>
>> +
>> + config_tables = (void *)(systab->tables + size * i);
>> + if (efi_64) {
>> + efi_config_table_64_t *tmp_table;
>> +
>> + tmp_table = (efi_config_table_64_t *)config_tables;
>> + guid = tmp_table->guid;
>> + table = tmp_table->table;
>> +#ifndef CONFIG_64BIT
>
>Above you have CONFIG_X86_64, here CONFIG_64BIT. Please use one only.
>
>Also, use IS_ENABLED() instead.
OK.
>
>> + if (table >> 32) {
>> + debug_putstr("Table located above 4G. EFI should be disabled.\n");
>> + return;
>> + }
>> +#endif
>> + } else {
>> + efi_config_table_32_t *tmp_table;
>> +
>> + tmp_table = (efi_config_table_32_t *)config_tables;
>> + guid = tmp_table->guid;
>> + table = tmp_table->table;
>> + }
>> +
>> + /*
>> + * Get RSDP from EFI tables.
>> + * If ACPI20 table found, use it.
>> + * If ACPI20 table not found, but ACPI table found,
>> + * use the ACPI table.
>> + */
>
>That comment is the opposite of what the code does. Also, why is that
>comment needed at all? If anything, it should say *why* ACPI_TABLE_GUID
>is preferred and then the fallback to ACPI_20_TABLE_GUID is done - not
>*what* it does. That's easily visible in the code.
I will drop the comment.
Thanks,
Chao Fan
>
>Thx.
>
>--
>Regards/Gruss,
> Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
next prev parent reply other threads:[~2018-11-13 1:58 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-12 9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
2018-11-12 9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
2018-11-12 14:54 ` Borislav Petkov
2018-11-13 1:57 ` Chao Fan [this message]
2018-11-12 9:46 ` [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory Chao Fan
2018-11-12 15:27 ` Borislav Petkov
2018-11-13 2:10 ` Chao Fan
2018-11-13 10:09 ` Borislav Petkov
2018-11-12 9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
2018-11-12 9:50 ` Chao Fan
2018-11-12 17:43 ` Masayoshi Mizuma
2018-11-13 2:12 ` Chao Fan
2018-11-13 16:11 ` Masayoshi Mizuma
2018-11-13 17:22 ` Borislav Petkov
2018-11-13 17:54 ` Borislav Petkov
2018-11-13 20:06 ` Masayoshi Mizuma
2018-11-13 21:51 ` Borislav Petkov
2018-11-14 6:12 ` Chao Fan
2018-11-14 18:30 ` Borislav Petkov
2018-11-19 1:16 ` Chao Fan
2018-11-13 17:51 ` Borislav Petkov
2018-11-14 1:54 ` Chao Fan
2018-11-14 1:59 ` Chao Fan
2018-11-14 18:33 ` Borislav Petkov
2018-11-12 9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
2018-11-12 20:52 ` Masayoshi Mizuma
2018-11-13 2:43 ` Chao Fan
2018-11-12 21:51 ` Masayoshi Mizuma
2018-11-13 2:45 ` Chao Fan
2018-11-16 11:16 ` Borislav Petkov
2018-11-19 2:08 ` Chao Fan
2018-11-20 6:18 ` Chao Fan
2018-11-12 9:46 ` [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter " Chao Fan
2018-11-16 13:50 ` Borislav Petkov
2018-11-19 1:31 ` Chao Fan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181113015730.GB7453@localhost.localdomain \
--to=fanc.fnst@cn.fujitsu.com \
--cc=bhe@redhat.com \
--cc=bp@alien8.de \
--cc=caoj.fnst@cn.fujitsu.com \
--cc=hpa@zytor.com \
--cc=indou.takao@jp.fujitsu.com \
--cc=keescook@chromium.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=msys.mizuma@gmail.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).