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

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

[Matt Fleming]

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?

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

[Sylvain Chouleur]

2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> 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.

This is indeed to be able to try write some crash data when we
are in a panic context.  In fact with this restoration of legacy
efi handlers on a panic we should be able to have pstore working
with CONFIG_EFI_INTERRUPTIBLE.

I say should because there is still issues with BIOS to have the
panic flow working.

>
>> +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?

I don't see how since this depends on a storage strategy more
than the SoC itself, it can be used on future platforms or we can
also get rid of it on BXT if an other storage is used for efi
variables.
Can't the KConfig protection be enough?

-- 
Sylvain

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

[Matt Fleming]

On Wed, 06 Jan, at 04:57:41PM, Sylvain Chouleur wrote:
> 2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> > On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
> >> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
> >>
> >> +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.
> 
> This is indeed to be able to try write some crash data when we
> are in a panic context.  In fact with this restoration of legacy
> efi handlers on a panic we should be able to have pstore working
> with CONFIG_EFI_INTERRUPTIBLE.
 
This makes the efivar registration more complicated because now it can
fail if someone else is holding efivars_lock. Which is a strange
semantic for a registration function.

> I say should because there is still issues with BIOS to have the
> panic flow working.

How would it work though? You'd need the kernel thread controlling
access to the flash to be run in order for any data to appear in the
EFI variable store. Except you're in the middle of a panic and all
bets are off regarding which tasks are going to be run by the
scheduler.

Until those issues are resolved, please delete the notifier code. But
honestly, I doubt this will ever be useful in practice. Setting up
panic handlers *once* you've crashed is far too late.

> >> +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?
> 
> I don't see how since this depends on a storage strategy more
> than the SoC itself, it can be used on future platforms or we can
> also get rid of it on BXT if an other storage is used for efi
> variables.
> Can't the KConfig protection be enough?

Kconfig is a last resort because it's a build-time decision and
greatly limits the flexibility of the kernel. It becomes no longer
possible to run a single kernel image with various CONFIG_* enabled on
x86 hardware - you now need a special EFI_INTERRUPTIBLE build.

Which apart from being a major headache for distributions in general
is generally frowned upon for the x86 architecture.

If there's any way at all of making this a runtime decision that would
be much better.

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

[Sylvain Chouleur]

2016-01-08 11:38 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> On Wed, 06 Jan, at 04:57:41PM, Sylvain Chouleur wrote:
>> 2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
>> > On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
>> >> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
>> >>
>> >> +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.
>>
>> This is indeed to be able to try write some crash data when we
>> are in a panic context.  In fact with this restoration of legacy
>> efi handlers on a panic we should be able to have pstore working
>> with CONFIG_EFI_INTERRUPTIBLE.
>
> This makes the efivar registration more complicated because now it can
> fail if someone else is holding efivars_lock. Which is a strange
> semantic for a registration function.

Right, I'll change the code to call legacy handlers from efi_interruptible code
directly it will be more consistent with a panic specific behavior.

>
>> I say should because there is still issues with BIOS to have the
>> panic flow working.
>
> How would it work though? You'd need the kernel thread controlling
> access to the flash to be run in order for any data to appear in the
> EFI variable store. Except you're in the middle of a panic and all
> bets are off regarding which tasks are going to be run by the
> scheduler.

The trick here is that in panic context, the cse is notified of this special
context and is asked to write data itself to the storage instead of sending
interrupt to the kernel. Then we don't need interruptible handler in
panic context. That's why we want to use legacy efi variable handlers.

>
> Until those issues are resolved, please delete the notifier code. But
> honestly, I doubt this will ever be useful in practice. Setting up
> panic handlers *once* you've crashed is far too late.

I understand, like I said above I'll modify efi_interruptible handlers to
call legacy ones in case of panic context.
I would like to avoid removing the panic part of this patch and take time
to clean it before merging the whole.

>
>> >> +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?
>>
>> I don't see how since this depends on a storage strategy more
>> than the SoC itself, it can be used on future platforms or we can
>> also get rid of it on BXT if an other storage is used for efi
>> variables.
>> Can't the KConfig protection be enough?
>
> Kconfig is a last resort because it's a build-time decision and
> greatly limits the flexibility of the kernel. It becomes no longer
> possible to run a single kernel image with various CONFIG_* enabled on
> x86 hardware - you now need a special EFI_INTERRUPTIBLE build.
>
> Which apart from being a major headache for distributions in general
> is generally frowned upon for the x86 architecture.
>
> If there's any way at all of making this a runtime decision that would
> be much better.

