Re: [PATCH v5 09/14] efi: passing kexec necessary efi data via setup_data

[Dave Young <dyoung@redhat.com>]

On 12/11/13 at 11:20pm, Borislav Petkov wrote:
> On Mon, Dec 09, 2013 at 05:42:22PM +0800, Dave Young wrote:
> > Add a new setup_data type SETUP_EFI for kexec use.
> > Passing the saved fw_vendor, runtime, config tables and efi runtime mappings.
> > 
> > When entering virtual mode, directly mapping the efi runtime ragions which
> > we passed in previously. And skip the step to call SetVirtualAddressMap.
> > 
> > Specially for HP z420 workstation we need save the smbios physical address.
> > The kernel boot sequence proceeds in the following order.  Step 2
> > requires efi.smbios to be the physical address.  However, I found that on
> > HP z420 EFI system table has a virtual address of SMBIOS in step 1.  Hence,
> > we need set it back to the physical address with the smbios in
> > efi_setup_data.  (When it is still the physical address, it simply sets
> > the same value.)
> > 
> > 1. efi_init() - Set efi.smbios from EFI system table
> > 2. dmi_scan_machine() - Temporary map efi.smbios to access SMBIOS table
> > 3. efi_enter_virtual_mode() - Map EFI ranges
> > 
> > Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
> > HP z420 workstation.
> > 
> > v2: refresh based on previous patch changes, code cleanup.
> > v3: use ioremap instead of phys_to_virt for efi_setup
> > v5: improve some code structure per comments from Matt
> >     Boris: improve code structure, spell fix, etc.
> >     Improve changelog from Toshi.
> >     change the variable efi_setup to the physical address of efi setup_data
> >     instead of the ioremapped virt address
> > 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> >  arch/x86/include/asm/efi.h            |  11 ++
> >  arch/x86/include/uapi/asm/bootparam.h |   1 +
> >  arch/x86/kernel/setup.c               |   3 +
> >  arch/x86/platform/efi/efi.c           | 195 ++++++++++++++++++++++++++++++----
> >  4 files changed, 187 insertions(+), 23 deletions(-)
> 
> ...
> 
> > @@ -115,6 +116,25 @@ static int __init setup_storage_paranoia(char *arg)
> >  }
> >  early_param("efi_no_storage_paranoia", setup_storage_paranoia);
> >  
> > +void __init parse_efi_setup(u64 phys_addr)
> > +{
> > +	struct setup_data *sd;
> > +
> > +	if (!efi_enabled(EFI_64BIT)) {
> > +		pr_warn("SETUP_EFI not supported on 32-bit\n");
> > +		return;
> > +	}
> 
> Shouldn't this function be in two versions in efi_64.c and efi_32.c?
> This way you don't need this check with cryptic printk message.

Ok, will update.

> 
> > +
> > +	sd = early_memremap(phys_addr, sizeof(struct setup_data));
> > +	if (!sd) {
> > +		pr_warn("efi: early_memremap setup_data failed\n");
> > +		return;
> > +	}
> > +	efi_setup = phys_addr + sizeof(struct setup_data);
> > +	nr_efi_runtime_map = (sd->len - sizeof(struct efi_setup_data)) /
> > +			     sizeof(efi_memory_desc_t);
> > +	early_memunmap(sd, sizeof(struct setup_data));
> > +}
> >  
> >  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> >  {
> > @@ -494,18 +514,28 @@ static int __init efi_systab_init(void *phys)
> >  {
> >  	if (efi_enabled(EFI_64BIT)) {
> >  		efi_system_table_64_t *systab64;
> > +		struct efi_setup_data *data = NULL;
> >  		u64 tmp = 0;
> >  
> > +		if (efi_setup) {
> > +			data = early_memremap(efi_setup, sizeof(*data));
> > +			if (!data)
> > +				return -ENOMEM;
> > +		}
> >  		systab64 = early_memremap((unsigned long)phys,
> >  					 sizeof(*systab64));
> >  		if (systab64 == NULL) {
> >  			pr_err("Couldn't map the system table!\n");
> > +			if (data)
> > +				early_memunmap(data, sizeof(*data));
> >  			return -ENOMEM;
> >  		}
> >  
> >  		efi_systab.hdr = systab64->hdr;
> > -		efi_systab.fw_vendor = systab64->fw_vendor;
> > -		tmp |= systab64->fw_vendor;
> > +
> > +		efi_systab.fw_vendor = data ? (unsigned long)data->fw_vendor :
> > +					      systab64->fw_vendor;
> > +		tmp |= efi_systab.fw_vendor;
> >  		efi_systab.fw_revision = systab64->fw_revision;
> >  		efi_systab.con_in_handle = systab64->con_in_handle;
> >  		tmp |= systab64->con_in_handle;
> > @@ -519,15 +549,20 @@ static int __init efi_systab_init(void *phys)
> >  		tmp |= systab64->stderr_handle;
> >  		efi_systab.stderr = systab64->stderr;
> >  		tmp |= systab64->stderr;
> > -		efi_systab.runtime = (void *)(unsigned long)systab64->runtime;
> > -		tmp |= systab64->runtime;
> > +		efi_systab.runtime = data ?
> > +				     (void *)(unsigned long)data->runtime :
> > +				     (void *)(unsigned long)systab64->runtime;
> > +		tmp |= (unsigned long)efi_systab.runtime;
> >  		efi_systab.boottime = (void *)(unsigned long)systab64->boottime;
> >  		tmp |= systab64->boottime;
> >  		efi_systab.nr_tables = systab64->nr_tables;
> > -		efi_systab.tables = systab64->tables;
> > -		tmp |= systab64->tables;
> > +		efi_systab.tables = data ? (unsigned long)data->tables :
> > +					   systab64->tables;
> > +		tmp |= efi_systab.tables;
> >  
> >  		early_memunmap(systab64, sizeof(*systab64));
> > +		if (data)
> > +			early_memunmap(data, sizeof(*data));
> >  #ifdef CONFIG_X86_32
> >  		if (tmp >> 32) {
> >  			pr_err("EFI data located above 4GB, disabling EFI.\n");
> > @@ -631,6 +666,61 @@ static int __init efi_memmap_init(void)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * For kexec kernel there's some special config table entries which could be
> > + * converted to virtual addresses after entering virtual mode. In kexec kernel
> > + * we need the physical addresses instead, thus passing them via setup_data
> > + * and update the entries to physical addresses in this function.
> 
> Rewrite:
> 
> "A number of config table entries get remapped to virtual addresses
> after entering EFI virtual mode. However, the kexec kernel requires
> their physical addresses therefore we pass them via setup_data and
> correct those entries to their respective physical addresses here."

Thanks for the rewriting. Will use.

> 
> > + *
> > + * Currently only handles smbios which is necessary for HP z420.
> 
> Didn't we say that this behavior is coming from a generic UEFI fw
> implementation and if so, no need to mention z420?

Yes, but till now I only reproduced this on z420 so how about below:
"Currently only handles smbios which is necessary for some firmware
implementation." 

It might be general but also might only related to some version of
the general firmware implementation, it's hard to say...

> 
> > + */
> > +static int __init efi_reuse_config(u64 tables, int nr_tables)
> > +{
> > +	int i, sz, ret = 0;
> > +	void *p, *tablep;
> > +	struct efi_setup_data *data;
> > +
> > +	if (!efi_setup)
> > +		return 0;
> > +
> > +	if (!efi_enabled(EFI_64BIT))
> > +		return 0;
> > +
> > +	data = early_memremap(efi_setup, sizeof(*data));
> > +	if (!data) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	if (!data->smbios)
> > +		goto out_memremap;
> > +
> > +	sz = sizeof(efi_config_table_64_t);
> > +
> > +	p = tablep = early_memremap(tables, nr_tables * sz);
> > +	if (!p) {
> > +		pr_err("Could not map Configuration table!\n");
> > +		ret = -ENOMEM;
> > +		goto out_memremap;
> > +	}
> > +
> > +	for (i = 0; i < efi.systab->nr_tables; i++) {
> > +		efi_guid_t guid;
> > +
> > +		guid = ((efi_config_table_64_t *)p)->guid;
> > +
> > +		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> > +			((efi_config_table_64_t *)p)->table = data->smbios;
> > +		p += sz;
> > +	}
> > +	early_memunmap(tablep, nr_tables * sz);
> > +
> > +out_memremap:
> > +	early_memunmap(data, sizeof(*data));
> > +out:
> > +	return ret;
> > +}
> > +
> >  void __init efi_init(void)
> >  {
> >  	efi_char16_t *c16;
> > @@ -676,6 +766,8 @@ void __init efi_init(void)
> >  		efi.systab->hdr.revision >> 16,
> >  		efi.systab->hdr.revision & 0xffff, vendor);
> >  
> > +	efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
> > +
> >  	if (efi_config_init(arch_tables))
> >  		return;
> >  
> > @@ -886,6 +978,50 @@ out_krealloc:
> >  }
> >  
> >  /*
> > + * Map efi regions which was passed via setup_data. The virt_addr is a fixed
> 
> 			   were

Will update

> 
> > + * addr which was used in first kernel in case kexec boot.
> 						 ^
> 						of a

Will update

> 
> > + */
> > +static int __init map_regions_fixed(void)
> > +{
> > +	int i, s, ret = 0;
> > +	u64 end, systab;
> > +	unsigned long size;
> > +	efi_memory_desc_t *md;
> > +	struct efi_setup_data *data;
> > +
> > +	s = sizeof(*data) + nr_efi_runtime_map * sizeof(data->map[0]);
> > +	data = early_memremap(efi_setup, s);
> > +	if (!data) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> 
> newline.

Will remove

> 
> > +	for (i = 0, md = data->map; i < nr_efi_runtime_map; i++, md++) {
> > +		efi_map_region_fixed(md); /* FIXME: add error handling */
> > +		size = md->num_pages << PAGE_SHIFT;
> > +		end = md->phys_addr + size;
> > +
> > +		systab = (u64) (unsigned long) efi_phys.systab;
> > +		if (md->phys_addr <= systab && systab < end) {
> > +			systab += md->virt_addr - md->phys_addr;
> > +			efi.systab = (efi_system_table_t *)(unsigned long)systab;
> > +		}
> > +		ret = save_runtime_map(md, i);
> 
> Wait a minute, this is executed in the second, kexec-ed kernel, right?
> Why do we need to save the map there too?

Yes, nested kexec need this.

> 
> > +		if (ret)
> > +			goto out_save_runtime;
> > +	}
> > +
> > +	early_memunmap(data, s);
> > +	return 0;
> > +
> > +out_save_runtime:
> > +	kfree(efi_runtime_map);
> > +	nr_efi_runtime_map = 0;
> > +	early_memunmap(data, s);
> > +out:
> > +	return ret;
> > +}
> > +
> > +/*
> >   * This function will switch the EFI runtime services to virtual mode.
> >   * Essentially, we look through the EFI memmap and map every region that
> >   * has the runtime attribute bit set in its memory descriptor into the
> > @@ -901,12 +1037,16 @@ out_krealloc:
> >   * so that we're in a different address space when calling a runtime
> >   * function. For function arguments passing we do copy the PGDs of the
> >   * kernel page table into ->trampoline_pgd prior to each call.
> > + *
> > + * Specially for kexec boot, efi runtime maps in previous kernel should
> > + * be passed in via setup_data. In that case runtime ranges will be mapped
> > + * to the same virtual addresses exactly same as the ones in previous kernel.
> 
> "... to the same ..exactly same as ... " sounds funny. What's wrong with
> 
> "... to the same virtual addresses as the first kernel."

Will use above one. 

--
Thanks for review
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec