Re: [PATCH] efi/x86: Clean up the eboot code a bit

From: Ard Biesheuvel
Date: Mon May 14 2018 - 02:47:58 EST

On 14 May 2018 at 08:43, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> So I looked at arch/x86/boot/compressed/eboot.c to improve a printk message and
> ended up with the cleanups below.
>
> Only build tested.
>
> Thanks,
>
> Ingo
>
> =================>
> Subject: efi/x86: Clean up the eboot code
> From: Ingo Molnar <mingo@xxxxxxxxxx>
> Date: Mon May 14 08:33:40 CEST 2018
>
> Various small cleanups:
>
> - Standardize printk messages:
>
> 'alloc' => 'allocate'
> 'mem' => 'memory'
>
> also put variable names in printk messages between quotes.
>
> - Align mass-assignments vertically for better readability
>
> - Break multi-line function prototypes at the name where possible,
> not in the middle of the parameter list
>
> - Use a newline before return statements consistently.
>
> - Use curly braces in a balanced fashion.
>
> - Remove stray newlines.
>
> No change in functionality.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: linux-efi@xxxxxxxxxxxxxxx
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>

Thanks Ingo

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>

> ---
> arch/x86/boot/compressed/eboot.c | 247 +++++++++++++++++++--------------------
> 1 file changed, 126 insertions(+), 121 deletions(-)
>
> Index: tip/arch/x86/boot/compressed/eboot.c
> ===================================================================
> --- tip.orig/arch/x86/boot/compressed/eboot.c
> +++ tip/arch/x86/boot/compressed/eboot.c
> @@ -34,9 +34,9 @@ static void setup_boot_services##bits(st
> \
> table = (typeof(table))sys_table; \
> \
> - c->runtime_services = table->runtime; \
> - c->boot_services = table->boottime; \
> - c->text_output = table->con_out; \
> + c->runtime_services = table->runtime; \
> + c->boot_services = table->boottime; \
> + c->text_output = table->con_out; \
> }
> BOOT_SERVICES(32);
> BOOT_SERVICES(64);
> @@ -64,6 +64,7 @@ static inline efi_status_t __open_volume
> efi_printk(sys_table, "Failed to open volume\n");
>
> *__fh = fh;
> +
> return status;
> }
>
> @@ -90,6 +91,7 @@ static inline efi_status_t __open_volume
> efi_printk(sys_table, "Failed to open volume\n");
>
> *__fh = fh;
> +
> return status;
> }
>
> @@ -140,16 +142,16 @@ __setup_efi_pci(efi_pci_io_protocol_t *p
>
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, &rom);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for rom\n");
> + efi_printk(sys_table, "Failed to allocate memory for 'rom'\n");
> return status;
> }
>
> memset(rom, 0, sizeof(*rom));
>
> - rom->data.type = SETUP_PCI;
> - rom->data.len = size - sizeof(struct setup_data);
> - rom->data.next = 0;
> - rom->pcilen = pci->romsize;
> + rom->data.type = SETUP_PCI;
> + rom->data.len = size - sizeof(struct setup_data);
> + rom->data.next = 0;
> + rom->pcilen = pci->romsize;
> *__rom = rom;
>
> status = efi_call_proto(efi_pci_io_protocol, pci.read, pci,
> @@ -186,8 +188,7 @@ free_struct:
> }
>
> static void
> -setup_efi_pci32(struct boot_params *params, void **pci_handle,
> - unsigned long size)
> +setup_efi_pci32(struct boot_params *params, void **pci_handle, unsigned long size)
> {
> efi_pci_io_protocol_t *pci = NULL;
> efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> @@ -226,13 +227,11 @@ setup_efi_pci32(struct boot_params *para
> params->hdr.setup_data = (unsigned long)rom;
>
> data = (struct setup_data *)rom;
> -
> }
> }
>
> static void
> -setup_efi_pci64(struct boot_params *params, void **pci_handle,
> - unsigned long size)
> +setup_efi_pci64(struct boot_params *params, void **pci_handle, unsigned long size)
> {
> efi_pci_io_protocol_t *pci = NULL;
> efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> @@ -271,7 +270,6 @@ setup_efi_pci64(struct boot_params *para
> params->hdr.setup_data = (unsigned long)rom;
>
> data = (struct setup_data *)rom;
> -
> }
> }
>
> @@ -301,7 +299,7 @@ static void setup_efi_pci(struct boot_pa
> size, (void **)&pci_handle);
>
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for pci_handle\n");
> + efi_printk(sys_table, "Failed to allocate memory for 'pci_handle'\n");
> return;
> }
>
> @@ -347,8 +345,7 @@ static void retrieve_apple_device_proper
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> size + sizeof(struct setup_data), &new);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table,
> - "Failed to alloc mem for properties\n");
> + efi_printk(sys_table, "Failed to allocate memory for 'properties'\n");
> return;
> }
>
> @@ -364,9 +361,9 @@ static void retrieve_apple_device_proper
> new->next = 0;
>
> data = (struct setup_data *)(unsigned long)boot_params->hdr.setup_data;
> - if (!data)
> + if (!data) {
> boot_params->hdr.setup_data = (unsigned long)new;
> - else {
> + } else {
> while (data->next)
> data = (struct setup_data *)(unsigned long)data->next;
> data->next = (unsigned long)new;
> @@ -479,8 +476,8 @@ setup_uga64(void **uga_handle, unsigned
> /*
> * See if we have Universal Graphics Adapter (UGA) protocol
> */
> -static efi_status_t setup_uga(struct screen_info *si, efi_guid_t *uga_proto,
> - unsigned long size)
> +static efi_status_t
> +setup_uga(struct screen_info *si, efi_guid_t *uga_proto, unsigned long size)
> {
> efi_status_t status;
> u32 width, height;
> @@ -509,23 +506,24 @@ static efi_status_t setup_uga(struct scr
> goto free_handle;
>
> /* EFI framebuffer */
> - si->orig_video_isVGA = VIDEO_TYPE_EFI;
> + si->orig_video_isVGA = VIDEO_TYPE_EFI;
>
> - si->lfb_depth = 32;
> - si->lfb_width = width;
> - si->lfb_height = height;
> -
> - si->red_size = 8;
> - si->red_pos = 16;
> - si->green_size = 8;
> - si->green_pos = 8;
> - si->blue_size = 8;
> - si->blue_pos = 0;
> - si->rsvd_size = 8;
> - si->rsvd_pos = 24;
> + si->lfb_depth = 32;
> + si->lfb_width = width;
> + si->lfb_height = height;
> +
> + si->red_size = 8;
> + si->red_pos = 16;
> + si->green_size = 8;
> + si->green_pos = 8;
> + si->blue_size = 8;
> + si->blue_pos = 0;
> + si->rsvd_size = 8;
> + si->rsvd_pos = 24;
>
> free_handle:
> efi_call_early(free_pool, uga_handle);
> +
> return status;
> }
>
> @@ -607,7 +605,7 @@ struct boot_params *make_boot_params(str
> status = efi_low_alloc(sys_table, 0x4000, 1,
> (unsigned long *)&boot_params);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc lowmem for boot params\n");
> + efi_printk(sys_table, "Failed to allocate lowmem for boot params\n");
> return NULL;
> }
>
> @@ -623,9 +621,9 @@ struct boot_params *make_boot_params(str
> * Fill out some of the header fields ourselves because the
> * EFI firmware loader doesn't load the first sector.
> */
> - hdr->root_flags = 1;
> - hdr->vid_mode = 0xffff;
> - hdr->boot_flag = 0xAA55;
> + hdr->root_flags = 1;
> + hdr->vid_mode = 0xffff;
> + hdr->boot_flag = 0xAA55;
>
> hdr->type_of_loader = 0x21;
>
> @@ -633,6 +631,7 @@ struct boot_params *make_boot_params(str
> cmdline_ptr = efi_convert_cmdline(sys_table, image, &options_size);
> if (!cmdline_ptr)
> goto fail;
> +
> hdr->cmd_line_ptr = (unsigned long)cmdline_ptr;
> /* Fill in upper bits of command line address, NOP on 32 bit */
> boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;
> @@ -669,10 +668,12 @@ struct boot_params *make_boot_params(str
> boot_params->ext_ramdisk_size = (u64)ramdisk_size >> 32;
>
> return boot_params;
> +
> fail2:
> efi_free(sys_table, options_size, hdr->cmd_line_ptr);
> fail:
> efi_free(sys_table, 0x4000, (unsigned long)boot_params);
> +
> return NULL;
> }
>
> @@ -684,7 +685,7 @@ static void add_e820ext(struct boot_para
> unsigned long size;
>
> e820ext->type = SETUP_E820_EXT;
> - e820ext->len = nr_entries * sizeof(struct boot_e820_entry);
> + e820ext->len = nr_entries * sizeof(struct boot_e820_entry);
> e820ext->next = 0;
>
> data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
> @@ -698,8 +699,8 @@ static void add_e820ext(struct boot_para
> params->hdr.setup_data = (unsigned long)e820ext;
> }
>
> -static efi_status_t setup_e820(struct boot_params *params,
> - struct setup_data *e820ext, u32 e820ext_size)
> +static efi_status_t
> +setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_size)
> {
> struct boot_e820_entry *entry = params->e820_table;
> struct efi_info *efi = &params->efi_info;
> @@ -820,11 +821,11 @@ static efi_status_t alloc_e820ext(u32 nr
> }
>
> struct exit_boot_struct {
> - struct boot_params *boot_params;
> - struct efi_info *efi;
> - struct setup_data *e820ext;
> - __u32 e820ext_size;
> - bool is64;
> + struct boot_params *boot_params;
> + struct efi_info *efi;
> + struct setup_data *e820ext;
> + __u32 e820ext_size;
> + bool is64;
> };
>
> static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -854,15 +855,15 @@ static efi_status_t exit_boot_func(efi_s
> signature = p->is64 ? EFI64_LOADER_SIGNATURE : EFI32_LOADER_SIGNATURE;
> memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));
>
> - p->efi->efi_systab = (unsigned long)sys_table_arg;
> - p->efi->efi_memdesc_size = *map->desc_size;
> - p->efi->efi_memdesc_version = *map->desc_ver;
> - p->efi->efi_memmap = (unsigned long)*map->map;
> - p->efi->efi_memmap_size = *map->map_size;
> + p->efi->efi_systab = (unsigned long)sys_table_arg;
> + p->efi->efi_memdesc_size = *map->desc_size;
> + p->efi->efi_memdesc_version = *map->desc_ver;
> + p->efi->efi_memmap = (unsigned long)*map->map;
> + p->efi->efi_memmap_size = *map->map_size;
>
> #ifdef CONFIG_X86_64
> - p->efi->efi_systab_hi = (unsigned long)sys_table_arg >> 32;
> - p->efi->efi_memmap_hi = (unsigned long)*map->map >> 32;
> + p->efi->efi_systab_hi = (unsigned long)sys_table_arg >> 32;
> + p->efi->efi_memmap_hi = (unsigned long)*map->map >> 32;
> #endif
>
> return EFI_SUCCESS;
> @@ -880,17 +881,17 @@ static efi_status_t exit_boot(struct boo
> struct efi_boot_memmap map;
> struct exit_boot_struct priv;
>
> - map.map = &mem_map;
> - map.map_size = &map_sz;
> - map.desc_size = &desc_size;
> - map.desc_ver = &desc_version;
> - map.key_ptr = &key;
> - map.buff_size = &buff_size;
> - priv.boot_params = boot_params;
> - priv.efi = &boot_params->efi_info;
> - priv.e820ext = NULL;
> - priv.e820ext_size = 0;
> - priv.is64 = is64;
> + map.map = &mem_map;
> + map.map_size = &map_sz;
> + map.desc_size = &desc_size;
> + map.desc_ver = &desc_version;
> + map.key_ptr = &key;
> + map.buff_size = &buff_size;
> + priv.boot_params = boot_params;
> + priv.efi = &boot_params->efi_info;
> + priv.e820ext = NULL;
> + priv.e820ext_size = 0;
> + priv.is64 = is64;
>
> /* Might as well exit boot services now */
> status = efi_exit_boot_services(sys_table, handle, &map, &priv,
> @@ -898,10 +899,11 @@ static efi_status_t exit_boot(struct boo
> if (status != EFI_SUCCESS)
> return status;
>
> - e820ext = priv.e820ext;
> - e820ext_size = priv.e820ext_size;
> + e820ext = priv.e820ext;
> + e820ext_size = priv.e820ext_size;
> +
> /* Historic? */
> - boot_params->alt_mem_k = 32 * 1024;
> + boot_params->alt_mem_k = 32 * 1024;
>
> status = setup_e820(boot_params, e820ext, e820ext_size);
> if (status != EFI_SUCCESS)
> @@ -914,8 +916,8 @@ static efi_status_t exit_boot(struct boo
> * On success we return a pointer to a boot_params structure, and NULL
> * on failure.
> */
> -struct boot_params *efi_main(struct efi_config *c,
> - struct boot_params *boot_params)
> +struct boot_params *
> +efi_main(struct efi_config *c, struct boot_params *boot_params)
> {
> struct desc_ptr *gdt = NULL;
> efi_loaded_image_t *image;
> @@ -963,7 +965,7 @@ struct boot_params *efi_main(struct efi_
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> sizeof(*gdt), (void **)&gdt);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for gdt structure\n");
> + efi_printk(sys_table, "Failed to allocate memory for 'gdt' structure\n");
> goto fail;
> }
>
> @@ -971,7 +973,7 @@ struct boot_params *efi_main(struct efi_
> status = efi_low_alloc(sys_table, gdt->size, 8,
> (unsigned long *)&gdt->address);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to alloc mem for gdt\n");
> + efi_printk(sys_table, "Failed to allocate memory for 'gdt'\n");
> goto fail;
> }
>
> @@ -1008,19 +1010,20 @@ struct boot_params *efi_main(struct efi_
>
> if (IS_ENABLED(CONFIG_X86_64)) {
> /* __KERNEL32_CS */
> - desc->limit0 = 0xffff;
> - desc->base0 = 0x0000;
> - desc->base1 = 0x0000;
> - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ;
> - desc->s = DESC_TYPE_CODE_DATA;
> - desc->dpl = 0;
> - desc->p = 1;
> - desc->limit1 = 0xf;
> - desc->avl = 0;
> - desc->l = 0;
> - desc->d = SEG_OP_SIZE_32BIT;
> - desc->g = SEG_GRANULARITY_4KB;
> - desc->base2 = 0x00;
> + desc->limit0 = 0xffff;
> + desc->base0 = 0x0000;
> + desc->base1 = 0x0000;
> + desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ;
> + desc->s = DESC_TYPE_CODE_DATA;
> + desc->dpl = 0;
> + desc->p = 1;
> + desc->limit1 = 0xf;
> + desc->avl = 0;
> + desc->l = 0;
> + desc->d = SEG_OP_SIZE_32BIT;
> + desc->g = SEG_GRANULARITY_4KB;
> + desc->base2 = 0x00;
> +
> desc++;
> } else {
> /* Second entry is unused on 32-bit */
> @@ -1028,15 +1031,16 @@ struct boot_params *efi_main(struct efi_
> }
>
> /* __KERNEL_CS */
> - desc->limit0 = 0xffff;
> - desc->base0 = 0x0000;
> - desc->base1 = 0x0000;
> - desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ;
> - desc->s = DESC_TYPE_CODE_DATA;
> - desc->dpl = 0;
> - desc->p = 1;
> - desc->limit1 = 0xf;
> - desc->avl = 0;
> + desc->limit0 = 0xffff;
> + desc->base0 = 0x0000;
> + desc->base1 = 0x0000;
> + desc->type = SEG_TYPE_CODE | SEG_TYPE_EXEC_READ;
> + desc->s = DESC_TYPE_CODE_DATA;
> + desc->dpl = 0;
> + desc->p = 1;
> + desc->limit1 = 0xf;
> + desc->avl = 0;
> +
> if (IS_ENABLED(CONFIG_X86_64)) {
> desc->l = 1;
> desc->d = 0;
> @@ -1044,41 +1048,41 @@ struct boot_params *efi_main(struct efi_
> desc->l = 0;
> desc->d = SEG_OP_SIZE_32BIT;
> }
> - desc->g = SEG_GRANULARITY_4KB;
> - desc->base2 = 0x00;
> + desc->g = SEG_GRANULARITY_4KB;
> + desc->base2 = 0x00;
> desc++;
>
> /* __KERNEL_DS */
> - desc->limit0 = 0xffff;
> - desc->base0 = 0x0000;
> - desc->base1 = 0x0000;
> - desc->type = SEG_TYPE_DATA | SEG_TYPE_READ_WRITE;
> - desc->s = DESC_TYPE_CODE_DATA;
> - desc->dpl = 0;
> - desc->p = 1;
> - desc->limit1 = 0xf;
> - desc->avl = 0;
> - desc->l = 0;
> - desc->d = SEG_OP_SIZE_32BIT;
> - desc->g = SEG_GRANULARITY_4KB;
> - desc->base2 = 0x00;
> + desc->limit0 = 0xffff;
> + desc->base0 = 0x0000;
> + desc->base1 = 0x0000;
> + desc->type = SEG_TYPE_DATA | SEG_TYPE_READ_WRITE;
> + desc->s = DESC_TYPE_CODE_DATA;
> + desc->dpl = 0;
> + desc->p = 1;
> + desc->limit1 = 0xf;
> + desc->avl = 0;
> + desc->l = 0;
> + desc->d = SEG_OP_SIZE_32BIT;
> + desc->g = SEG_GRANULARITY_4KB;
> + desc->base2 = 0x00;
> desc++;
>
> if (IS_ENABLED(CONFIG_X86_64)) {
> /* Task segment value */
> - desc->limit0 = 0x0000;
> - desc->base0 = 0x0000;
> - desc->base1 = 0x0000;
> - desc->type = SEG_TYPE_TSS;
> - desc->s = 0;
> - desc->dpl = 0;
> - desc->p = 1;
> - desc->limit1 = 0x0;
> - desc->avl = 0;
> - desc->l = 0;
> - desc->d = 0;
> - desc->g = SEG_GRANULARITY_4KB;
> - desc->base2 = 0x00;
> + desc->limit0 = 0x0000;
> + desc->base0 = 0x0000;
> + desc->base1 = 0x0000;
> + desc->type = SEG_TYPE_TSS;
> + desc->s = 0;
> + desc->dpl = 0;
> + desc->p = 1;
> + desc->limit1 = 0x0;
> + desc->avl = 0;
> + desc->l = 0;
> + desc->d = 0;
> + desc->g = SEG_GRANULARITY_4KB;
> + desc->base2 = 0x00;
> desc++;
> }
>
> @@ -1088,5 +1092,6 @@ struct boot_params *efi_main(struct efi_
> return boot_params;
> fail:
> efi_printk(sys_table, "efi_main() failed!\n");
> +
> return NULL;
> }