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