Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Sat, Mar 10, 2018 at 12:33:35AM +, Prakhya, Sai Praneeth wrote:
> Although the discussions were off my understanding, the present issue
> I see is, (and also the motivation for me to do the patch is) when a
> thread tries to execute any efi_runtime_service() we switch to efi_pgd
> (which doesn't have user space mappings) and all other subsystems in
> kernel aren't aware of this switch. This looks like a perfect case for
> kthread.

That's all fine and good but you need to be prepared and handle properly
an NMI while EFI is running.

I have no clue whether the platform delays delivery of NMIs during EFI
runtime services or whatever happens but you need to have all those
cases covered so that no monkey business happens.

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
> > We might have issues only when, we are already in efi_pgd, NMI comes
> > along
> 
> Can you trigger this? Or is it something hypothetical?
> 

AFAIK, it's hypothetical. I did try to trigger the issue, but failed [1].
Maybe, I need to have some more constraints [2].

[1] https://lkml.org/lkml/2017/8/23/715
[2] https://lkml.org/lkml/2017/8/25/469

> > and NMI handler tries to touch the regions that are not mapped in
> > efi_pgd
> 
> If it is not hypothetical, the NMI handler should learn to look at CR3 first 
> and
> return if CR3 has the efi pgd.

This solution and it's variants were discussed here [1], [2] and for varied 
reasons 
the community had decided to go with "Everything EFI as kthread" approach [3] 
[4].

Although the discussions were off my understanding, the present issue I see is, 
(and also the motivation for me to do the patch is)
when a thread tries to execute any  efi_runtime_service() we switch to efi_pgd 
(which doesn't have user space mappings) and all other subsystems in kernel 
aren't aware of this switch. This looks like a perfect case for kthread.

Kthread by definition doesn’t have user space mappings and if we run 
efi_runtime_services()
in a kthread context and if any other subsystem tries to access user space 
mappings 
while in efi_kthread, it's terminally broken [5].

There were several issues Andy, Peter and Mark raised.
One such (hypothetical) case is accessing user space from the back of an 
interrupt (NMI).
Others include
1. Issue specific to ARM because it runs efi_runtime_services() with interrupts 
enabled [6]
2. Interrupt taken while mmap_sem() is held for write that tries to access user 
memory [7]
3. If EFI were to have IO memory mapped at a "user" address, perf could end up 
reading it [8]

[1] https://lkml.org/lkml/2017/8/15/757
[2] https://lkml.org/lkml/2017/8/16/487
[3] https://lkml.org/lkml/2017/8/21/573
[4] https://lkml.org/lkml/2017/8/16/540

[5] https://lkml.org/lkml/2017/8/17/667
[6] https://lkml.org/lkml/2017/8/16/176
[7] https://lkml.org/lkml/2017/8/17/667
[8] https://lkml.org/lkml/2017/8/21/427

Regards,
Sai

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Fri, Mar 09, 2018 at 02:37:59AM +, Prakhya, Sai Praneeth wrote:
> But, I guess, we have some downsides with this design:
> 1. We are doing this to have "no exceptions to use efi_rts_wq", but we will 
> be making
> the common case complicated. i.e. When a user requests to write some efi 
> variable,

So if the pstore use case is so important and special, I think we should
make the EFI path as fast as possible as getting that data to the pstore
is a priority.

> Alternatively, instead of playing around with in_atomic(), we could have 
> wrapper
> functions like efi_write_var_non_wq() which will only be used by pstore. This 
> function
> will not use efi_rts_wq and directly invoke efi_runtime_service. Just an 
> attempt to
> make the code not look messy.

I guess.

If the write-to-pstore case is such a critical one, I guess the
exception is justified.

> That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
> We might have issues only when, we are already in efi_pgd, NMI comes along

Can you trigger this? Or is it something hypothetical?

> and NMI handler tries to touch the regions that are not mapped in efi_pgd

If it is not hypothetical, the NMI handler should learn to look at CR3
first and return if CR3 has the efi pgd.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Thu, Mar 08, 2018 at 05:05:44PM +, Luck, Tony wrote:
> > "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" -
> > that doesn't sound like optimal design to me. I would try to shove them
> > all through the workqueue - not have exceptions.
> 
> But pstore is trying to save the last gasp dying words from a kernel that
> has paniced. There isn't any guarantee that work queue will run. I think
> it is reasonable to have some special case to make sure that we do save
> the information.  But perhaps that special case should be to have pstore
> call directly into the guts of the EFI code and not worry about all these
> fancy switches of "mm" etc.

I guess...

> This is true for the machine check logging case too, but the mitigation is
> that the details of the error persist in the machine check banks across the
> reset ... so all is not lost if the work queue isn't run here.

Well, I'm still hoping to have this wonderful and reliable
logging facility one day where we can dump bytes into a
persistent-across-reboots buffer and analyze them later. And yap, in
that case, I don't mind if the code simply bypasses all the dancing in
the OS and writes those bytes. Even switching pagetables would be too
much for that case - just fire'n'forget writing from ring 0.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > Another warning by checkpatch is "use of in_atomic() in drivers code"
> 
> I'm assuming it warns because you're touching files in drivers/ but the efi 
> fun is
> not really a driver...

True! That makes sense :)

> 
> But looking at patch 3, that thing looks like a real mess. Some of the things 
> -
> pstore, it seems - do stuff in atomic context and yet you want to do efi 
> stuff in a
> workqueue which doesn't stomach atomic context to begin with.
> 
> So if you wanna do workqueue, you should make sure all efi stuff gets delayed
> to process context and queued properly. For example, we log MCEs from atomic
> context by putting them on a lockless buffer and then kicking irq_work to 
> queue
> the work when we return to process context.
> Can you do something like that?
> 

I think we can do this, it's is a good idea. I looked at this approach and saw 
that
in oops_end() function, part of arch/x86/kernel/dumpstack, between oops_exit()
and panic() (here we are not in atomic context, so, I think we can use work 
queues)
we could have something like efi_flush_buffer() which will flush the buffer and
queue the work to efi_rts_wq.

But, I guess, we have some downsides with this design:
1. We are doing this to have "no exceptions to use efi_rts_wq", but we will be 
making
the common case complicated. i.e. When a user requests to write some efi 
variable,
we will first write it to a buffer and then flush the buffer using efi_rts_wq. 
Instead, we
could have written the variable directly.
Maybe, you meant, we should use this buffer only while pstore and not during 
normal
case (which sounds reasonably OK).
2. It doesn't look rational that, when we are already going down, we schedule 
(because
we will be invoking efi_runtime_services() through work queue) to log some 
stuff.
Not, sure if that's happening in other parts of kernel or if it's OK to do that.

I will try the suggested approach and will keep this thread posted.

> "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" - that
> doesn't sound like optimal design to me. I would try to shove them all through
> the workqueue - not have exceptions.
> 

Alternatively, instead of playing around with in_atomic(), we could have wrapper
functions like efi_write_var_non_wq() which will only be used by pstore. This 
function
will not use efi_rts_wq and directly invoke efi_runtime_service. Just an 
attempt to
make the code not look messy.

> Then this:
> 
> > A potential issue could be, for instance, an NMI interrupt (like perf)
> > trying to profile some user data while in efi_pgd.
> 
> I can't understand.
> 
> How did we handle this until now and why is it a problem all of a sudden?
> 
> Because I don't recall being unable to run perf while efi runtime services are
> happening.
> 

That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
We might have issues only when, we are already in efi_pgd, NMI comes along
and NMI handler tries to touch the regions that are not mapped in efi_pgd
(Eg: User space part of process address space) and using kthread inherently
means that we will not have any user space.

Regards,
Sai

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

/*
> >> > +* Since we process only one efi_runtime_service() at a time, an
> >> > +* ordered workqueue (which creates only one execution context)
> >> > +* should suffice all our needs.
> >> > +*/
> >> > +   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue",
> >> > + 0);
> >>
> >> efi_rts_wq or efi_rts_workqueue?
> >>
> >> > +   if (!efi_rts_wq) {
> >> > +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> >> > services "
> >>
> >> Same here.
> >
> > Sure! I will make it consistent with "efi_rts_wq". Just tried to be
> > more verbose with names :)
> >
> 
> It is not a big deal, but using the exact same name is better for the 
> purposes of
> grepping and things like that :-)

