Re: [PATCH V2 1/3] x86/efi: Call efi_delete_dummy_variable() during efi subsystem initialization

2018-03-07 Thread Ard Biesheuvel 
Hello Sai,

On 5 March 2018 at 23:23, Sai Praneeth Prakhya
 wrote:
> From: Sai Praneeth 
>
> Invoking efi_runtime_services() through efi_workqueue means all accesses
> to efi_runtime_services() should be done after efi_rts_wq has been
> created. efi_delete_dummy_variable() calls set_variable(), hence
> efi_delete_dummy_variable() should be called after efi_rts_wq has been
> created.
>
> efi_delete_dummy_variable() is called from efi_enter_virtual_mode()
> which is early in the boot phase (efi_rts_wq isn't created yet), so call
> efi_delete_dummy_variable() later in the boot phase i.e. while
> initializing efi subsystem. In the next patch, this is the place where
> we create efi_rts_wq and all the efi_runtime_services() will be called
> using efi_rts_wq.
>
> 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 
> ---
>  arch/x86/include/asm/efi.h  | 1 -
>  arch/x86/platform/efi/efi.c | 6 --
>  drivers/firmware/efi/efi.c  | 6 ++
>  include/linux/efi.h | 3 +++
>  4 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index a399c1ebf6f0..43009e3f821b 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -143,7 +143,6 @@ extern void __init efi_runtime_update_mappings(void);
>  extern void __init efi_dump_pagetable(void);
>  extern void __init efi_apply_memmap_quirks(void);
>  extern int __init efi_reuse_config(u64 tables, int nr_tables);
> -extern void efi_delete_dummy_variable(void);
>
>  struct efi_setup_data {
> u64 fw_vendor;
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 9061babfbc83..a3169d14583f 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -893,9 +893,6 @@ static void __init kexec_enter_virtual_mode(void)
>
> if (efi_enabled(EFI_OLD_MEMMAP) && (__supported_pte_mask & _PAGE_NX))
> runtime_code_page_mkexec();
> -
> -   /* clean DUMMY object */
> -   efi_delete_dummy_variable();
>  #endif
>  }
>
> @@ -1015,9 +1012,6 @@ static void __init __efi_enter_virtual_mode(void)
>  * necessary relocation fixups for the new virtual addresses.
>  */
> efi_runtime_update_mappings();
> -
> -   /* clean DUMMY object */
> -   efi_delete_dummy_variable();
>  }
>
>  void __init efi_enter_virtual_mode(void)
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index cd42f66a7c85..838b8efe639c 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -328,6 +328,12 @@ static int __init efisubsys_init(void)
> if (!efi_enabled(EFI_BOOT))
> return 0;
>
> +   /*
> +* Clean DUMMY object calls EFI Runtime Service, set_variable(), so
> +* it should be invoked only after efi_rts_workqueue is ready.
> +*/
> +   efi_delete_dummy_variable();
> +

Is there any way to keep this local to arch/x86?