I think the best would be to bind this driver with the one which receives the
interrupts from CSE to write the variables. Then we would have a consistency
on the feature.
Does this seems ok for you?

-- 
Sylvain

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

[Matt Fleming]

On Fri, 08 Jan, at 02:57:13PM, Sylvain Chouleur wrote:
> 
> I understand, like I said above I'll modify efi_interruptible handlers to
> call legacy ones in case of panic context.
> I would like to avoid removing the panic part of this patch and take time
> to clean it before merging the whole.
 
OK, well at least split out the panic diddling into a separate patch
so that we can discuss the merits of it separately to the other, less
contentious changes.

> > Kconfig is a last resort because it's a build-time decision and
> > greatly limits the flexibility of the kernel. It becomes no longer
> > possible to run a single kernel image with various CONFIG_* enabled on
> > x86 hardware - you now need a special EFI_INTERRUPTIBLE build.
> >
> > Which apart from being a major headache for distributions in general
> > is generally frowned upon for the x86 architecture.
> >
> > If there's any way at all of making this a runtime decision that would
> > be much better.
> 
> I think the best would be to bind this driver with the one which receives the
> interrupts from CSE to write the variables. Then we would have a consistency
> on the feature.
> Does this seems ok for you?

I think that'd be an improvement, yeah.

[PATCH v2 0/3] efi interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

Changes in v2:
 - Split patch 1 in patches 1 and 2
 - Document (un)registration of __efivars protection
 - Return early from efivars_sysfs_exit() in case of failure
 - Improve readability
 - Update functions documentation
 - Fix typo
 - Fix checkpatch warnings
 - Warning in KConfig description

Sylvain Chouleur (3):
 efi: implement interruptible runtime services
 efi: don't use spinlocks for efi vars
 efi: use a file local lock for efivars

 arch/x86/Kconfig                          |   17 +++++
 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 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c         |   36 +++++++---
 drivers/firmware/efi/efivars.c            |   20 ++++-
 drivers/firmware/efi/vars.c               |  230 ++++++++++++++++++++++++++++++++++++++++----------------------------
 fs/efivarfs/inode.c                       |    5 +
 fs/efivarfs/super.c                       |    9 ++
 include/linux/efi.h                       |   12 ---
 12 files changed, 415 insertions(+), 117 deletions(-)

[PATCH v2 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

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                          |  17 +++
 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, 220 insertions(+)
 create mode 100644 arch/x86/platform/efi/efi_interruptible.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..37301516f4e6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1720,6 +1720,23 @@ 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---
+	  Only use this if you really know what you are doing, you
+	  possibly risk to break your BIOS.
+
+	  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
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0010c78c4998..5e9620b3688b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -111,6 +111,7 @@ extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
+extern pgd_t *efi_get_pgd(void);
 extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 2846aaab5103..dcc894d1e603 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_INTERRUPTIBLE)		+= efi_interruptible.o
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index ed5b67338294..02a3c616c59e 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -56,6 +56,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
+pgd_t *efi_get_pgd(void)
+{
+	return initial_page_table;
+}
+
 pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index d347e854a5e4..67cffb69d405 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -143,6 +143,11 @@ void efi_sync_low_kernel_mappings(void)
 		sizeof(pgd_t) * num_pgds);
 }
 
+pgd_t *efi_get_pgd(void)
+{
+	return __va(efi_scratch.efi_pgt);
+}
+
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
 	unsigned long text;
