[PATCH] kvm/emulate: fix a -Werror=cast-function-type

[PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Qian Cai]

arch/x86/kvm/emulate.c: In function 'x86_emulate_insn':
arch/x86/kvm/emulate.c:5686:22: error: cast between incompatible
function types from 'int (*)(struct x86_emulate_ctxt *)' to 'void
(*)(struct fastop *)' [-Werror=cast-function-type]
    rc = fastop(ctxt, (fastop_t)ctxt->execute);

Fixes: 3009afc6e39e ("KVM: x86: Use a typedef for fastop functions")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 arch/x86/kvm/emulate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ddbc61984227..17ae820cf59d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5682,10 +5682,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 		ctxt->eflags &= ~X86_EFLAGS_RF;
 
 	if (ctxt->execute) {
-		if (ctxt->d & Fastop)
-			rc = fastop(ctxt, (fastop_t)ctxt->execute);
-		else
+		if (ctxt->d & Fastop) {
+			fastop_t fop = (void *)ctxt->execute;
+			rc = fastop(ctxt, fop);
+		} else {
 			rc = ctxt->execute(ctxt);
+		}
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		goto writeback;
-- 
1.8.3.1

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Sean Christopherson]

On Fri, Feb 14, 2020 at 10:56:08AM -0500, Qian Cai wrote:
> arch/x86/kvm/emulate.c: In function 'x86_emulate_insn':
> arch/x86/kvm/emulate.c:5686:22: error: cast between incompatible
> function types from 'int (*)(struct x86_emulate_ctxt *)' to 'void
> (*)(struct fastop *)' [-Werror=cast-function-type]
>     rc = fastop(ctxt, (fastop_t)ctxt->execute);
> 
> Fixes: 3009afc6e39e ("KVM: x86: Use a typedef for fastop functions")
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  arch/x86/kvm/emulate.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ddbc61984227..17ae820cf59d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5682,10 +5682,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>  		ctxt->eflags &= ~X86_EFLAGS_RF;
>  
>  	if (ctxt->execute) {
> -		if (ctxt->d & Fastop)
> -			rc = fastop(ctxt, (fastop_t)ctxt->execute);

Alternatively, can we do -Wno-cast-function-type?  That's a silly warning
IMO.

If not, will either of these work?

			rc = fastop(ctxt, (void *)ctxt->execute);

or
			rc = fastop(ctxt, (fastop_t)(void *)ctxt->execute);

> -		else
> +		if (ctxt->d & Fastop) {
> +			fastop_t fop = (void *)ctxt->execute;
> +			rc = fastop(ctxt, fop);
> +		} else {
>  			rc = ctxt->execute(ctxt);
> +		}
>  		if (rc != X86EMUL_CONTINUE)
>  			goto done;
>  		goto writeback;
> -- 
> 1.8.3.1
> 

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Qian Cai]

On Fri, 2020-02-14 at 08:59 -0800, Sean Christopherson wrote:
> On Fri, Feb 14, 2020 at 10:56:08AM -0500, Qian Cai wrote:
> > arch/x86/kvm/emulate.c: In function 'x86_emulate_insn':
> > arch/x86/kvm/emulate.c:5686:22: error: cast between incompatible
> > function types from 'int (*)(struct x86_emulate_ctxt *)' to 'void
> > (*)(struct fastop *)' [-Werror=cast-function-type]
> >     rc = fastop(ctxt, (fastop_t)ctxt->execute);
> > 
> > Fixes: 3009afc6e39e ("KVM: x86: Use a typedef for fastop functions")
> > Signed-off-by: Qian Cai <cai@lca.pw>
> > ---
> >  arch/x86/kvm/emulate.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index ddbc61984227..17ae820cf59d 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -5682,10 +5682,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
> >  		ctxt->eflags &= ~X86_EFLAGS_RF;
> >  
> >  	if (ctxt->execute) {
> > -		if (ctxt->d & Fastop)
> > -			rc = fastop(ctxt, (fastop_t)ctxt->execute);
> 
> Alternatively, can we do -Wno-cast-function-type?  That's a silly warning
> IMO.

I am doing W=1 on linux-next where some of the warnings might be silly but the
recent commit changes all warnings to errors forces me having to silence those
somehow.

> 
> If not, will either of these work?
> 
> 			rc = fastop(ctxt, (void *)ctxt->execute);
> 
> or
> 			rc = fastop(ctxt, (fastop_t)(void *)ctxt->execute);

I have no strong preference. I originally thought just to go back the previous
code style where might be more acceptable, but it is up to maintainers.

> 
> > -		else
> > +		if (ctxt->d & Fastop) {
> > +			fastop_t fop = (void *)ctxt->execute;
> > +			rc = fastop(ctxt, fop);
> > +		} else {
> >  			rc = ctxt->execute(ctxt);
> > +		}
> >  		if (rc != X86EMUL_CONTINUE)
> >  			goto done;
> >  		goto writeback;
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Jim Mattson]