> /* We register the efi directory at /sys/firmware/efi */
> efi_kobj = kobject_create_and_add("efi", firmware_kobj);
> if (!efi_kobj) {
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f5083aa72eae..c4efb3ef0dfa 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -992,6 +992,7 @@ extern efi_status_t efi_query_variable_store(u32 
> attributes,
>  unsigned long size,
>  bool nonblocking);
>  extern void efi_find_mirror(void);
> +extern void efi_delete_dummy_variable(void);
>  #else
>  static inline void efi_late_init(void) {}
>  static inline void efi_free_boot_services(void) {}
> @@ -1002,6 +1003,8 @@ static inline efi_status_t efi_query_variable_store(u32 
> attributes,
>  {
> return EFI_SUCCESS;
>  }
> +
> +static inline void efi_delete_dummy_variable(void) {}
>  #endif
>  extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
>
> --
> 2.7.4
>
--
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

Urgent response

2018-03-07 Thread James Williams 
Attn: Beneficiary,

We have contacted the Federal Ministry of Finance on your Behalf and
they have brought a solution to your problem by coordinating your
payment in total (10,000,000.00) Ten Million Dollars in an atm card
which you can use to withdraw money from any ATM MACHINE CENTER
anywhere in the world with a maximum of 1 Dollars daily. You now
have the lawful right to claim your fund in an atm card. Since the
Federal Bureau of Investigation is involved in this transaction, you
have to be rest assured for this is 100% risk free it is our duty to
protect the American Citizens, European Citizens, Asian Citizen. All I
want you to do is to contact the atm card CENTER Via email or call the
office telephone number one of the Consultant will assist you for
their requirements to proceed and procure your Approval Slip on your
behalf.

CONTACT INFORMATION
NAME: James  Williams
EMAIL: paymasterofficed...@gmail.com


Do contact us with your details:

Full name//
Address// Age//
 Telephone Numbers//
Occupation//
 Your Country//
Bank Details//

So your files would be updated after which the Delivery of your
atm card will be affected to your designated home Address without any
further delay and the bank will transfer your funds in total
(10,000,000.00) Ten Million Dollars to your Bank account. We
will reply you with the secret code (1600 atm card). We advice you get
back to the payment office after you have contacted the ATM SWIFT CARD
CENTER and we do await your response so we can move on with our
Investigation and make sure your ATM SWIFT CARD gets to you.


Best Regards
James Williams
Paymaster General
Federal Republic Of Nigeri
--
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()

2018-03-07 Thread 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()

2018-03-07 Thread 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 3/3] efi: Use efi_rts_workqueue to invoke EFI Runtime Services

2018-03-07 Thread Prakhya, Sai Praneeth 
> >> > pstore writes could potentially be invoked in interrupt context and
> >> > it uses set_variable<>() and query_variable_info<>() to store logs.
> >> > If we invoke efi_runtime_services() through efi_rts_wq while in
> >> > atomic() kernel issues a warning ("scheduling wile in atomic") and
> >> > prints stack trace. One way to overcome this is to not make the
> >> > caller process wait for the worker thread to finish. This approach
> >> > breaks pstore i.e. the log messages aren't written to efi
> >> > variables. Hence, pstore calls
> >> > efi_runtime_services() without using efi_rts_wq or in other words
> >> > efi_rts_wq will be used unconditionally for all the
> >> > efi_runtime_services() except set_variable<>() and
> >> > query_variable_info<>()
> >>
> >> Can't NMIs still come in during this?
> >>
> >> ... or do we assume that since something has already gone wrong, this
> >> doesn't matter?
> >>
> >
> > I think they can come. AFAIK, pstore (if enabled) runs only when we crashed.
> > Since, we are still executing stuff to log messages and NMI's can't be
> > masked, there is still a possibility for NMI's to occur (please correct me 
> > if I am
> wrong).
> > But, as you said earlier, I guess it doesn't matter because anyways we are
> going down.
> 
> The problem is that we are not always in a "already going down"
> condition for typical set_variable and query_variable_info calls.

That's correct.

> So are we actually fixing anything with this patchset?

When we are _not_ in interrupt context (eg: process context)
we still use efi_rts_wq to invoke *all* efi_runtime_services().
This solves *someone trying to access user space* while in EFI runtime services
mapping problem. A instance could be, some user space process requests us to
execute efi_runtime_service(), so, kernel switches to efi_pgd (which doesn’t 
have
user space part of process address space) and while executing 
efi_runtime_service()
perf NMI comes along to profile user data.

> In other words if the NMI vs EFI
> mapping problem requires the workqueue context then we can't have any EFI
> calls outside of that context.  Am I missing how this scheme addresses that
> problem?

I think so, because, we are not trying to solve NMI vs EFI Runtime Service 
mappings.
AFAIK, they both work together (for x86_64). The problem is, as stated above,
"someone trying to access user space while executing EFI runtime services".
The problem exists because EFI runtime services mappings don’t have user space
part of process address space.

I think the problem still persists when we are already in interrupt context and 
then
we were requested to execute some efi_runtime_service() and then NMI happens
and that NMI handler touches user space.

