Re: [PATCH v4 2/4] x86/boot: Add acpitb.c to parse acpi tables

[Dou Liyang]

At 07/23/2018 05:29 PM, Chao Fan wrote:
> Imitate the ACPI code to parse ACPI tables. Functions are simplified
> cause some operations are not needed here.
> And also, this method won't influence the initialization of ACPI.
>
> Signed-off-by: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>

Hi Fan,

I know you got the code from acpica subsystem and EFI code... and do
many adaptation work for KASLR. It's awesome!

I think you can add some other simple comments.

  - what differences between your function and the function you based on
    and why did you do that?

... to make this more credible and easy to remember the details as time
goes on.

Also some concerns below.
> ---
[...]
> +	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
> +		efi_64 = false;
> +	else {
> +		debug_putstr("Wrong efi loader signature.\n");

s/efi/EFI/, also need fix in the comments below.

> +		return false;
> +	}
> +
[...]
> +		/*
> +		 * Get rsdp from efi tables.
> +		 * If we find acpi table, go on searching for acpi20 table.
> +		 * If we didn't get acpi20 table then use acpi table.
> +		 * If neither acpi table nor acpi20 table found,
> +		 * return false.
> +		 */
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)) && !acpi_20) {
> +			*rsdp_addr = (acpi_physical_address)table;
> +			acpi_20 = false;
> +			find_rsdp = true;
> +		} else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
> +			*rsdp_addr = (acpi_physical_address)table;
> +			acpi_20 = true;
> +			return true;

If we find the ACPI 2.0, we will return immediately, so the variable and
logic of _acpi_20_ is redundant.

Thanks,
	dou