[v2] x86/hyperv: Mark hv_ghcb_terminate() as noreturn

  Annotate the function prototype as noreturn to prevent objtool
warnings like:

vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction

Also, as per Josh's suggestion, add it to the global_noreturns list.
As a comparison, an objdump output without the annotation:

[...]
1b63:  mov    $0x1,%esi
1b68:  xor    %edi,%edi
1b6a:  callq  ffffffff8102f680 <hv_ghcb_terminate>
1b6f:  jmpq   ffffffff82f217ec <hyperv_init+0x9c> # unreachable
1b74:  cmpq   $0xffffffffffffffff,-0x702a24(%rip)
[...]

Now, after adding the __noreturn to the function prototype:

[...]
17df:  callq  ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
17e4:  test   %al,%al
17e6:  je     ffffffff82f21bb9 <hyperv_init+0x469>
[...]  <many insns>
1bb9:  mov    $0x1,%esi
1bbe:  xor    %edi,%edi
1bc0:  callq  ffffffff8102f680 <hv_ghcb_terminate>
1bc5:  nopw   %cs:0x0(%rax,%rax,1) # end of function

Reported-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/r/9698eff1-9680-4f0a-94de-590eaa923e94@app.fastmail.com/
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


V2:
- Per Josh's suggestion (thanks!), added the function to the objtool global
table as well.


 arch/x86/include/asm/mshyperv.h | 2 +-
 tools/objtool/check.c           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
  

From: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Annotate the function prototype as noreturn to prevent objtool
> warnings like:

Just curious:  Should the actual function also be updated with
__noreturn?   In similar situations, such as snp_abort(), the
__noreturn attribute is both places.   I don't know what the 
guidance is on this question.

In any case, thanks for doing this cleanup!

Michael