Please let me know if my explanation didn’t make sense or if I misunderstood
your question.

Ard and Matt, please correct me if I stated something that is incorrect.

Re: [PATCH V2 3/3] efi: Use efi_rts_workqueue to invoke EFI Runtime Services

2018-03-07 Thread Dan Williams 
On Wed, Mar 7, 2018 at 8:11 PM, Prakhya, Sai Praneeth
 wrote:
> 05, 2018 at 03:23:10PM -0800, Sai Praneeth Prakhya wrote:
>> > From: Sai Praneeth 
>> >
>> > Presently, efi_runtime_services() are executed by firmware in process
>> > context. To execute efi_runtime_service(), kernel switches the page
>> > directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't have
>> > any user space mappings. A potential issue could be, for instance, an
>> > NMI interrupt (like perf) trying to profile some user data while in 
>> > efi_pgd.
>>
>> It might be worth pointing out that this could result in disaster (e.g.
>> if the frame pointer happens to point at MMIO in the EFI runtime services
>> mappings).
>>
>
> Sorry! I didn't get it. I would like to add this point, so could you please
> explain it more?
>
>> [...]
>>
>> > pstore writes could potentially be invoked in interrupt context and it
>> > uses set_variable<>() and query_variable_info<>() to store logs. If we
>> > invoke efi_runtime_services() through efi_rts_wq while in atomic()
>> > kernel issues a warning ("scheduling wile in atomic") and prints stack
>> > trace. One way to overcome this is to not make the caller process wait
>> > for the worker thread to finish. This approach breaks pstore i.e. the
>> > log messages aren't written to efi variables. Hence, pstore calls
>> > efi_runtime_services() without using efi_rts_wq or in other words
>> > efi_rts_wq will be used unconditionally for all the
>> > efi_runtime_services() except set_variable<>() and
>> > query_variable_info<>()
>>
>> Can't NMIs still come in during this?
>>
>> ... or do we assume that since something has already gone wrong, this doesn't
>> matter?
>>
>
> I think they can come. AFAIK, pstore (if enabled) runs only when we crashed.
> Since, we are still executing stuff to log messages and NMI's can't be masked,
> there is still a possibility for NMI's to occur (please correct me if I am 
> wrong).
> But, as you said earlier, I guess it doesn't matter because anyways we are 
> going down.

The problem is that we are not always in a "already going down"
condition for typical set_variable and query_variable_info calls. So
are we actually fixing anything with this patchset? In other words if
the NMI vs EFI mapping problem requires the workqueue context then we
can't have any EFI calls outside of that context.  Am I missing how
this scheme addresses that problem?
--
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()

2018-03-07 Thread 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 3/3] efi: Use efi_rts_workqueue to invoke EFI Runtime Services

2018-03-07 Thread Prakhya, Sai Praneeth 
05, 2018 at 03:23:10PM -0800, Sai Praneeth Prakhya wrote:
> > From: Sai Praneeth 
> >
> > Presently, efi_runtime_services() are executed by firmware in process
> > context. To execute efi_runtime_service(), kernel switches the page
> > directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't have
> > any user space mappings. A potential issue could be, for instance, an
> > NMI interrupt (like perf) trying to profile some user data while in efi_pgd.
> 
> It might be worth pointing out that this could result in disaster (e.g.
> if the frame pointer happens to point at MMIO in the EFI runtime services
> mappings).
> 

Sorry! I didn't get it. I would like to add this point, so could you please
explain it more?

> [...]
> 
> > pstore writes could potentially be invoked in interrupt context and it
> > uses set_variable<>() and query_variable_info<>() to store logs. If we
> > invoke efi_runtime_services() through efi_rts_wq while in atomic()
> > kernel issues a warning ("scheduling wile in atomic") and prints stack
> > trace. One way to overcome this is to not make the caller process wait
> > for the worker thread to finish. This approach breaks pstore i.e. the
> > log messages aren't written to efi variables. Hence, pstore calls
> > efi_runtime_services() without using efi_rts_wq or in other words
> > efi_rts_wq will be used unconditionally for all the
> > efi_runtime_services() except set_variable<>() and
> > query_variable_info<>()
> 
> Can't NMIs still come in during this?
> 
> ... or do we assume that since something has already gone wrong, this doesn't
> matter?
>