On Fri, Feb 14, 2020 at 9:08 AM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2020-02-14 at 08:59 -0800, Sean Christopherson wrote:
> > On Fri, Feb 14, 2020 at 10:56:08AM -0500, Qian Cai wrote:
> > > arch/x86/kvm/emulate.c: In function 'x86_emulate_insn':
> > > arch/x86/kvm/emulate.c:5686:22: error: cast between incompatible
> > > function types from 'int (*)(struct x86_emulate_ctxt *)' to 'void
> > > (*)(struct fastop *)' [-Werror=cast-function-type]
> > >     rc = fastop(ctxt, (fastop_t)ctxt->execute);
> > >
> > > Fixes: 3009afc6e39e ("KVM: x86: Use a typedef for fastop functions")
> > > Signed-off-by: Qian Cai <cai@lca.pw>
> > > ---
> > >  arch/x86/kvm/emulate.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > index ddbc61984227..17ae820cf59d 100644
> > > --- a/arch/x86/kvm/emulate.c
> > > +++ b/arch/x86/kvm/emulate.c
> > > @@ -5682,10 +5682,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
> > >             ctxt->eflags &= ~X86_EFLAGS_RF;
> > >
> > >     if (ctxt->execute) {
> > > -           if (ctxt->d & Fastop)
> > > -                   rc = fastop(ctxt, (fastop_t)ctxt->execute);
> >
> > Alternatively, can we do -Wno-cast-function-type?  That's a silly warning
> > IMO.
>
> I am doing W=1 on linux-next where some of the warnings might be silly but the
> recent commit changes all warnings to errors forces me having to silence those
> somehow.
>
> >
> > If not, will either of these work?
> >
> >                       rc = fastop(ctxt, (void *)ctxt->execute);
> >
> > or
> >                       rc = fastop(ctxt, (fastop_t)(void *)ctxt->execute);
>
> I have no strong preference. I originally thought just to go back the previous
> code style where might be more acceptable, but it is up to maintainers.

It seems misguided to define a local variable just to get an implicit
cast from (void *) to (fastop_t). Sean's first suggestion gives you
the same implicit cast without the local variable. The second
suggestion makes both casts explicit.

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Qian Cai]

On Fri, 2020-02-14 at 09:40 -0800, Jim Mattson wrote:
> On Fri, Feb 14, 2020 at 9:08 AM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2020-02-14 at 08:59 -0800, Sean Christopherson wrote:
> > > On Fri, Feb 14, 2020 at 10:56:08AM -0500, Qian Cai wrote:
> > > > arch/x86/kvm/emulate.c: In function 'x86_emulate_insn':
> > > > arch/x86/kvm/emulate.c:5686:22: error: cast between incompatible
> > > > function types from 'int (*)(struct x86_emulate_ctxt *)' to 'void
> > > > (*)(struct fastop *)' [-Werror=cast-function-type]
> > > >     rc = fastop(ctxt, (fastop_t)ctxt->execute);
> > > > 
> > > > Fixes: 3009afc6e39e ("KVM: x86: Use a typedef for fastop functions")
> > > > Signed-off-by: Qian Cai <cai@lca.pw>
> > > > ---
> > > >  arch/x86/kvm/emulate.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > > index ddbc61984227..17ae820cf59d 100644
> > > > --- a/arch/x86/kvm/emulate.c
> > > > +++ b/arch/x86/kvm/emulate.c
> > > > @@ -5682,10 +5682,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
> > > >             ctxt->eflags &= ~X86_EFLAGS_RF;
> > > > 
> > > >     if (ctxt->execute) {
> > > > -           if (ctxt->d & Fastop)
> > > > -                   rc = fastop(ctxt, (fastop_t)ctxt->execute);
> > > 
> > > Alternatively, can we do -Wno-cast-function-type?  That's a silly warning
> > > IMO.
> > 
> > I am doing W=1 on linux-next where some of the warnings might be silly but the
> > recent commit changes all warnings to errors forces me having to silence those
> > somehow.
> > 
> > > 
> > > If not, will either of these work?
> > > 
> > >                       rc = fastop(ctxt, (void *)ctxt->execute);
> > > 
> > > or
> > >                       rc = fastop(ctxt, (fastop_t)(void *)ctxt->execute);
> > 
> > I have no strong preference. I originally thought just to go back the previous
> > code style where might be more acceptable, but it is up to maintainers.
> 
> It seems misguided to define a local variable just to get an implicit
> cast from (void *) to (fastop_t). Sean's first suggestion gives you
> the same implicit cast without the local variable. The second
> suggestion makes both casts explicit.

OK, I'll do a v2 using the first suggestion which looks simpler once it passed
compilations.

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Paolo Bonzini]

