| Message ID | 1581695768-6123-1-git-send-email-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm/emulate: fix a -Werror=cast-function-type | expand |
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.
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
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.
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)
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) >
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;
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(-)