I think they can come. AFAIK, pstore (if enabled) runs only when we crashed.
Since, we are still executing stuff to log messages and NMI's can't be masked,
there is still a possibility for NMI's to occur (please correct me if I am 
wrong).
But, as you said earlier, I guess it doesn't matter because anyways we are 
going down.
 
> Otherwise, this doesn't seem to break basic stuff on arm64 platforms. I can 
> boot
> up, read the EFI RTC, and reboot. I ahd lockdep and KASAN enabled, I received
> no splats from either.
> 
> FWIW:
> 
> Tested-by: Mark Rutland  
> 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()

2018-03-07 Thread 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 0/2] ESRT fixes for relocatable kexec'd kernel

2018-03-07 Thread Ard Biesheuvel 
(+ James)

Hello Akashi,

On 6 March 2018 at 09:00, AKASHI Takahiro  wrote:
> Tyler, Jeffrey,
>
> On Fri, Mar 02, 2018 at 08:27:11AM -0500, Tyler Baicar wrote:
>> On 3/2/2018 12:53 AM, AKASHI Takahiro wrote:
>> >Tyler, Jeffrey,
>> >
>> >[Note: This issue takes place in kexec, not kdump. So to be precise,
>> >it is not the same phenomenon as what I addressed in [1],[2]:
>> >   [1] 
>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-February/557254.html
>> >   [2] 
>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/553098.html
>> >]
>> >
>> >On Thu, Mar 01, 2018 at 12:56:38PM -0500, Tyler Baicar wrote:
>> >>Hello,
>> >>
>> >>On 2/28/2018 9:50 PM, AKASHI Takahiro wrote:
>> >>>Hi,
>> >>>
>> >>>On Wed, Feb 28, 2018 at 08:39:42AM -0700, Jeffrey Hugo wrote:
>> On 2/27/2018 11:19 PM, AKASHI Takahiro wrote:
>> >Tyler,
>> >
>> ># I missed catching your patch as its subject doesn't contain arm64.
>> >
>> >On Fri, Feb 23, 2018 at 12:42:31PM -0700, Tyler Baicar wrote:
>> >>Currently on arm64 ESRT memory does not appear to be properly blocked 
>> >>off.
>> >>Upon successful initialization, ESRT prints out the memory region that 
>> >>it
>> >>exists in like:
>> >>
>> >>esrt: Reserving ESRT space from 0x0a4c1c18 to 
>> >>0x0a4c1cf0.
>> >>
>> >>But then by dumping /proc/iomem this region appears as part of System 
>> >>RAM
>> >>rather than being reserved:
>> >>
>> >>08f1-0dee : System RAM
>> >>
>> >>This causes issues when trying to kexec if the kernel is relocatable. 
>> >>When
>> >>kexec tries to execute, this memory can be selected to relocate the 
>> >>kernel to
>> >>which then overwrites all the ESRT information. Then when the kexec'd 
>> >>kernel
>> >>tries to initialize ESRT, it doesn't recognize the ESRT version number 
>> >>and
>> >>just returns from efi_esrt_init().
>> >I'm not sure what is the root cause of your problem.
>> >Do you have good confidence that the kernel (2nd kernel image in this 
>> >case?)
>> >really overwrite ESRT region?
>> According to my debug, yes.
>> Using JTAG, I was able to determine that the ESRT memory region was 
>> getting
>> overwritten by the secondary kernel in
>> kernel/arch/arm64/kernel/relocate_kernel.S - specifically the "copy_page"
>> line of arm64_relocate_new_kernel()
>> 
>> >To my best knowledge, kexec is carefully designed not to do such a thing
>> >as it allocates a temporary buffer for kernel image and copies it to the
>> >final destination at the very end of the 1st kernel.
>> >
>> >My guess is that kexec, or rather kexec-tools, tries to load the kernel 
>> >image
>> >at 0x8f8 (or 0x908?, not sure) in your case. It may or may not 
>> >be
>> >overlapped with ESRT.
>> >(Try "-d" option when executing kexec command for confirmation.)
>> With -d, I see
>> 
>> get_memory_ranges_iomem_cb: 09611000 - 0e5f : System 
>> RAM
>> 
>> That overlaps the ESRT reservation -
>> [ 0.00] esrt: Reserving ESRT space from 0x0b708718 to
>> 0x0b7087f0
>> 
>> >Are you using initrd with kexec?
>> Yes
>> >>>To make the things clear, can you show me, if possible, the followings:
>> >>I have attached all of these:
>> >Many thanks.
>> >According to the data, ESRT was overwritten by initrd, not the kernel image.
>> >It doesn't matter to you though :)
>> >
>> >The solution would be, as Ard suggested, that more information be
>> >added to /proc/iomem.
>> >I'm going to fix the issue as quickly as possible.
>> Great, thank you!! Please add us to the fix and we will gladly test it out.
>
> I have created a workaround patch, attached below, as a kind of PoC.
> Can you give it a go, please?
> You need another patch for kexec-tools, too. See
> https:/git.linaro.org/people/takahiro.akashi/kexecl-tools.git arm64/resv_mem
>