> 
> 
> Also, as per Josh's suggestion, add it to the global_noreturns list.
> As a comparison, an objdump output without the annotation:
>
> vmlinux.o: warning: objtool: hyperv_init+0x55c: unreachable instruction 
> [...]
> 1b63:  mov    $0x1,%esi
> 1b68:  xor    %edi,%edi
> 1b6a:  callq  ffffffff8102f680 <hv_ghcb_terminate>
> 1b6f:  jmpq   ffffffff82f217ec <hyperv_init+0x9c> # unreachable
> 1b74:  cmpq   $0xffffffffffffffff,-0x702a24(%rip)
> [...]
> 
> Now, after adding the __noreturn to the function prototype:
> 
> [...]
> 17df:  callq  ffffffff8102f6d0 <hv_ghcb_negotiate_protocol>
> 17e4:  test   %al,%al
> 17e6:  je     ffffffff82f21bb9 <hyperv_init+0x469>
> [...]  <many insns>
> 1bb9:  mov    $0x1,%esi
> 1bbe:  xor    %edi,%edi
> 1bc0:  callq  ffffffff8102f680 <hv_ghcb_terminate>
> 1bc5:  nopw   %cs:0x0(%rax,%rax,1) # end of function
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Link: https://lore.kernel.org/all/9698eff1-9680-4f0a-94de-590eaa923e94@app.fastmail.com/
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> 
> 
> V2:
> - Per Josh's suggestion (thanks!), added the function to the objtool global
> table as well.
> 
> 
>  arch/x86/include/asm/mshyperv.h | 2 +-
>  tools/objtool/check.c           | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 4c4c0ec3b62e..09c26e658bcc 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -212,7 +212,7 @@ int hv_set_mem_host_visibility(unsigned long addr, int
> numpages, bool visible);
>  void hv_ghcb_msr_write(u64 msr, u64 value);
>  void hv_ghcb_msr_read(u64 msr, u64 *value);
>  bool hv_ghcb_negotiate_protocol(void);
> -void hv_ghcb_terminate(unsigned int set, unsigned int reason);
> +void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
>  #else
>  static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
>  static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index f937be1afe65..4b5e03f61f1f 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -209,6 +209,7 @@ static bool __dead_end_function(struct objtool_file *file, struct
> symbol *func,
>  		"do_task_dead",
>  		"ex_handler_msr_mce",
>  		"fortify_panic",
> +		"hv_ghcb_terminate",
>  		"kthread_complete_and_exit",
>  		"kthread_exit",
>  		"kunit_try_catch_throw",
> --
> 2.39.2
  

On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <gpiccoli@igalia.com>
>>
>> Annotate the function prototype as noreturn to prevent objtool
>> warnings like:
> 
> Just curious:  Should the actual function also be updated with
> __noreturn?   In similar situations, such as snp_abort(), the
> __noreturn attribute is both places.   I don't know what the 
> guidance is on this question.
> 
> In any case, thanks for doing this cleanup!
> 
> Michael

Thanks Michael!

In my understanding (anybody please correct me if I'm wrong) any user of
this function that rely on this header will "inherit" the attribute -
hence, if this function is not used in any other header or statically
inside it's own definition file, it's not necessary.

But I'm glad in submitting a V3 with that if it's better, let me know.
Cheers,


Guilherme
  

On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> [...]
> Just curious:  Should the actual function also be updated with
> __noreturn?   In similar situations, such as snp_abort(), the
> __noreturn attribute is both places.   I don't know what the 
> guidance is on this question.
> 

Hi Michael / Josh (and all), lemme know if you want me to submit a V3
doing that. The function is not called inside this own definition file
nor exported, so I'm not sure that'd be necessary, but glad to do so if
you prefer.

Thanks,


Guilherme
  

From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Thursday, March 16, 2023 2:24 PM
> 
> On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> > [...]
> > Just curious:  Should the actual function also be updated with
> > __noreturn?   In similar situations, such as snp_abort(), the
> > __noreturn attribute is both places.   I don't know what the
> > guidance is on this question.
> >
> 
> Hi Michael / Josh (and all), lemme know if you want me to submit a V3
> doing that. The function is not called inside this own definition file
> nor exported, so I'm not sure that'd be necessary, but glad to do so if
> you prefer.
> 

I don't have a preference.  I was just trying to make sure the details
are all correct.  I'll defer to those with more knowledge of the
__noreturn attribute than I have.

Michael
  

On Fri, Mar 17, 2023 at 01:40:25PM +0000, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Thursday, March 16, 2023 2:24 PM
> > 
> > On 10/03/2023 18:17, Michael Kelley (LINUX) wrote:
> > > [...]
> > > Just curious:  Should the actual function also be updated with
> > > __noreturn?   In similar situations, such as snp_abort(), the
> > > __noreturn attribute is both places.   I don't know what the
> > > guidance is on this question.
> > >
> > 
> > Hi Michael / Josh (and all), lemme know if you want me to submit a V3
> > doing that. The function is not called inside this own definition file
> > nor exported, so I'm not sure that'd be necessary, but glad to do so if
> > you prefer.
> > 
> 
> I don't have a preference.  I was just trying to make sure the details
> are all correct.  I'll defer to those with more knowledge of the
> __noreturn attribute than I have.

It's not required, but probably good practice to put __noreturn in both
places to make it more self-documenting.
  

On 17/03/2023 11:53, Josh Poimboeuf wrote:
> [...]
>>> Hi Michael / Josh (and all), lemme know if you want me to submit a V3
>>> doing that. The function is not called inside this own definition file
>>> nor exported, so I'm not sure that'd be necessary, but glad to do so if
>>> you prefer.
>>>
>>
>> I don't have a preference.  I was just trying to make sure the details
>> are all correct.  I'll defer to those with more knowledge of the
>> __noreturn attribute than I have.
> 
> It's not required, but probably good practice to put __noreturn in both
> places to make it more self-documenting.
> 

Thanks Josh and Michael, will submit a V3 shortly with this improvement!
Cheers,


Guilherme