Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 16:40 +, David Howells wrote:
> Joe Perches  wrote:
> 
> > Umm, no, that's not correct.
> > SHA-1 lengths of 12 are unique for quite awhile yet.
> > 
> > https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
> 
> The article says:
> 
>   1.9% at 12
> 
> which is for 3253824 objects (I get 1.86%).
> 
> However, that was three years ago, and we now have over five million objects,
> so the collision possibility is 4.5% now.
> 
> If we add another 2 million over the next three years, then the probability
> will be over 8% then.
> 
> I've attached my spreadsheet for you to have a look at.
> 
> > No.  Use --format=email as appropriate instead.
> 
> Fix checkpatch.  This is an entirely reasonable supposition.

No.  There's nothing to fix there IMO.
Of course you are welcome to submit patches.

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 16:40 +, David Howells wrote:
> Joe Perches  wrote:
> 
> > Umm, no, that's not correct.
> > SHA-1 lengths of 12 are unique for quite awhile yet.
> > 
> > https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
> 
> The article says:
> 
>   1.9% at 12
> 
> which is for 3253824 objects (I get 1.86%).
> 
> However, that was three years ago, and we now have over five million objects,
> so the collision possibility is 4.5% now.
> 
> If we add another 2 million over the next three years, then the probability
> will be over 8% then.
> 
> I've attached my spreadsheet for you to have a look at.
> 
> > No.  Use --format=email as appropriate instead.
> 
> Fix checkpatch.  This is an entirely reasonable supposition.

No.  There's nothing to fix there IMO.
Of course you are welcome to submit patches.

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Joe Perches  wrote:

> Umm, no, that's not correct.
> SHA-1 lengths of 12 are unique for quite awhile yet.
> 
> https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

The article says:

1.9% at 12

which is for 3253824 objects (I get 1.86%).

However, that was three years ago, and we now have over five million objects,
so the collision possibility is 4.5% now.

If we add another 2 million over the next three years, then the probability
will be over 8% then.

I've attached my spreadsheet for you to have a look at.

> No.  Use --format=email as appropriate instead.

Fix checkpatch.  This is an entirely reasonable supposition.

David



birthday-problem.ods
Description: application/vnd.oasis.opendocument.spreadsheet

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Joe Perches  wrote:

> Umm, no, that's not correct.
> SHA-1 lengths of 12 are unique for quite awhile yet.
> 
> https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

The article says:

1.9% at 12

which is for 3253824 objects (I get 1.86%).

However, that was three years ago, and we now have over five million objects,
so the collision possibility is 4.5% now.

If we add another 2 million over the next three years, then the probability
will be over 8% then.

I've attached my spreadsheet for you to have a look at.

> No.  Use --format=email as appropriate instead.

Fix checkpatch.  This is an entirely reasonable supposition.

David



birthday-problem.ods
Description: application/vnd.oasis.opendocument.spreadsheet

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 15:52 +, David Howells wrote:
> Joe Perches  wrote:
> 
> > > > Small nit, checkpatch usually complains that this should be written as
> > > > 12-character SHA-1 followed by the commit subject, i.e.
> > > > 
> > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > > 
> > > In this case, checkpatch is wrong.
> > 
> > Why do you think so?
> 
> Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
> that case, it's just about acceptable.

checkpatch still emits warnings about the format of
commmit IDs.

What version of checkpatch are yuu using?

> Apart from that, I think we should put in the full SHA-1 commit.  The
> probability of a collision in a 12-digit hex number for the >5,000,000 commits
> just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
> all the commits in not-yet-upstreamed trees - which might be another million
> commits, say - then we're over 6%..

Umm, no, that's not correct.
SHA-1 lengths of 12 are unique for quite awhile yet.

https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

Using Linus' tree today, from commit 3b404a519815
the current output of the git-uniq-abbrev script is:

$ git-uniq-abbrev 
5048673 objects
 4: 5048673 / 65536
 5: 5007413 / 998721
 6: 1312496 / 623343
 7: 94487 / 47089
 8: 6163 / 3081
 9: 416 / 208
10: 28 / 14
11: 4 / 2
12: 0 / 0
d597639e2036f04f0226761e2d818b31f2db7820
d597639e203a100156501df8a0756fd09573e2de
ef91b6e893a00d903400f8e1303efc4d52b710af
ef91b6e893afc4c4ca488453ea9f19ced5fa5861