Thanks for putting this together. Some questions below.

> With this patch, extra entries for firmware-reserved memory resources,
> which means any regions that are already reserved before 
> arm64_memblock_init(),
> or specifically efi/acpi tables in this case, are added to /proc/iomem.
>
>  $ cat /proc/iomem (on my qemu+edk2 execution)
> ...
> 4000-5871 : System RAM
>   4008-40f1 : Kernel code
>   4104-411e9fff : Kernel data
>   5440-583f : Crash kernel
>   5859-585e : reserved
>   5870-5871 : reserved
> 5872-58b5 : reserved
> 58b6-5be3 : System RAM
>   58b61000-58b61fff : reserved
>   59a7b118-59a7b667 : reserved
> 5be4-5bec : reserved
> 5bed-5bed : System RAM
>   5bee-5bff : reserved
> 5c00-5fff :

Re: [PATCH 0/2] ESRT fixes for relocatable kexec'd kernel

2018-03-07 Thread Tyler Baicar 

Hello Akashi,


On 3/6/2018 4:00 AM, AKASHI Takahiro wrote:

Tyler, Jeffrey,

On Fri, Mar 02, 2018 at 08:27:11AM -0500, Tyler Baicar wrote:

On 3/2/2018 12:53 AM, AKASHI Takahiro wrote:

Tyler, Jeffrey,