diff --git a/arch/x86/platform/efi/efi_interruptible.c b/arch/x86/platform/efi/efi_interruptible.c
new file mode 100644
index 000000000000..d1f14fcb12a4
--- /dev/null
+++ b/arch/x86/platform/efi/efi_interruptible.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * Author: Sylvain Chouleur <sylvain.chouleur@intel.com>
+ *
+ * Distributed under the terms of the GNU GPL, version 2
+ */
+
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+static DEFINE_KTHREAD_WORKER(efi_kworker);
+
+#define efi_call_interruptible(f, ...)					\
+({									\
+	efi_status_t __s;						\
+	efi_sync_low_kernel_mappings();					\
+	__kernel_fpu_begin();						\
+	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
+	__kernel_fpu_end();						\
+	__s;								\
+})
+
+struct efi_work {
+	struct kthread_work work;
+	efi_status_t status;
+	unsigned long *name_size;
+	efi_char16_t *name;
+	efi_guid_t *vendor;
+	u32 *attr;
+	unsigned long *data_size;
+	void *data;
+};
+
+static void do_get_next_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_next_variable, ew->name_size,
+					    ew->name, ew->vendor);
+}
+
+static void do_set_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(set_variable, ew->name, ew->vendor,
+					    *ew->attr, *ew->data_size,
+					    ew->data);
+}
+
+static void do_get_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_variable, ew->name, ew->vendor,
+					    ew->attr, ew->data_size, ew->data);
+}
+
+static efi_status_t execute_service(kthread_work_func_t service,
+				    unsigned long *name_size,
+				    efi_char16_t *name, efi_guid_t *vendor,
+				    u32 *attr, unsigned long *data_size,
+				    void *data)
+{
+	struct efi_work work = {
+		.name_size = name_size,
+		.name = name,
+		.vendor = vendor,
+		.attr = attr,
+		.data_size = data_size,
+		.data = data,
+	};
+
+	init_kthread_work(&work.work, service);
+	queue_kthread_work(&efi_kworker, &work.work);
+
+	flush_kthread_work(&work.work);
+	return work.status;
+}
+
+static efi_status_t get_next_variable_interruptible(unsigned long *name_size,
+					       efi_char16_t *name,
+					       efi_guid_t *vendor)
+{
+	return execute_service(do_get_next_variable_interruptible,
+			       name_size, name, vendor, NULL, NULL, NULL);
+}
+
+static efi_status_t set_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 attr,
+					  unsigned long data_size,
+					  void *data)
+{
+	return execute_service(do_set_variable_interruptible,
+			       NULL, name, vendor, &attr, &data_size, data);
+}
+
+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 blocking operation is not allowed\n");
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t get_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 *attr,
+					  unsigned long *data_size,
+					  void *data)
+{
+	return execute_service(do_get_variable_interruptible,
+			       NULL, name, vendor, attr, data_size, data);
+}
+
+static struct efivars interruptible_efivars;
+
+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,
+};
+
+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());
+}
+
+static void __exit efi_interruptible_exit(void)
+{
+	efivars_unregister(&interruptible_efivars);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &panic_nb);
+	if (efi_kworker_task) {
+		kthread_stop(efi_kworker_task);
+		mmdrop(efi_kworker_task->mm);
+	}
+}
+
+module_init(efi_interruptible_init);
+module_exit(efi_interruptible_exit);
+
+MODULE_AUTHOR("Sylvain Chouleur <sylvain.chouleur@intel.com>");
+MODULE_LICENSE("GPL");
-- 
2.6.4

[PATCH v3 0/3] efi interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

Changes in v3:
 - efivars_(un)register: use down_interruptible()
 - efivar_create_sysfs_entry: unregister new_var in case of failure
 - efi_interruptible: in case of panic call directly legacy handlers

Sylvain Chouleur (3):
 efi: implement interruptible runtime services
 efi: don't use spinlocks for efi vars
 efi: use a file local lock for efivars

 arch/x86/Kconfig                          |   17 +++
 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 |  189 ++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c         |   36 +++++--
 drivers/firmware/efi/efivars.c            |   22 +++-
 drivers/firmware/efi/vars.c               |  315 ++++++++++++++++++++++++++++++++++++++++----------------------------
 fs/efivarfs/inode.c                       |    5 -
 fs/efivarfs/super.c                       |    9 +
 include/linux/efi.h                       |   18 ---
 12 files changed, 463 insertions(+), 160 deletions(-)

