[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-mips: add missing check_d

From: Michael Tokarev
Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-mips: add missing check_dspr2 for multiply instructions
Date: Thu, 09 May 2013 12:32:59 +0400
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/17.0 Icedove/17.0

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,
>       * 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

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!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]