[Note: This issue takes place in kexec, not kdump. So to be precise,
it is not the same phenomenon as what I addressed in [1],[2]:
   [1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-February/557254.html
   [2] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/553098.html
]

On Thu, Mar 01, 2018 at 12:56:38PM -0500, Tyler Baicar wrote:

Hello,

On 2/28/2018 9:50 PM, AKASHI Takahiro wrote:

Hi,

On Wed, Feb 28, 2018 at 08:39:42AM -0700, Jeffrey Hugo wrote:

On 2/27/2018 11:19 PM, AKASHI Takahiro wrote:

Tyler,

# I missed catching your patch as its subject doesn't contain arm64.

On Fri, Feb 23, 2018 at 12:42:31PM -0700, Tyler Baicar wrote:

Currently on arm64 ESRT memory does not appear to be properly blocked off.
Upon successful initialization, ESRT prints out the memory region that it
exists in like:

esrt: Reserving ESRT space from 0x0a4c1c18 to 0x0a4c1cf0.

But then by dumping /proc/iomem this region appears as part of System RAM
rather than being reserved:

08f1-0dee : System RAM

This causes issues when trying to kexec if the kernel is relocatable. When
kexec tries to execute, this memory can be selected to relocate the kernel to
which then overwrites all the ESRT information. Then when the kexec'd kernel
tries to initialize ESRT, it doesn't recognize the ESRT version number and
just returns from efi_esrt_init().

I'm not sure what is the root cause of your problem.
Do you have good confidence that the kernel (2nd kernel image in this case?)
really overwrite ESRT region?

According to my debug, yes.
Using JTAG, I was able to determine that the ESRT memory region was getting
overwritten by the secondary kernel in
kernel/arch/arm64/kernel/relocate_kernel.S - specifically the "copy_page"
line of arm64_relocate_new_kernel()


To my best knowledge, kexec is carefully designed not to do such a thing
as it allocates a temporary buffer for kernel image and copies it to the
final destination at the very end of the 1st kernel.

My guess is that kexec, or rather kexec-tools, tries to load the kernel image
at 0x8f8 (or 0x908?, not sure) in your case. It may or may not be
overlapped with ESRT.
(Try "-d" option when executing kexec command for confirmation.)

With -d, I see

get_memory_ranges_iomem_cb: 09611000 - 0e5f : System RAM

That overlaps the ESRT reservation -
[ 0.00] esrt: Reserving ESRT space from 0x0b708718 to
0x0b7087f0


Are you using initrd with kexec?

Yes

To make the things clear, can you show me, if possible, the followings:

I have attached all of these:

Many thanks.
According to the data, ESRT was overwritten by initrd, not the kernel image.
It doesn't matter to you though :)

The solution would be, as Ard suggested, that more information be
added to /proc/iomem.
I'm going to fix the issue as quickly as possible.

Great, thank you!! Please add us to the fix and we will gladly test it out.

I have created a workaround patch, attached below, as a kind of PoC.
Can you give it a go, please?
You need another patch for kexec-tools, too. See
https:/git.linaro.org/people/takahiro.akashi/kexecl-tools.git arm64/resv_mem

With this patch, extra entries for firmware-reserved memory resources,
which means any regions that are already reserved before arm64_memblock_init(),
or specifically efi/acpi tables in this case, are added to /proc/iomem.

  $ cat /proc/iomem (on my qemu+edk2 execution)
 ...
 4000-5871 : System RAM
   4008-40f1 : Kernel code
   4104-411e9fff : Kernel data
   5440-583f : Crash kernel
   5859-585e : reserved
   5870-5871 : reserved
 5872-58b5 : reserved
 58b6-5be3 : System RAM
   58b61000-58b61fff : reserved
   59a7b118-59a7b667 : reserved
 5be4-5bec : reserved
 5bed-5bed : System RAM
   5bee-5bff : reserved
 5c00-5fff : System RAM
   5ec0-5edf : reserved
 80-ff : PCI Bus :00
   80-803fff : :00:01.0
 80-803fff : virtio-pci-modern

While all the entries are currently marked as just "reserved," we'd better
give them more specific names for general/extensive use.
(Then it will require modifying respective fw/drivers.)

Kexec-tools will allocate spaces for kernel, initrd and dtb so that
they will not be overlapped with "reserved" memory.

As I haven't run extensive tests, please let me know if you find
any problems.

Thank you! I've run the test with both the kernel patch and the kexec patch and 
can
see that this fixes the issue. I see my ESRT memory space marked as reserved:

[   

Re: [PATCH v1] efi/apple-properties: Use memremap() instead of ioremap()

2018-03-07 Thread Ard Biesheuvel 
On 7 March 2018 at 17:44, Andy Shevchenko
 wrote:
> On Tue, 2018-02-27 at 17:31 +, Ard Biesheuvel wrote:
>> On 27 February 2018 at 17:27, Andy Shevchenko
>>  wrote:
>> > The memory we are accessing through virtual address has no IO side
>> > effects. Moreover, for IO memory we have to use special accessors,
>> > which we don't use.
>> >
>>
>> Not 100% relevant in this context, but perhaps interesting
>> nonetheless: using device mappings to access things like firmware
>> tables or other structured data will break ARM and other non-x86
>> architectures that do not tolerate unaligned accessed to device
>> memory.
>>
>> > Due to above, convert the driver to use memremap() instead of
>> > ioremap().
>> >
>> > Signed-off-by: Andy Shevchenko 
>>
>> Acked-by: Ard Biesheuvel 
>
> Thanks!
>
> It's supposed to go through EFI tree.
>

Yes, I have queued it already. I expect to send out the PR to -tip by
the end of the next week.
--
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 0/9] KEYS: Blacklisting & UEFI database load

2018-03-07 Thread Mimi Zohar 
On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> On 11/16/2016, 07:10 PM, David Howells wrote:
> > Here are two sets of patches.  Firstly, the first three patches provide a
> > blacklist, making the following changes:
> ...
> > Secondly, the remaining patches allow the UEFI database to be used to load
> > the system keyrings:
> ...
> > Dave Howells (2):
> >   efi: Add EFI signature data types
> >   efi: Add an EFI signature blob parser
> > 
> > David Howells (5):
> >   KEYS: Add a system blacklist keyring
> >   X.509: Allow X.509 certs to be blacklisted
> >   PKCS#7: Handle blacklisted certificates
> >   KEYS: Allow unrestricted boot-time addition of keys to secondary 
> > keyring
> >   efi: Add SHIM and image security database GUID definitions
> > 
> > Josh Boyer (2):
> >   MODSIGN: Import certificates from UEFI Secure Boot
> >   MODSIGN: Allow the "db" UEFI variable to be suppressed
> 
> Hi,
> 
> what's the status of this please? Distributors (I checked SUSE, RedHat
> and Ubuntu) have to carry these patches and every of them have to
> forward-port the patches to new kernels. So are you going to resend the
> PR to have this merged?