[PATCH v3 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

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                          |  17 +++
 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 | 189 ++++++++++++++++++++++++++++++
 6 files changed, 218 insertions(+)
 create mode 100644 arch/x86/platform/efi/efi_interruptible.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..37301516f4e6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1720,6 +1720,23 @@ 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---
+	  Only use this if you really know what you are doing, you
+	  possibly risk to break your BIOS.
+
+	  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
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 8fd9e637629a..5b94d9dd9409 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -137,6 +137,7 @@ extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
 extern int __init efi_alloc_page_tables(void);
+extern pgd_t *efi_get_pgd(void);
 extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 2846aaab5103..ff57d3840842 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_INTERRUPTIBLE)	+= efi_interruptible.o
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 58d669bc8250..f2403e16a8bb 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -61,6 +61,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
+pgd_t *efi_get_pgd(void)
+{
+	return initial_page_table;
+}
+
 pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index b492521503fe..7886c7527cdf 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -212,6 +212,11 @@ void efi_sync_low_kernel_mappings(void)
 	memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
 }
 
+pgd_t *efi_get_pgd(void)
+{
+	return __va(efi_scratch.efi_pgt);
+}
+
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
 	unsigned long pfn, text;
diff --git a/arch/x86/platform/efi/efi_interruptible.c b/arch/x86/platform/efi/efi_interruptible.c
new file mode 100644
index 000000000000..2b24f915fa19
--- /dev/null
+++ b/arch/x86/platform/efi/efi_interruptible.c
@@ -0,0 +1,189 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * Author: Sylvain Chouleur <sylvain.chouleur@intel.com>
+ *
+ * Distributed under the terms of the GNU GPL, version 2
+ */
+
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+static DEFINE_KTHREAD_WORKER(efi_kworker);
+static bool in_panic = false;
+
+#define efi_call_interruptible(f, ...)					\
+({									\
+	efi_status_t __s;						\
+	efi_sync_low_kernel_mappings();					\
+	__kernel_fpu_begin();						\
+	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
+	__kernel_fpu_end();						\
+	__s;								\
+})
+
+struct efi_work {
+	struct kthread_work work;
+	efi_status_t status;
+	unsigned long *name_size;
+	efi_char16_t *name;
+	efi_guid_t *vendor;
+	u32 *attr;
+	unsigned long *data_size;
+	void *data;
+};
+
+static void do_get_next_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_next_variable, ew->name_size,
+					    ew->name, ew->vendor);
+}
+
+static void do_set_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(set_variable, ew->name, ew->vendor,
+					    *ew->attr, *ew->data_size,
+					    ew->data);
+}
+
+static void do_get_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_variable, ew->name, ew->vendor,
+					    ew->attr, ew->data_size, ew->data);
+}
+
+static efi_status_t execute_service(kthread_work_func_t service,
+				    unsigned long *name_size,
+				    efi_char16_t *name, efi_guid_t *vendor,
+				    u32 *attr, unsigned long *data_size,
+				    void *data)
+{
+	struct efi_work work = {
+		.name_size = name_size,
+		.name = name,
+		.vendor = vendor,
+		.attr = attr,
+		.data_size = data_size,
+		.data = data,
+	};
+
+	init_kthread_work(&work.work, service);
+	queue_kthread_work(&efi_kworker, &work.work);
+
+	flush_kthread_work(&work.work);
+	return work.status;
+}
+
+static efi_status_t get_next_variable_interruptible(unsigned long *name_size,
+						    efi_char16_t *name,
+						    efi_guid_t *vendor)
+{
+	return execute_service(do_get_next_variable_interruptible,
+			       name_size, name, vendor, NULL, NULL, NULL);
+}
+
+static efi_status_t set_variable_interruptible(efi_char16_t *name,
+					       efi_guid_t *vendor,
+					       u32 attr,
+					       unsigned long data_size,
+					       void *data)
+{
+	if (in_panic)
+		return efi.set_variable(name, vendor, attr, data_size, data);
+	return execute_service(do_set_variable_interruptible,
+			       NULL, name, vendor, &attr, &data_size, data);
+}
+
+static efi_status_t non_blocking_not_allowed(efi_char16_t *name,
+					     efi_guid_t *vendor,
+					     u32 attr,
+					     unsigned long data_size,
+					     void *data)
+{
+	if (in_panic)
+		return efi.set_variable_nonblocking(name, vendor, attr,
+						    data_size, data);
+	pr_err("efi_interruptible: non blocking operation is not allowed\n");
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t get_variable_interruptible(efi_char16_t *name,
+					       efi_guid_t *vendor,
+					       u32 *attr,
+					       unsigned long *data_size,
+					       void *data)
+{
+	if (in_panic)
+		return efi.get_variable(name, vendor, attr, data_size, data);
+	return execute_service(do_get_variable_interruptible,
+			       NULL, name, vendor, attr, data_size, data);
+}
+
+static struct efivars interruptible_efivars;
+
+static int efi_interruptible_panic_notifier_call(
+	struct notifier_block *notifier,
+	unsigned long what, void *data)
+{
+	in_panic = true;
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_nb = {
+	.notifier_call = efi_interruptible_panic_notifier_call,
+	.priority = 100,
+};
+
+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());
+}
+
+static void __exit efi_interruptible_exit(void)
+{
+	efivars_unregister(&interruptible_efivars);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &panic_nb);
+	if (efi_kworker_task) {
+		kthread_stop(efi_kworker_task);
+		mmdrop(efi_kworker_task->mm);
+	}
+}
+
+module_init(efi_interruptible_init);
+module_exit(efi_interruptible_exit);
+
+MODULE_AUTHOR("Sylvain Chouleur <sylvain.chouleur@intel.com>");
+MODULE_LICENSE("GPL");
-- 
2.6.3

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Matt Fleming]