On 14/02/20 20:14, Qian Cai wrote:
>> It seems misguided to define a local variable just to get an implicit
>> cast from (void *) to (fastop_t). Sean's first suggestion gives you
>> the same implicit cast without the local variable. The second
>> suggestion makes both casts explicit.
> 
> OK, I'll do a v2 using the first suggestion which looks simpler once it passed
> compilations.
> 

Another interesting possibility is to use an unnamed union of a
(*execute) function pointer and a (*fastop) function pointer.

Paolo

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Sean Christopherson]

On Fri, Feb 14, 2020 at 08:33:50PM +0100, Paolo Bonzini wrote:
> On 14/02/20 20:14, Qian Cai wrote:
> >> It seems misguided to define a local variable just to get an implicit
> >> cast from (void *) to (fastop_t). Sean's first suggestion gives you
> >> the same implicit cast without the local variable. The second
> >> suggestion makes both casts explicit.
> > 
> > OK, I'll do a v2 using the first suggestion which looks simpler once it passed
> > compilations.
> > 
> 
> Another interesting possibility is to use an unnamed union of a
> (*execute) function pointer and a (*fastop) function pointer.

I considered that when introducing fastop_t.  I don't remember why I
didn't go that route.  It's entirely possible I completely forgot that
anonymous unions are allowed and thought it would mean changing a bunch
of use sites.

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Qian Cai]

On Fri, 2020-02-14 at 20:33 +0100, Paolo Bonzini wrote:
> On 14/02/20 20:14, Qian Cai wrote:
> > > It seems misguided to define a local variable just to get an implicit
> > > cast from (void *) to (fastop_t). Sean's first suggestion gives you
> > > the same implicit cast without the local variable. The second
> > > suggestion makes both casts explicit.
> > 
> > OK, I'll do a v2 using the first suggestion which looks simpler once it passed
> > compilations.
> > 
> 
> Another interesting possibility is to use an unnamed union of a
> (*execute) function pointer and a (*fastop) function pointer.
> 

This?

diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index 03946eb3e2b9..2a8f2bd2e5cf 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -292,6 +292,14 @@ enum x86emul_mode {
 #define X86EMUL_SMM_MASK             (1 << 6)
 #define X86EMUL_SMM_INSIDE_NMI_MASK  (1 << 7)
 
+/*
+ * fastop functions are declared as taking a never-defined fastop parameter,
+ * so they can't be called from C directly.
+ */
+struct fastop;
+
+typedef void (*fastop_t)(struct fastop *);
+
 struct x86_emulate_ctxt {
 	const struct x86_emulate_ops *ops;
 
@@ -324,7 +332,10 @@ struct x86_emulate_ctxt {
 	struct operand src;
 	struct operand src2;
 	struct operand dst;
-	int (*execute)(struct x86_emulate_ctxt *ctxt);
+	union {
+		int (*execute)(struct x86_emulate_ctxt *ctxt);
+		fastop_t fop;
+	};
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
 	/*
 	 * The following six fields are cleared together,
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ddbc61984227..dd19fb3539e0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -191,25 +191,6 @@
 #define NR_FASTOP (ilog2(sizeof(ulong)) + 1)
 #define FASTOP_SIZE 8
 
-/*
- * fastop functions have a special calling convention:
- *
- * dst:    rax        (in/out)
- * src:    rdx        (in/out)
- * src2:   rcx        (in)
- * flags:  rflags     (in/out)
- * ex:     rsi        (in:fastop pointer, out:zero if exception)
- *
- * Moreover, they are all exactly FASTOP_SIZE bytes long, so functions for
- * different operand sizes can be reached by calculation, rather than a jump
- * table (which would be bigger than the code).
- *
- * fastop functions are declared as taking a never-defined fastop parameter,
- * so they can't be called from C directly.
- */
-
-struct fastop;
-
 struct opcode {
 	u64 flags : 56;
 	u64 intercept : 8;
@@ -311,8 +292,19 @@ static void invalidate_registers(struct x86_emulate_ctxt
*ctxt)
 #define ON64(x)
 #endif
 
-typedef void (*fastop_t)(struct fastop *);
-
+/*
+ * fastop functions have a special calling convention:
+ *
+ * dst:    rax        (in/out)
+ * src:    rdx        (in/out)
+ * src2:   rcx        (in)
+ * flags:  rflags     (in/out)
+ * ex:     rsi        (in:fastop pointer, out:zero if exception)
+ *
+ * Moreover, they are all exactly FASTOP_SIZE bytes long, so functions for
+ * different operand sizes can be reached by calculation, rather than a jump
+ * table (which would be bigger than the code).
+ */
 static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 
 #define __FOP_FUNC(name) \
@@ -5683,7 +5675,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 
 	if (ctxt->execute) {
 		if (ctxt->d & Fastop)
-			rc = fastop(ctxt, (fastop_t)ctxt->execute);
+			rc = fastop(ctxt, ctxt->fop);
 		else
 			rc = ctxt->execute(ctxt);
 		if (rc != X86EMUL_CONTINUE)

Re: [PATCH] kvm/emulate: fix a -Werror=cast-function-type

[Paolo Bonzini]

On 17/02/20 15:47, Qian Cai wrote:
> On Fri, 2020-02-14 at 20:33 +0100, Paolo Bonzini wrote:
>> On 14/02/20 20:14, Qian Cai wrote:
>>>> It seems misguided to define a local variable just to get an implicit
>>>> cast from (void *) to (fastop_t). Sean's first suggestion gives you
>>>> the same implicit cast without the local variable. The second
>>>> suggestion makes both casts explicit.
>>>
>>> OK, I'll do a v2 using the first suggestion which looks simpler once it passed
>>> compilations.
>>>
>>
>> Another interesting possibility is to use an unnamed union of a
>> (*execute) function pointer and a (*fastop) function pointer.
>>
> 
> This?

Yes, perfect.  Can you send it with Signed-off-by and all that?

Thanks,

Paolo

> diff --git a/arch/x86/include/asm/kvm_emulate.h
> b/arch/x86/include/asm/kvm_emulate.h
> index 03946eb3e2b9..2a8f2bd2e5cf 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -292,6 +292,14 @@ enum x86emul_mode {
>  #define X86EMUL_SMM_MASK             (1 << 6)
>  #define X86EMUL_SMM_INSIDE_NMI_MASK  (1 << 7)
>  
> +/*
> + * fastop functions are declared as taking a never-defined fastop parameter,
> + * so they can't be called from C directly.
> + */
> +struct fastop;
> +
> +typedef void (*fastop_t)(struct fastop *);
> +
>  struct x86_emulate_ctxt {
>  	const struct x86_emulate_ops *ops;
>  
> @@ -324,7 +332,10 @@ struct x86_emulate_ctxt {
>  	struct operand src;
>  	struct operand src2;
>  	struct operand dst;
> -	int (*execute)(struct x86_emulate_ctxt *ctxt);
> +	union {
> +		int (*execute)(struct x86_emulate_ctxt *ctxt);
> +		fastop_t fop;
> +	};
>  	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
>  	/*
>  	 * The following six fields are cleared together,
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ddbc61984227..dd19fb3539e0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -191,25 +191,6 @@
>  #define NR_FASTOP (ilog2(sizeof(ulong)) + 1)
>  #define FASTOP_SIZE 8
>  
> -/*
> - * fastop functions have a special calling convention:
> - *
> - * dst:    rax        (in/out)
> - * src:    rdx        (in/out)
> - * src2:   rcx        (in)
> - * flags:  rflags     (in/out)
> - * ex:     rsi        (in:fastop pointer, out:zero if exception)
> - *
> - * Moreover, they are all exactly FASTOP_SIZE bytes long, so functions for
> - * different operand sizes can be reached by calculation, rather than a jump
> - * table (which would be bigger than the code).
> - *
> - * fastop functions are declared as taking a never-defined fastop parameter,
> - * so they can't be called from C directly.
> - */
> -
> -struct fastop;
> -
>  struct opcode {
>  	u64 flags : 56;
>  	u64 intercept : 8;
> @@ -311,8 +292,19 @@ static void invalidate_registers(struct x86_emulate_ctxt
> *ctxt)
>  #define ON64(x)
>  #endif
>  
> -typedef void (*fastop_t)(struct fastop *);
> -
> +/*
> + * fastop functions have a special calling convention:
> + *
> + * dst:    rax        (in/out)
> + * src:    rdx        (in/out)
> + * src2:   rcx        (in)
> + * flags:  rflags     (in/out)
> + * ex:     rsi        (in:fastop pointer, out:zero if exception)
> + *
> + * Moreover, they are all exactly FASTOP_SIZE bytes long, so functions for
> + * different operand sizes can be reached by calculation, rather than a jump
> + * table (which would be bigger than the code).
> + */
>  static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  
>  #define __FOP_FUNC(name) \
> @@ -5683,7 +5675,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>  
>  	if (ctxt->execute) {
>  		if (ctxt->d & Fastop)
> -			rc = fastop(ctxt, (fastop_t)ctxt->execute);
> +			rc = fastop(ctxt, ctxt->fop);
>  		else
>  			rc = ctxt->execute(ctxt);
>  		if (rc != X86EMUL_CONTINUE)
>