Re: [PATCH 2/2] efi: implement interruptible runtime services

* Re: [PATCH 2/2] efi: implement interruptible runtime services
       [not found] ` <1450434591-31104-2-git-send-email-sylvain.chouleur@gmail.com>
@ 2016-01-06 12:58   ` Matt Fleming
  2016-01-06 15:57     ` Sylvain Chouleur
  0 siblings, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2016-01-06 12:58 UTC (permalink / raw)
  To: sylvain.chouleur
  Cc: linux-efi, Sylvain Chouleur, linux-kernel, H. Peter Anvin,
	Ingo Molnar, Thomas Gleixner

On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
> 
> This patch adds an implementation of EFI runtime services wappers which
> allow interrupts and preemption during execution of BIOS code.
> 
> It's needed by Broxton platform which requires the OS to do the write
> of the non volatile variables. To do that, the OS will receive an
> interrupt coming from the firmware to notify that it must write the
> data.
> 
> BIOS code must be executed using a specific PGD. This is currently
> done by efi_call() which force value of CR3 before calling BIOS and
> restore previous value after.
> 
> We use a dedicated kthread which will execute all interruptible runtime
> services. This efi_kthread is special because it has it's own mm_struct
> whereas kthread should not have one. This mm_struct contains the EFI PGD
> so the scheduler will load it before running any BIOS code.
> 
> Obviously, an interruptible runtime service must never be called from an
> interrupt context. EFI pstore is the only use case here, that's why we
> require it to be disabled when activating interruptible runtime services.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
> ---
>  arch/x86/Kconfig                          |  14 +++
>  arch/x86/include/asm/efi.h                |   1 +
>  arch/x86/platform/efi/Makefile            |   1 +
>  arch/x86/platform/efi/efi_32.c            |   5 +
>  arch/x86/platform/efi/efi_64.c            |   5 +
>  arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
>  6 files changed, 217 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..3ddd5a9cab30 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,20 @@ config EFI_MIXED
>  
>  	   If unsure, say N.
>  
> +if X86_64
> +config EFI_INTERRUPTIBLE
> +	bool "Interruptible Efi Runtime Services"
> +	depends on EFI && !EFI_VARS_PSTORE
> +	default n
> +	help
> +          This is an interruptible implementation of efi runtime
> +	  services wrappers. If you say Y, BIOS code could be
> +	  interrupted and preempted as a standard process. Enable this
> +	  only if you are sure that your BIOS will support
> +	  interruptible efi calls
> +	  In doubt, say "N".
> +endif
> +

These words should be a little more assertive. Almost everyone is
going to want to turn this feature off.

> +static efi_status_t
> +non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
> +			 __attribute__((unused)) efi_guid_t *vendor,
> +			 __attribute__((unused)) u32 attr,
> +			 __attribute__((unused)) unsigned long data_size,
> +			 __attribute__((unused)) void *data)
> +{
> +	pr_err("efi_interruptible: non bloking operation is not allowed\n");
> +	return EFI_UNSUPPORTED;
> +}

Typo, s/bloking/blocking/

> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	static struct efivars generic_efivars;
> +	static struct efivar_operations generic_ops;
> +
> +	generic_ops.get_variable = efi.get_variable;
> +	generic_ops.set_variable = efi.set_variable;
> +	generic_ops.get_next_variable = efi.get_next_variable;
> +	generic_ops.query_variable_store = efi_query_variable_store;
> +
> +	efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +

What's the purpose of this? The only reason you'd want to do this that
I can think of is because you'd want efi-pstore to run and attempt to
save some crash data for you. But you can't build with efi-pstore
enable and CONFIG_EFI_INTERRUPTIBLE.

I'd be tempted to just delete this notifier code.

> +static struct task_struct *efi_kworker_task;
> +static struct efivar_operations interruptible_ops;
> +static __init int efi_interruptible_init(void)
> +{
> +	int ret;
> +
> +	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> +					  "efi_kthread");
> +	if (IS_ERR(efi_kworker_task)) {
> +		pr_err("efi_interruptible: Failed to create kthread\n");
> +		ret = PTR_ERR(efi_kworker_task);
> +		efi_kworker_task = NULL;
> +		return ret;
> +	}
> +
> +	efi_kworker_task->mm = mm_alloc();
> +	efi_kworker_task->active_mm = efi_kworker_task->mm;
> +	efi_kworker_task->mm->pgd = efi_get_pgd();
> +	wake_up_process(efi_kworker_task);
> +
> +	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
> +
> +	interruptible_ops.get_variable = get_variable_interruptible;
> +	interruptible_ops.set_variable = set_variable_interruptible;
> +	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
> +	interruptible_ops.get_next_variable = get_next_variable_interruptible;
> +	interruptible_ops.query_variable_store = efi_query_variable_store;
> +	return efivars_register(&interruptible_efivars, &interruptible_ops,
> +				efivars_kobject());
> +}
> +

Is there some way we can match DMI/PCI identifiers for Broxton so that
we don't start seeing people accidentally running with preemptible EFI
services on non-BXT hardware?

^ permalink raw reply	[flat|nested] 11+ messages in thread