lkml.org 
[lkml]   [2019]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v4] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel
Date
On 4/11/19 2:14 AM, Borislav Petkov wrote:
> On Mon, Apr 08, 2019 at 11:10:17PM +0000, Junichi Nomura wrote:
>> -#ifdef CONFIG_EFI
>> - unsigned long systab, systab_tables, config_tables;
>> - unsigned int nr_tables;
>> +static void efi_read_boot_params(void)
>> +{
>> + struct efi_setup_data *esd;
>> struct efi_info *ei;
>> - bool efi_64;
>> - int size, i;
>> char *sig;
>>
>> + kexec_efi_setup_data = efi_get_kexec_setup_data_addr();
>
> Why is that written here and tested in another function?!?

Both efi_get_rsdp_addr() and kexec_get_rsdp_addr() need to check
the result of efi_get_kexec_setup_data_addr(); the former to check
whether to exit early, the latter to use the address of the tables.
I thought it's better to store the result instead of calling twice.

>> +
>> ei = &boot_params->efi_info;
>> sig = (char *)&ei->efi_loader_signature;
>>
>> if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
>> efi_64 = true;
>> + efi_booted = true;
>
> What is that ugliness for? Have you heard of functions returning values?

Same as above. I didn't want to do signature check twice, in
efi_get_rsdp_addr() and kexec_get_rsdp_addr().
Also, the signature check has 2 return values, whether it was 32bit
or 64bit, and whether the signature was valid or not.
I could return one of them via pointer passed parameter but I thought
it's a little bit ugly. Or I could encode them as something like
EFI_SIGNATURE_64, EFI_SIGNATURE_32, and EFI_SIGNATURE_INVALID.
But I'm not sure it's good to introduce such a thing just for here.

>> +static acpi_physical_address efi_get_rsdp_addr(void)
>> +{
>> +#ifdef CONFIG_EFI
>> + unsigned long config_tables;
>> + unsigned int nr_tables;
>> +
>> + efi_read_boot_params();
>
> Why do you read boot params here?
>
> No, no, no.
>
> First you do
>
> efi_get_rsdp_addr()
>
> if you cannot get an address, you

But efi_get_rsdp_addr() needs to check whether the kernel was
kexec booted to avoid accessing invalid EFI table address.
efi_get_kexec_setup_data_addr() is the only method I know
to check if it was kexec-booted.

> - parse boot params
> - then parse EFI tables from the address the kexeced kernel received
>
> No intermixing of code paths and assigning variables in one function and
> using them in another.

Yeah, I don't like that. But if we are to handle 32bit EFI case,
efi_get_rsdp_addr() and kexec_get_rsdp_addr() become full of
duplication.

> You were on the right track with v3...

--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
\
 
 \ /
  Last update: 2019-04-11 01:37    [W:0.129 / U:10.500 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site