Yes, that makes sense.

> By the way, check the commit title/message, there are some others there too.

Sure! I will make changes to commit/title messages too.

Regards,
Sai

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Luck, Tony]

> "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" -
> that doesn't sound like optimal design to me. I would try to shove them
> all through the workqueue - not have exceptions.

But pstore is trying to save the last gasp dying words from a kernel that
has paniced. There isn't any guarantee that work queue will run. I think
it is reasonable to have some special case to make sure that we do save
the information.  But perhaps that special case should be to have pstore
call directly into the guts of the EFI code and not worry about all these
fancy switches of "mm" etc.

This is true for the machine check logging case too, but the mitigation is
that the details of the error persist in the machine check banks across the
reset ... so all is not lost if the work queue isn't run here.

-Tony

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Thu, Mar 08, 2018 at 05:31:03AM +, Prakhya, Sai Praneeth wrote:
> Another warning by checkpatch is "use of in_atomic() in drivers code"

I'm assuming it warns because you're touching files in drivers/ but the
efi fun is not really a driver...

But looking at patch 3, that thing looks like a real mess. Some of the
things - pstore, it seems - do stuff in atomic context and yet you want
to do efi stuff in a workqueue which doesn't stomach atomic context to
begin with.

So if you wanna do workqueue, you should make sure all efi stuff gets
delayed to process context and queued properly. For example, we log
MCEs from atomic context by putting them on a lockless buffer and then
kicking irq_work to queue the work when we return to process context.
Can you do something like that?