> Oh, yes, and speaking of checkpatch, can you make it so that if it sees:
> 
> commit 12345...
> Author: foo 
> Date: blah
> 
>Subject line
> 
>Description lines
>...
>...
>...
>...
> 
>Signed-off-by-and-suchline-lines
> 
> diff ...
> 
> with the all description indented by 4 spaces, then assume that it's the
> output of git show and not give the warnings about signed-off-by and other
> things being indented?

No.  Use --format=email as appropriate instead.

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 15:52 +, David Howells wrote:
> Joe Perches  wrote:
> 
> > > > Small nit, checkpatch usually complains that this should be written as
> > > > 12-character SHA-1 followed by the commit subject, i.e.
> > > > 
> > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > > 
> > > In this case, checkpatch is wrong.
> > 
> > Why do you think so?
> 
> Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
> that case, it's just about acceptable.

checkpatch still emits warnings about the format of
commmit IDs.

What version of checkpatch are yuu using?

> Apart from that, I think we should put in the full SHA-1 commit.  The
> probability of a collision in a 12-digit hex number for the >5,000,000 commits
> just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
> all the commits in not-yet-upstreamed trees - which might be another million
> commits, say - then we're over 6%..

Umm, no, that's not correct.
SHA-1 lengths of 12 are unique for quite awhile yet.

https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

Using Linus' tree today, from commit 3b404a519815
the current output of the git-uniq-abbrev script is:

$ git-uniq-abbrev 
5048673 objects
 4: 5048673 / 65536
 5: 5007413 / 998721
 6: 1312496 / 623343
 7: 94487 / 47089
 8: 6163 / 3081
 9: 416 / 208
10: 28 / 14
11: 4 / 2
12: 0 / 0
d597639e2036f04f0226761e2d818b31f2db7820
d597639e203a100156501df8a0756fd09573e2de
ef91b6e893a00d903400f8e1303efc4d52b710af
ef91b6e893afc4c4ca488453ea9f19ced5fa5861

> Oh, yes, and speaking of checkpatch, can you make it so that if it sees:
> 
> commit 12345...
> Author: foo 
> Date: blah
> 
>Subject line
> 
>Description lines
>...
>...
>...
>...
> 
>Signed-off-by-and-suchline-lines
> 
> diff ...
> 
> with the all description indented by 4 spaces, then assume that it's the
> output of git show and not give the warnings about signed-off-by and other
> things being indented?

No.  Use --format=email as appropriate instead.

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Joe Perches  wrote:

> > > Small nit, checkpatch usually complains that this should be written as
> > > 12-character SHA-1 followed by the commit subject, i.e.
> > > 
> > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > 
> > In this case, checkpatch is wrong.
>
> Why do you think so?

Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
that case, it's just about acceptable.

Apart from that, I think we should put in the full SHA-1 commit.  The
probability of a collision in a 12-digit hex number for the >5,000,000 commits
just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
all the commits in not-yet-upstreamed trees - which might be another million
commits, say - then we're over 6%..

Oh, yes, and speaking of checkpatch, can you make it so that if it sees:

commit 12345...
Author: foo 
Date: blah

   Subject line

   Description lines
   ...
   ...
   ...
   ...

   Signed-off-by-and-suchline-lines

diff ...

with the all description indented by 4 spaces, then assume that it's the
output of git show and not give the warnings about signed-off-by and other
things being indented?

David

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Joe Perches  wrote:

> > > Small nit, checkpatch usually complains that this should be written as
> > > 12-character SHA-1 followed by the commit subject, i.e.
> > > 
> > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > 
> > In this case, checkpatch is wrong.
>
> Why do you think so?

Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
that case, it's just about acceptable.

Apart from that, I think we should put in the full SHA-1 commit.  The
probability of a collision in a 12-digit hex number for the >5,000,000 commits
just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
all the commits in not-yet-upstreamed trees - which might be another million
commits, say - then we're over 6%..

Oh, yes, and speaking of checkpatch, can you make it so that if it sees:

commit 12345...
Author: foo 
Date: blah

   Subject line

   Description lines
   ...
   ...
   ...
   ...

   Signed-off-by-and-suchline-lines

diff ...

with the all description indented by 4 spaces, then assume that it's the
output of git show and not give the warnings about signed-off-by and other
things being indented?

David

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 14:17 +, David Howells wrote:
> Lukas Wunner  wrote:
> 
> > Small nit, checkpatch usually complains that this should be written as
> > 12-character SHA-1 followed by the commit subject, i.e.
> > 
> > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> 
> In this case, checkpatch is wrong.

