Re: [Qemu-trivial] [PATCH] target-mips: add missing check_dspr2 for multiply instructions

[Michael Tokarev]

08.05.2013 18:09, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
> 
> The emulator needs to check in hflags if DSP unit has been turned off before
> it generates code for MUL_PH, MUL_S_PH, MULQ_S_W, and MULQ_RS_W.
> 
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
>  target-mips/translate.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index b7f8203..0a53203 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -13400,6 +13400,7 @@ static void gen_mipsdsp_multiply(DisasContext *ctx, 
> uint32_t op1, uint32_t op2,
>      /* OPC_MULT_G_2E, OPC_ADDUH_QB_DSP, OPC_MUL_PH_DSP have
>       * the same mask and op1. */
>      case OPC_MULT_G_2E:
> +        check_dspr2(ctx);
>          switch (op2) {
>          case  OPC_MUL_PH:
>              gen_helper_mul_ph(cpu_gpr[ret], v1_t, v2_t, cpu_env);
> 

FWIW.  While the patch has been applied by Aurelien, I've a small
comment about the "trivialness" of patches, having in mind this
example.  This appears to be a "trivial" patch by the look of it,
that is, it is just one-liner adding a simple instruction.  But
nevertheless, it is NOT suitable for qemu-trivial, because it
need to be verified by the subsystem maintainer who understand
all the possible implications.  The patch changes core logic
which may have unexpected consequences in unexpected places,
it may lead to wrong code generation and bugs, and other "fun"
side effects.  In that way it is not "trivial", and really
need to be sent to subsystem maintainers (and to qemu-devel@),
not to the trivial patch queue.

>From not-so-trivial stuff I can consider, for example, fixes in
command line processing which do not break backward compatibility
(if it wasn't outright bug like a misspelt option name), -- but
even there, it might be a good idea to discuss it on qemu-devel
anyway.

In other words, if you know some subsystem and to you some thing
looks really trivial, it isn't necessary the same for someone
who does not have the same knowlege about that subsystem.. ;)

Thank you!

/mjt