With secure boot enabled, we establish a signature chain of trust,
rooted in HW, up to the kernel and then transition from those keys to
a new set of keys builtin the kernel and loaded onto the
builtin_trusted_keys (builtin).

Enabling the secondary_builtin_keys (secondary) allows keys signed by
a key on the builtin keyring to be added to the secondary keyring.
 Any key, signed by a key on either the builtin or secondary keyring,
can be added to the IMA trusted keyring.

The "KEYS: Allow unrestricted boot-time addition of keys to secondary
keyring" patch loads the platform keys directly onto the secondary
keyring, without requiring them to be signed by a key on the builtin
or secondary keyring.  With this change, any key signed by a platfrom
key on the secondary, can be loaded onto the .ima trusted keyring.

Just because I trust the platform keys prior to booting the kernel,
doesn't mean that I *want* to trust those keys once booted.  There
are, however, places where we need access to those keys to verify a
signature (eg. kexec kernel image).

Nayna Jain's "certs: define a trusted platform keyring" patch set
introduces a new, separate keyring for these platform keys.

Mimi

--
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()

2018-03-07 Thread 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: Regression from efi: call get_event_log before ExitBootServices

2018-03-07 Thread Javier Martinez Canillas 
Hi Hans,

On 03/07/2018 12:16 PM, Hans de Goede wrote:
> Hi,
> 
> On 07-03-18 09:41, Thiebaud Weksteen wrote:
>> Hi,
>>
>> Thanks for testing and sending this report! This patch relies heavily on
>> the functions exposed by the firmware. My first guess would be that some of
>> these may not be implemented correctly by the manufacturer.
>>
>> Could you share more information on this specific device?
> 
> I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel
> and I'm not seeing this problem, BIOS settings all default (I loaded
> the BIOS defaults to make sure).
> 
>> Do you have any link to the manufacturer website (I found [1] but it is
>> based on an ARM CPU)?
>> Do you have the option to update your firmware? Is a copy of the firmware
>> available from the manufacturer?
> 
> This is a really cheap Windows tablet which was given away for free in
> the Netherlands with some home-schooling language courses, or something
> similar.
> 
> Both mine and Jeremy tablets come from a website in the Netherlands
> where people can buy/sell used goods.
> 
> Most relevant for this discussion I guess is that this device is
> based on a Bay Trail Z3735G SoC, on which according to the internets:
> https://embedded.communities.intel.com/thread/7868
> 
> The TPM 2.0 it contains is implemented as part of the TXE firmware.
> 
> Since I cannot reproduce I'm thinking that maybe Jeremy actually has
> some log messages in the TPM log, where as mine is empty.  Is there a
> way to make sure some messages are in there?
>

The UEFI firmware does some measurements and so does shim. So you should
have some event logs. What version of shim are you using? And also would
be good to know if it's the same shim version that Jeremy is using.

> Regards,
> 
> Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
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()

2018-03-07 Thread 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: Regression from efi: call get_event_log before ExitBootServices

2018-03-07 Thread Hans de Goede 

Hi,

On 07-03-18 09:41, Thiebaud Weksteen wrote:

Hi,

Thanks for testing and sending this report! This patch relies heavily on
the functions exposed by the firmware. My first guess would be that some of
these may not be implemented correctly by the manufacturer.

Could you share more information on this specific device?


I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel
and I'm not seeing this problem, BIOS settings all default (I loaded
the BIOS defaults to make sure).


Do you have any link to the manufacturer website (I found [1] but it is
based on an ARM CPU)?
Do you have the option to update your firmware? Is a copy of the firmware
available from the manufacturer?


This is a really cheap Windows tablet which was given away for free in
the Netherlands with some home-schooling language courses, or something
similar.

Both mine and Jeremy tablets come from a website in the Netherlands
where people can buy/sell used goods.

Most relevant for this discussion I guess is that this device is
based on a Bay Trail Z3735G SoC, on which according to the internets:
https://embedded.communities.intel.com/thread/7868

The TPM 2.0 it contains is implemented as part of the TXE firmware.

Since I cannot reproduce I'm thinking that maybe Jeremy actually has
some log messages in the TPM log, where as mine is empty.  Is there a
way to make sure some messages are in there?

Regards,

Hans




On your side, I assume no error message got displayed on the screen when
booting. Would you be able to try to boot in an UEFI shell [2] and execute
the command "dh -v"?

Thanks,
Thiebaud

[1] https://www.gp-electronic.nl/product/7inchtablet
[2]
https://wiki.archlinux.org/index.php/Unified_Extensible_Firmware_Interface#UEFI_Shell

On Tue, Mar 6, 2018 at 5:00 PM Jeremy Cline  wrote:


Hi folks,



Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices")
causes my GP-electronic T701 tablet to hang when booting. Reverting the
patch series or hiding the TPM in the BIOS fixes the problem.



I've never fiddled with TPMs before so I'm not sure what what debugging
information to provide. It's got an Atom Z3735G and the UEFI firmware is
InsydeH20 version BYT70A.YNCHENG.WIN.007.




Regards,
Jeremy

--
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: Regression from efi: call get_event_log before ExitBootServices

2018-03-07 Thread Thiebaud Weksteen 
Hi,

Thanks for testing and sending this report! This patch relies heavily on
the functions exposed by the firmware. My first guess would be that some of
these may not be implemented correctly by the manufacturer.

Could you share more information on this specific device?
Do you have any link to the manufacturer website (I found [1] but it is
based on an ARM CPU)?
Do you have the option to update your firmware? Is a copy of the firmware
available from the manufacturer?

On your side, I assume no error message got displayed on the screen when
booting. Would you be able to try to boot in an UEFI shell [2] and execute
the command "dh -v"?

Thanks,
Thiebaud

[1] https://www.gp-electronic.nl/product/7inchtablet
[2]
https://wiki.archlinux.org/index.php/Unified_Extensible_Firmware_Interface#UEFI_Shell

On Tue, Mar 6, 2018 at 5:00 PM Jeremy Cline  wrote:

> Hi folks,

> Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices")
> causes my GP-electronic T701 tablet to hang when booting. Reverting the
> patch series or hiding the TPM in the BIOS fixes the problem.

> I've never fiddled with TPMs before so I'm not sure what what debugging
> information to provide. It's got an Atom Z3735G and the UEFI firmware is
> InsydeH20 version BYT70A.YNCHENG.WIN.007.


> Regards,
> Jeremy
--
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