On Wed, 13 Jan, at 05:32:42PM, Sylvain Chouleur 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                          |  17 +++
>  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 | 189 ++++++++++++++++++++++++++++++
>  6 files changed, 218 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c

Last time Peter and I talked about this patch (sometime last year), he
said he had a different approach in mind.

Peter, do you have any comments on this patch?

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..37301516f4e6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,23 @@ 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---
> +	  Only use this if you really know what you are doing, you
> +	  possibly risk to break your BIOS.
> +
> +	  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
> +
>  config SECCOMP
>  	def_bool y
>  	prompt "Enable seccomp to safely compute untrusted bytecode"
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 8fd9e637629a..5b94d9dd9409 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -137,6 +137,7 @@ extern void __init efi_map_region(efi_memory_desc_t *md);
>  extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
>  extern void efi_sync_low_kernel_mappings(void);
>  extern int __init efi_alloc_page_tables(void);
> +extern pgd_t *efi_get_pgd(void);
>  extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init old_map_region(efi_memory_desc_t *md);
> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
> index 2846aaab5103..ff57d3840842 100644
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
>  obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>  obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
>  obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
> +obj-$(CONFIG_EFI_INTERRUPTIBLE)	+= efi_interruptible.o
> diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
> index 58d669bc8250..f2403e16a8bb 100644
> --- a/arch/x86/platform/efi/efi_32.c
> +++ b/arch/x86/platform/efi/efi_32.c
> @@ -61,6 +61,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
>  void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
>  void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return initial_page_table;
> +}
> +
>  pgd_t * __init efi_call_phys_prolog(void)
>  {
>  	struct desc_ptr gdt_descr;

Shouldn't be necessary. You can't build this for 32-bit because of the
Kconfig magic.

> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index b492521503fe..7886c7527cdf 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -212,6 +212,11 @@ void efi_sync_low_kernel_mappings(void)
>  	memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
>  }
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return __va(efi_scratch.efi_pgt);
> +}
> +

You could make this easier and just return 'efi_pgd', since we now
have entirely separate EFI page tables.

> +static struct efivars interruptible_efivars;
> +
> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	in_panic = true;
> +	return NOTIFY_DONE;
> +}

Please make the panic notifier changes a separate patch.

> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +
> +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);

Did you have any luck binding this driver to the other
Broxton-specific drivers?

Additionally, you're going to want to make sure that
efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
otherwise you can't use the new EFI page table scheme, e.g. if someone
boots with efi=old_map.

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-02-11 15:19 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> Did you have any luck binding this driver to the other
> Broxton-specific drivers?
>
> Additionally, you're going to want to make sure that
> efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
> otherwise you can't use the new EFI page table scheme, e.g. if someone
> boots with efi=old_map.

I didn't have very much time to work on this but yes, we should be
able to make efi_interruptible a platform driver which would be probed
by SPD driver or something like that.
I'll upload it as soon as I've something

-- 
Sylvain