"Hence, pstore calls efi_runtime_services() without using efi_rts_wq" -
that doesn't sound like optimal design to me. I would try to shove them
all through the workqueue - not have exceptions.

Then this:

> A potential issue could be, for instance, an NMI interrupt (like perf)
> trying to profile some user data while in efi_pgd.

I can't understand.

How did we handle this until now and why is it a problem all of a
sudden?

Because I don't recall being unable to run perf while efi runtime
services are happening.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Miguel Ojeda]

On Thu, Mar 8, 2018 at 5:22 AM, Prakhya, Sai Praneeth
 wrote:
>> > +struct workqueue_struct *efi_rts_wq;
>> > +
>> >  static bool disable_runtime;
>> >  static int __init setup_noefi(char *arg)  { @@ -329,6 +331,19 @@
>> > static int __init efisubsys_init(void)
>> > return 0;
>> >
>> > /*
>> > +* Since we process only one efi_runtime_service() at a time, an
>> > +* ordered workqueue (which creates only one execution context)
>> > +* should suffice all our needs.
>> > +*/
>> > +   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
>>
>> efi_rts_wq or efi_rts_workqueue?
>>
>> > +   if (!efi_rts_wq) {
>> > +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
>> > services "
>>
>> Same here.
>
> Sure! I will make it consistent with "efi_rts_wq". Just tried to be more 
> verbose
> with names :)
>

It is not a big deal, but using the exact same name is better for the
purposes of grepping and things like that :-) By the way, check the
commit title/message, there are some others there too.