Why do you think so?

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Joe Perches]

On Tue, 2016-11-22 at 14:17 +, David Howells wrote:
> Lukas Wunner  wrote:
> 
> > Small nit, checkpatch usually complains that this should be written as
> > 12-character SHA-1 followed by the commit subject, i.e.
> > 
> > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> 
> In this case, checkpatch is wrong.

Why do you think so?

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Lukas Wunner  wrote:

> Small nit, checkpatch usually complains that this should be written as
> 12-character SHA-1 followed by the commit subject, i.e.
> 
> 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

In this case, checkpatch is wrong.

David

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[David Howells]

Lukas Wunner  wrote:

> Small nit, checkpatch usually complains that this should be written as
> 12-character SHA-1 followed by the commit subject, i.e.
> 
> 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

In this case, checkpatch is wrong.

David

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Lukas Wunner]

On Tue, Nov 22, 2016 at 12:31:39AM +, David Howells wrote:
> Provide the ability to perform mixed-mode runtime service calls for x86 in
> the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides

Small nit, checkpatch usually complains that this should be written as
12-character SHA-1 followed by the commit subject, i.e.

0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

Other than that LGTM.  Same for patch 2 of this series.

Thanks,

Lukas

> the ability to invoke arbitrary boot services.
> 
> Suggested-by: Lukas Wunner 
> Signed-off-by: David Howells 
> ---
> 
>  arch/x86/boot/compressed/eboot.c   |1 +
>  arch/x86/boot/compressed/head_32.S |6 +++---
>  arch/x86/boot/compressed/head_64.S |8 
>  arch/x86/include/asm/efi.h |5 +
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index ff01c8fc76f7..c8c32ebcdfdb 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c) 
> \
>   \
>   table = (typeof(table))sys_table;   \
>   \
> + c->runtime_services = table->runtime;   \
>   c->boot_services = table->boottime; \
>   c->text_output = table->con_out;\
>  }
> diff --git a/arch/x86/boot/compressed/head_32.S 
> b/arch/x86/boot/compressed/head_32.S
> index fd0b6a272dd5..d85b9625e836 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
>  
>   /* Relocate efi_config->call() */
>   lealefi32_config(%esi), %eax
> - add %esi, 32(%eax)
> + add %esi, 40(%eax)
>   pushl   %eax
>  
>   callmake_boot_params
> @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
>  
>   /* Relocate efi_config->call() */
>   lealefi32_config(%esi), %eax
> - add %esi, 32(%eax)
> + add %esi, 40(%eax)
>   pushl   %eax
>  2:
>   callefi_main
> @@ -264,7 +264,7 @@ relocated:
>  #ifdef CONFIG_EFI_STUB
>   .data
>  efi32_config:
> - .fill 4,8,0
> + .fill 5,8,0
>   .long efi_call_phys
>   .long 0
>   .byte 0
> diff --git a/arch/x86/boot/compressed/head_64.S 
> b/arch/x86/boot/compressed/head_64.S
> index efdfba21a5b2..beab8322f72a 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
>   /*
>* Relocate efi_config->call().
>*/
> - addq%rbp, efi64_config+32(%rip)
> + addq%rbp, efi64_config+40(%rip)
>  
>   movq%rax, %rdi
>   callmake_boot_params
> @@ -285,7 +285,7 @@ handover_entry:
>* Relocate efi_config->call().
>*/
>   movqefi_config(%rip), %rax
> - addq%rbp, 32(%rax)
> + addq%rbp, 40(%rax)
>  2:
>   movqefi_config(%rip), %rdi
>   callefi_main
> @@ -457,14 +457,14 @@ efi_config:
>  #ifdef CONFIG_EFI_MIXED
>   .global efi32_config
>  efi32_config:
> - .fill   4,8,0
> + .fill   5,8,0
>   .quad   efi64_thunk
>   .byte   0
>  #endif
>  
>   .global efi64_config
>  efi64_config:
> - .fill   4,8,0
> + .fill   5,8,0
>   .quad   efi_call
>   .byte   1
>  #endif /* CONFIG_EFI_STUB */
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index e99675b9c861..2f77bcefe6b4 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -191,6 +191,7 @@ static inline efi_status_t 
> efi_thunk_set_virtual_address_map(
>  struct efi_config {
>   u64 image_handle;
>   u64 table;
> + u64 runtime_services;
>   u64 boot_services;
>   u64 text_output;
>   efi_status_t (*call)(unsigned long, ...);
> @@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void)
>  #define __efi_call_early(f, ...) \
>   __efi_early()->call((unsigned long)f, __VA_ARGS__);
>  
> +#define efi_call_runtime(f, ...) \
> + __efi_early()->call(efi_table_attr(efi_runtime_services, f, \
> + __efi_early()->runtime_services), __VA_ARGS__)
> +
>  extern bool efi_reboot_required(void);
>  
>  #else
> 

Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

[Lukas Wunner]

On Tue, Nov 22, 2016 at 12:31:39AM +, David Howells wrote:
> Provide the ability to perform mixed-mode runtime service calls for x86 in
> the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187 provides

Small nit, checkpatch usually complains that this should be written as
12-character SHA-1 followed by the commit subject, i.e.

0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")

Other than that LGTM.  Same for patch 2 of this series.

Thanks,

Lukas

> the ability to invoke arbitrary boot services.
> 
> Suggested-by: Lukas Wunner 
> Signed-off-by: David Howells 
> ---
> 
>  arch/x86/boot/compressed/eboot.c   |1 +
>  arch/x86/boot/compressed/head_32.S |6 +++---
>  arch/x86/boot/compressed/head_64.S |8 
>  arch/x86/include/asm/efi.h |5 +
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index ff01c8fc76f7..c8c32ebcdfdb 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c) 
> \
>   \
>   table = (typeof(table))sys_table;   \
>   \
> + c->runtime_services = table->runtime;   \
>   c->boot_services = table->boottime; \
>   c->text_output = table->con_out;\
>  }
> diff --git a/arch/x86/boot/compressed/head_32.S 
> b/arch/x86/boot/compressed/head_32.S
> index fd0b6a272dd5..d85b9625e836 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
>  
>   /* Relocate efi_config->call() */
>   lealefi32_config(%esi), %eax
> - add %esi, 32(%eax)
> + add %esi, 40(%eax)
>   pushl   %eax
>  
>   callmake_boot_params
> @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
>  
>   /* Relocate efi_config->call() */
>   lealefi32_config(%esi), %eax
> - add %esi, 32(%eax)
> + add %esi, 40(%eax)
>   pushl   %eax
>  2:
>   callefi_main
> @@ -264,7 +264,7 @@ relocated:
>  #ifdef CONFIG_EFI_STUB
>   .data
>  efi32_config:
> - .fill 4,8,0
> + .fill 5,8,0
>   .long efi_call_phys
>   .long 0
>   .byte 0
> diff --git a/arch/x86/boot/compressed/head_64.S 
> b/arch/x86/boot/compressed/head_64.S
> index efdfba21a5b2..beab8322f72a 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
>   /*
>* Relocate efi_config->call().
>*/
> - addq%rbp, efi64_config+32(%rip)
> + addq%rbp, efi64_config+40(%rip)
>  
>   movq%rax, %rdi
>   callmake_boot_params
> @@ -285,7 +285,7 @@ handover_entry:
>* Relocate efi_config->call().
>*/
>   movqefi_config(%rip), %rax
> - addq%rbp, 32(%rax)
> + addq%rbp, 40(%rax)
>  2:
>   movqefi_config(%rip), %rdi
>   callefi_main
> @@ -457,14 +457,14 @@ efi_config:
>  #ifdef CONFIG_EFI_MIXED
>   .global efi32_config
>  efi32_config:
> - .fill   4,8,0
> + .fill   5,8,0
>   .quad   efi64_thunk
>   .byte   0
>  #endif
>  
>   .global efi64_config
>  efi64_config:
> - .fill   4,8,0
> + .fill   5,8,0
>   .quad   efi_call
>   .byte   1
>  #endif /* CONFIG_EFI_STUB */
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index e99675b9c861..2f77bcefe6b4 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -191,6 +191,7 @@ static inline efi_status_t 
> efi_thunk_set_virtual_address_map(
>  struct efi_config {
>   u64 image_handle;
>   u64 table;
> + u64 runtime_services;
>   u64 boot_services;
>   u64 text_output;
>   efi_status_t (*call)(unsigned long, ...);
> @@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void)
>  #define __efi_call_early(f, ...) \
>   __efi_early()->call((unsigned long)f, __VA_ARGS__);
>  
> +#define efi_call_runtime(f, ...) \
> + __efi_early()->call(efi_table_attr(efi_runtime_services, f, \
> + __efi_early()->runtime_services), __VA_ARGS__)
> +
>  extern bool efi_reboot_required(void);
>  
>  #else
>