> [...]
>
>> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)   
>> >  \
>> > +({ \
>> > +   struct efi_runtime_work efi_rts_work;   \
>> > +   \
>> > +   INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
>> > +   efi_rts_work.func = _rts;   \
>> > +   efi_rts_work.arg1 = _arg1;  \
>> > +   efi_rts_work.arg2 = _arg2;  \
>> > +   efi_rts_work.arg3 = _arg3;  \
>> > +   efi_rts_work.arg4 = _arg4;  \
>> > +   efi_rts_work.arg5 = _arg5;  \
>> > +   /*  \
>> > +* queue_work() returns 0 if work was already on queue, \
>> > +* _ideally_ this should never happen.  \
>> > +*/ \
>> > +   if (queue_work(efi_rts_wq, _rts_work.work)) \
>> > +   flush_work(_rts_work.work); \
>> > +   else\
>> > +   BUG();  \
>>
>> Thanks for the change! One remark, I would just do:
>
> Sorry! but I am planning to remove BUG(). Looks like it could defeat the 
> purpose
> of patch. Please see Boris comments on the other thread.

No problem. Let's see how it looks in v3 :-)

>
> [...]
>
>> > +/*
>> > + * efi_runtime_work:   Details of EFI Runtime Service work
>> > + * @func:  EFI Runtime Service function identifier
>> > + * @arg<1-5>:  EFI Runtime Service function arguments
>> > + * @status:Status of executing EFI Runtime Service
>> > + */
>> > +struct efi_runtime_work {
>> > +   u8 func;
>> > +   void *arg1;
>> > +   void *arg2;
>> > +   void *arg3;
>> > +   void *arg4;
>> > +   void *arg5;
>> > +   efi_status_t status;
>> > +   struct work_struct work;
>> > +};
>>
>> Why is efi_runtime_work in the .h at all?
>>
>
> Thanks for the catch. I will move it to runtime-wrappers.c file and will make 
> it
> static too. It isn't being used in any other place.
>
>> Please CC me for the next version! :-)
>
> Sure! Sorry for that. I should have done in V2.

Thanks!

Cheers,
Miguel

>
> Regards,
> Sai
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

+Cc Miguel Ojeda

> > > +({   
> > > \
> > > + struct efi_runtime_work efi_rts_work;   \
> > > + \
> > > + INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
> > > + efi_rts_work.func = _rts;   \
> > > + efi_rts_work.arg1 = _arg1;  \
> > > + efi_rts_work.arg2 = _arg2;  \
> > > + efi_rts_work.arg3 = _arg3;  \
> > > + efi_rts_work.arg4 = _arg4;  \
> > > + efi_rts_work.arg5 = _arg5;  \
> > > + /*  \
> > > +  * queue_work() returns 0 if work was already on queue, \
> > > +  * _ideally_ this should never happen.  \
> > > +  */ \
> > > + if (queue_work(efi_rts_wq, _rts_work.work))
> > \
> > > + flush_work(_rts_work.work);
> > \
> > > + else\
> > > + BUG();  \
> >
> > So failure to queue that work is such a critical problem that we need
> > to BUG() and can't possibly continue and shoult not attempt recovery at all?
> >
> 
> I think it's not critical, we can just return error status.
> I think the problem in itself is not at all critical but when I initially 
> thought about
> why the problem could have occurred, it sounded like one i.e. ideally (if the
> system is running fine) we should always be able to queue work. Failure to 
> queue
> means that the previous work is already on queue and that shouldn't be the
> case.
> So, thought, maybe something bad had happened already (just doubtful).
> 
> But, I see your point. BUG() sounds more like an over kill. Instead of fixing 
> an
> existing problem, this patch could completely take down the system.
> 
> > IOW, we should always strive to fail gracefully and not shit in pants
> > at the first sign of trouble.
> >
> 
> Yes, that makes sense. I will remove BUG() in V3 (in the two places that I
> introduced).
> 
> > Even checkpatch warns here:
> >
> > WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code
> > rather than BUG() or BUG_ON()
> > #184: FILE: drivers/firmware/efi/runtime-wrappers.c:92:
> > +   BUG();  \
> >
> 
> Sure! I will fix this
> 
> >
> > and by looking at the other output, you should run your patches
> > through checkpatch. Some of the things make sense like:
> >
> > WARNING: quoted string split across lines
> > #97: FILE: drivers/firmware/efi/efi.c:341:
> > +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> > services "
> > +  "disabled.\n");
> >
> > for example.
> >
> 
> I will fix this one too.
> 
> Another warning by checkpatch is "use of in_atomic() in drivers code"
> Do you think it's OK to check if were are "in_atomic()" in drivers code.
> I wasn't able to decide on other alternative, if possible, could you please 
> suggest
> one?
> 
> Regards,
> Sai
N�r��yb�X��ǧv�^�)޺{.n�+{�y^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > +({ \
> > +   struct efi_runtime_work efi_rts_work;   \
> > +   \
> > +   INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
> > +   efi_rts_work.func = _rts;   \
> > +   efi_rts_work.arg1 = _arg1;  \
> > +   efi_rts_work.arg2 = _arg2;  \
> > +   efi_rts_work.arg3 = _arg3;  \
> > +   efi_rts_work.arg4 = _arg4;  \
> > +   efi_rts_work.arg5 = _arg5;  \
> > +   /*  \
> > +* queue_work() returns 0 if work was already on queue, \
> > +* _ideally_ this should never happen.  \
> > +*/ \
> > +   if (queue_work(efi_rts_wq, _rts_work.work))
>   \
> > +   flush_work(_rts_work.work);
>   \
> > +   else\
> > +   BUG();  \
> 
> So failure to queue that work is such a critical problem that we need to BUG()
> and can't possibly continue and shoult not attempt recovery at all?
> 

I think it's not critical, we can just return error status.
I think the problem in itself is not at all critical but when I initially 
thought about
why the problem could have occurred, it sounded like one i.e. ideally (if the 
system
is running fine) we should always be able to queue work. Failure to queue means
that the previous work is already on queue and that shouldn't be the case.
So, thought, maybe something bad had happened already (just doubtful).
 
But, I see your point. BUG() sounds more like an over kill. Instead of fixing 
an existing
problem, this patch could completely take down the system.

> IOW, we should always strive to fail gracefully and not shit in pants at the 
> first
> sign of trouble.
>

Yes, that makes sense. I will remove BUG() in V3 (in the two places that I 
introduced).

> Even checkpatch warns here:
> 
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code
> rather than BUG() or BUG_ON()
> #184: FILE: drivers/firmware/efi/runtime-wrappers.c:92:
> +   BUG();  \
>

Sure! I will fix this
 
> 
> and by looking at the other output, you should run your patches through
> checkpatch. Some of the things make sense like:
> 
> WARNING: quoted string split across lines
> #97: FILE: drivers/firmware/efi/efi.c:341:
> +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> services "
> +  "disabled.\n");
> 
> for example.
> 

I will fix this one too.

Another warning by checkpatch is "use of in_atomic() in drivers code"
Do you think it's OK to check if were are "in_atomic()" in drivers code.
I wasn't able to decide on other alternative, if possible, could you please 
suggest one?

Regards,
Sai
N�r��yb�X��ǧv�^�)޺{.n�+{�y^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

> > +struct workqueue_struct *efi_rts_wq;
> > +
> >  static bool disable_runtime;
> >  static int __init setup_noefi(char *arg)  { @@ -329,6 +331,19 @@
> > static int __init efisubsys_init(void)
> > return 0;
> >
> > /*
> > +* Since we process only one efi_runtime_service() at a time, an
> > +* ordered workqueue (which creates only one execution context)
> > +* should suffice all our needs.
> > +*/
> > +   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
> 
> efi_rts_wq or efi_rts_workqueue?
> 
> > +   if (!efi_rts_wq) {
> > +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> > services "
> 
> Same here.

Sure! I will make it consistent with "efi_rts_wq". Just tried to be more verbose
with names :)

[...]

> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)
> > \
> > +({ \
> > +   struct efi_runtime_work efi_rts_work;   \
> > +   \
> > +   INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
> > +   efi_rts_work.func = _rts;   \
> > +   efi_rts_work.arg1 = _arg1;  \
> > +   efi_rts_work.arg2 = _arg2;  \
> > +   efi_rts_work.arg3 = _arg3;  \
> > +   efi_rts_work.arg4 = _arg4;  \
> > +   efi_rts_work.arg5 = _arg5;  \
> > +   /*  \
> > +* queue_work() returns 0 if work was already on queue, \
> > +* _ideally_ this should never happen.  \
> > +*/ \
> > +   if (queue_work(efi_rts_wq, _rts_work.work)) \
> > +   flush_work(_rts_work.work); \
> > +   else\
> > +   BUG();  \
> 
> Thanks for the change! One remark, I would just do:

Sorry! but I am planning to remove BUG(). Looks like it could defeat the purpose
of patch. Please see Boris comments on the other thread.

[...]

> > +/*
> > + * efi_runtime_work:   Details of EFI Runtime Service work
> > + * @func:  EFI Runtime Service function identifier
> > + * @arg<1-5>:  EFI Runtime Service function arguments
> > + * @status:Status of executing EFI Runtime Service
> > + */
> > +struct efi_runtime_work {
> > +   u8 func;
> > +   void *arg1;
> > +   void *arg2;
> > +   void *arg3;
> > +   void *arg4;
> > +   void *arg5;
> > +   efi_status_t status;
> > +   struct work_struct work;
> > +};
> 
> Why is efi_runtime_work in the .h at all?
> 

Thanks for the catch. I will move it to runtime-wrappers.c file and will make it
static too. It isn't being used in any other place.

> Please CC me for the next version! :-)

Sure! Sorry for that. I should have done in V2.

Regards,
Sai

RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Prakhya, Sai Praneeth]

-0800, Sai Praneeth Prakhya wrote:
> > @@ -329,6 +331,19 @@ static int __init efisubsys_init(void)
> > return 0;
> >
> > /*
> > +* Since we process only one efi_runtime_service() at a time, an
> > +* ordered workqueue (which creates only one execution context)
> > +* should suffice all our needs.
> > +*/
> > +   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
> > +   if (!efi_rts_wq) {
> > +   pr_err("Failed to create efi_rts_workqueue, EFI runtime services
> "
> > +  "disabled.\n");
> > +   clear_bit(EFI_RUNTIME_SERVICES, );
> > +   return 0;
> > +   }
> 
> I'm a little worried that something might sample this flag between it being 
> set in
> an early_initcall (arm_enable_runtime_services), and cleared in a 
> subsys_initcall
> here.
> 
> However, nothing seems to do that so far, so maybe that's ok...
> 

Thanks for raising this. I will take a look at initcalls.

> [...]
> 
> > +/* efi_runtime_service() function identifiers */ enum {
> > +   GET_TIME,
> > +   SET_TIME,
> > +   GET_WAKEUP_TIME,
> > +   SET_WAKEUP_TIME,
> > +   GET_VARIABLE,
> > +   GET_NEXT_VARIABLE,
> > +   SET_VARIABLE,
> > +   SET_VARIABLE_NONBLOCKING,
> > +   QUERY_VARIABLE_INFO,
> > +   QUERY_VARIABLE_INFO_NONBLOCKING,
> > +   GET_NEXT_HIGH_MONO_COUNT,
> > +   RESET_SYSTEM,
> > +   UPDATE_CAPSULE,
> > +   QUERY_CAPSULE_CAPS,
> > +};
> 
> Can we please give this enum a name

Sure! Added in V3.

> 
> [...]
> 
> > +/*
> > + * efi_runtime_work:   Details of EFI Runtime Service work
> > + * @func:  EFI Runtime Service function identifier
> > + * @arg<1-5>:  EFI Runtime Service function arguments
> > + * @status:Status of executing EFI Runtime Service
> > + */
> > +struct efi_runtime_work {
> > +   u8 func;
> 
> ... and use it here rather than an opaque u8? I realise that means placing the
> enum in .
> 

Actually, with Miguel comments, I am considering making this struct static and 
moving
it to runtime-wrappers.c, since "struct efi_runtime_work" isn't really being 
used anywhere
except runtime-wrappers.c. Please see in V3.

> > +   void *arg1;
> > +   void *arg2;
> > +   void *arg3;
> > +   void *arg4;
> > +   void *arg5;
> > +   efi_status_t status;
> > +   struct work_struct work;
> > +};
> 
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Borislav Petkov]

On Mon, Mar 05, 2018 at 03:23:09PM -0800, Sai Praneeth Prakhya wrote:
> +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)  
> \
> +({   \
> + struct efi_runtime_work efi_rts_work;   \
> + \
> + INIT_WORK_ONSTACK(_rts_work.work, efi_call_rts);\
> + efi_rts_work.func = _rts;   \
> + efi_rts_work.arg1 = _arg1;  \
> + efi_rts_work.arg2 = _arg2;  \
> + efi_rts_work.arg3 = _arg3;  \
> + efi_rts_work.arg4 = _arg4;  \
> + efi_rts_work.arg5 = _arg5;  \
> + /*  \
> +  * queue_work() returns 0 if work was already on queue, \
> +  * _ideally_ this should never happen.  \
> +  */ \
> + if (queue_work(efi_rts_wq, _rts_work.work)) \
> + flush_work(_rts_work.work); \
> + else\
> + BUG();  \

So failure to queue that work is such a critical problem that we need
to BUG() and can't possibly continue and shoult not attempt recovery at
all?

IOW, we should always strive to fail gracefully and not shit in pants at
the first sign of trouble.

Even checkpatch warns here:

WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
than BUG() or BUG_ON()
#184: FILE: drivers/firmware/efi/runtime-wrappers.c:92:
+   BUG();  \


and by looking at the other output, you should run your patches through
checkpatch. Some of the things make sense like:

WARNING: quoted string split across lines
#97: FILE: drivers/firmware/efi/efi.c:341:
+   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
services "
+  "disabled.\n");

for example.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Miguel Ojeda]

On Tue, Mar 6, 2018 at 12:23 AM, Sai Praneeth Prakhya
 wrote:
> From: Sai Praneeth 
>
> When a process requests the kernel to execute any efi_runtime_service(),
> the requested efi_runtime_service (represented as an identifier) and its
> arguments are packed into a struct named efi_runtime_work and queued
> onto work queue named efi_rts_wq. The caller then waits until the work
> is completed.
>
> Introduce some infrastructure:
> 1. Creating workqueue named efi_rts_wq
> 2. A macro (efi_queue_work()) that
> a. populates efi_runtime_work
> b. queues work onto efi_rts_wq and
> c. waits until worker thread returns
>
> The caller thread has to wait until the worker thread returns, because
> it's dependent on the return status of efi_runtime_service() and, in
> specific cases, the arguments populated by efi_runtime_service(). Some
> efi_runtime_services() takes a pointer to buffer as an argument and
> fills up the buffer with requested data. For instance,
> efi_get_variable() and efi_get_next_variable(). Hence, caller process
> cannot just post the work and get going.
>
> Some facts about efi_runtime_services():
> 1. A quick look at all the efi_runtime_services() shows that any
> efi_runtime_service() has five or less arguments.
> 2. An argument of efi_runtime_service() can be a value (of any type)
> or a pointer (of any type).
> Hence, efi_runtime_work has five void pointers to store these arguments.
>
> Signed-off-by: Sai Praneeth Prakhya 
> Suggested-by: Andy Lutomirski 
> Cc: Lee, Chun-Yi 
> Cc: Borislav Petkov 
> Cc: Tony Luck 
> Cc: Will Deacon 
> Cc: Dave Hansen 
> Cc: Mark Rutland 
> Cc: Bhupesh Sharma 
> Cc: Ricardo Neri 
> Cc: Ravi Shankar 
> Cc: Matt Fleming 
> Cc: Peter Zijlstra 
> Cc: Ard Biesheuvel 
> Cc: Dan Williams 
> ---
>  drivers/firmware/efi/efi.c  | 15 
>  drivers/firmware/efi/runtime-wrappers.c | 61 
> +
>  include/linux/efi.h | 20 +++
>  3 files changed, 96 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 838b8efe639c..04b46c62f3ce 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -75,6 +75,8 @@ static unsigned long *efi_tables[] = {
> _attr_table,
>  };
>
> +struct workqueue_struct *efi_rts_wq;
> +
>  static bool disable_runtime;
>  static int __init setup_noefi(char *arg)
>  {
> @@ -329,6 +331,19 @@ static int __init efisubsys_init(void)
> return 0;
>
> /*
> +* Since we process only one efi_runtime_service() at a time, an
> +* ordered workqueue (which creates only one execution context)
> +* should suffice all our needs.
> +*/
> +   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);

efi_rts_wq or efi_rts_workqueue?

> +   if (!efi_rts_wq) {
> +   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> services "

Same here.

> +  "disabled.\n");
> +   clear_bit(EFI_RUNTIME_SERVICES, );
> +   return 0;
> +   }
> +
> +   /*
>  * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
>  * it should be invoked only after efi_rts_workqueue is ready.
>  */
> diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> b/drivers/firmware/efi/runtime-wrappers.c
> index ae54870b2788..649763171439 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -1,6 +1,14 @@
>  /*
>   * runtime-wrappers.c - Runtime Services function call wrappers
>   *
> + * Implementation summary:
> + * ---
> + * 1. When user/kernel thread requests to execute efi_runtime_service(),
> + * enqueue work to efi_rts_workqueue.
> + * 2. Caller thread waits until the work is finished because it's
> + * dependent on the return status and execution of efi_runtime_service().
> + * For instance, get_variable() and get_next_variable().
> + *
>   * Copyright (C) 2014 Linaro Ltd. 
>   *
>   * Split off from arch/x86/platform/efi/efi.c
> @@ -22,6 +30,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
>  #include 
>
>  /*
> @@ -33,6 +43,57 @@
>  #define __efi_call_virt(f, args...) \
> __efi_call_virt_pointer(efi.systab->runtime, f, args)
>
> +/* efi_runtime_service() function identifiers */
> +enum {
> +   GET_TIME,
> +   SET_TIME,
> +   GET_WAKEUP_TIME,
> +   SET_WAKEUP_TIME,
> +   GET_VARIABLE,
> +   GET_NEXT_VARIABLE,
> +   SET_VARIABLE,

Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Mark Rutland]

On Mon, Mar 05, 2018 at 03:23:09PM -0800, Sai Praneeth Prakhya wrote:
> @@ -329,6 +331,19 @@ static int __init efisubsys_init(void)
>   return 0;
>  
>   /*
> +  * Since we process only one efi_runtime_service() at a time, an
> +  * ordered workqueue (which creates only one execution context)
> +  * should suffice all our needs.
> +  */
> + efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
> + if (!efi_rts_wq) {
> + pr_err("Failed to create efi_rts_workqueue, EFI runtime 
> services "
> +"disabled.\n");
> + clear_bit(EFI_RUNTIME_SERVICES, );
> + return 0;
> + }

I'm a little worried that something might sample this flag between it
being set in an early_initcall (arm_enable_runtime_services), and
cleared in a subsys_initcall here.

However, nothing seems to do that so far, so maybe that's ok...

[...]

> +/* efi_runtime_service() function identifiers */
> +enum {
> + GET_TIME,
> + SET_TIME,
> + GET_WAKEUP_TIME,
> + SET_WAKEUP_TIME,
> + GET_VARIABLE,
> + GET_NEXT_VARIABLE,
> + SET_VARIABLE,
> + SET_VARIABLE_NONBLOCKING,
> + QUERY_VARIABLE_INFO,
> + QUERY_VARIABLE_INFO_NONBLOCKING,
> + GET_NEXT_HIGH_MONO_COUNT,
> + RESET_SYSTEM,
> + UPDATE_CAPSULE,
> + QUERY_CAPSULE_CAPS,
> +};

Can we please give this enum a name

[...]

> +/*
> + * efi_runtime_work: Details of EFI Runtime Service work
> + * @func:EFI Runtime Service function identifier
> + * @arg<1-5>:EFI Runtime Service function arguments
> + * @status:  Status of executing EFI Runtime Service
> + */
> +struct efi_runtime_work {
> + u8 func;

... and use it here rather than an opaque u8? I realise that means
placing the enum in .

> + void *arg1;
> + void *arg2;
> + void *arg3;
> + void *arg4;
> + void *arg5;
> + efi_status_t status;
> + struct work_struct work;
> +};

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

[Sai Praneeth Prakhya]

From: Sai Praneeth 

When a process requests the kernel to execute any efi_runtime_service(),
the requested efi_runtime_service (represented as an identifier) and its
arguments are packed into a struct named efi_runtime_work and queued
onto work queue named efi_rts_wq. The caller then waits until the work
is completed.

Introduce some infrastructure:
1. Creating workqueue named efi_rts_wq
2. A macro (efi_queue_work()) that
a. populates efi_runtime_work
b. queues work onto efi_rts_wq and
c. waits until worker thread returns

The caller thread has to wait until the worker thread returns, because
it's dependent on the return status of efi_runtime_service() and, in
specific cases, the arguments populated by efi_runtime_service(). Some
efi_runtime_services() takes a pointer to buffer as an argument and
fills up the buffer with requested data. For instance,
efi_get_variable() and efi_get_next_variable(). Hence, caller process
cannot just post the work and get going.

Some facts about efi_runtime_services():
1. A quick look at all the efi_runtime_services() shows that any
efi_runtime_service() has five or less arguments.
2. An argument of efi_runtime_service() can be a value (of any type)
or a pointer (of any type).
Hence, efi_runtime_work has five void pointers to store these arguments.

Signed-off-by: Sai Praneeth Prakhya 
Suggested-by: Andy Lutomirski 
Cc: Lee, Chun-Yi 
Cc: Borislav Petkov 
Cc: Tony Luck 
Cc: Will Deacon 
Cc: Dave Hansen 
Cc: Mark Rutland 
Cc: Bhupesh Sharma 
Cc: Ricardo Neri 
Cc: Ravi Shankar 
Cc: Matt Fleming 
Cc: Peter Zijlstra 
Cc: Ard Biesheuvel 
Cc: Dan Williams 
---
 drivers/firmware/efi/efi.c  | 15 
 drivers/firmware/efi/runtime-wrappers.c | 61 +
 include/linux/efi.h | 20 +++
 3 files changed, 96 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 838b8efe639c..04b46c62f3ce 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -75,6 +75,8 @@ static unsigned long *efi_tables[] = {
_attr_table,
 };
 
+struct workqueue_struct *efi_rts_wq;
+
 static bool disable_runtime;
 static int __init setup_noefi(char *arg)
 {
@@ -329,6 +331,19 @@ static int __init efisubsys_init(void)
return 0;
 
/*
+* Since we process only one efi_runtime_service() at a time, an
+* ordered workqueue (which creates only one execution context)
+* should suffice all our needs.
+*/
+   efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
+   if (!efi_rts_wq) {
+   pr_err("Failed to create efi_rts_workqueue, EFI runtime 
services "
+  "disabled.\n");
+   clear_bit(EFI_RUNTIME_SERVICES, );
+   return 0;
+   }
+
+   /*
 * Clean DUMMY object calls EFI Runtime Service, set_variable(), so
 * it should be invoked only after efi_rts_workqueue is ready.
 */
diff --git a/drivers/firmware/efi/runtime-wrappers.c 
b/drivers/firmware/efi/runtime-wrappers.c
index ae54870b2788..649763171439 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -1,6 +1,14 @@
 /*
  * runtime-wrappers.c - Runtime Services function call wrappers
  *
+ * Implementation summary:
+ * ---
+ * 1. When user/kernel thread requests to execute efi_runtime_service(),
+ * enqueue work to efi_rts_workqueue.
+ * 2. Caller thread waits until the work is finished because it's
+ * dependent on the return status and execution of efi_runtime_service().
+ * For instance, get_variable() and get_next_variable().
+ *
  * Copyright (C) 2014 Linaro Ltd. 
  *
  * Split off from arch/x86/platform/efi/efi.c
@@ -22,6 +30,8 @@
 #include 
 #include 
 #include 
+#include 
+
 #include 
 
 /*
@@ -33,6 +43,57 @@
 #define __efi_call_virt(f, args...) \
__efi_call_virt_pointer(efi.systab->runtime, f, args)
 
+/* efi_runtime_service() function identifiers */
+enum {
+   GET_TIME,
+   SET_TIME,
+   GET_WAKEUP_TIME,
+   SET_WAKEUP_TIME,
+   GET_VARIABLE,
+   GET_NEXT_VARIABLE,
+   SET_VARIABLE,
+   SET_VARIABLE_NONBLOCKING,
+   QUERY_VARIABLE_INFO,
+   QUERY_VARIABLE_INFO_NONBLOCKING,
+   GET_NEXT_HIGH_MONO_COUNT,
+   RESET_SYSTEM,
+   UPDATE_CAPSULE,
+   QUERY_CAPSULE_CAPS,
+};
+
+/*
+ * efi_queue_work: Queue efi_runtime_service() and wait until it's done
+ * @rts:   efi_runtime_service() function identifier
+ * @rts_arg<1-5>: