Re: [Qemu-devel] [PATCH v3] target-sparc: Add and use CPU_FEATURE_CASA

[Andreas Färber]

Am 14.02.2014 16:43, schrieb Sebastian Huber:
> On 2014-02-14 16:33, Andreas Färber wrote:
>>> @@ -5120,6 +5119,20 @@ static void disas_sparc_insn(DisasContext *
>>> dc, unsigned int insn)
>>> >                  case 0x37: /* stdc */
>>> >                      goto ncp_insn;
>>> >  #endif
>>> >+#if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
>>> >+                case 0x3c: /* V9 or LEON3 casa */
>>> >+                    CHECK_FPU_FEATURE(dc, CASA);
>>> >+#ifndef TARGET_SPARC64
>>> >+                    if (IS_IMM)
>>
>> Did you forget to run checkpatch.pl? Missing braces here ...
>>
>>> >+                        goto illegal_insn;
>>> >+                    if (!supervisor(dc))
>> ... and here.
>>
>>> >+                        goto priv_insn;
>> Otherwise patch looks okay, thanks for the subject normalization.
> 
> I didn't know there was a checkpatch.pl, I run it next time.

http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html
may be helpful then. :)

There's also scripts/get_maintainer.pl --nogit-fallback.

> This code fragment is a copy and paste move from another place in this
> file. There a lot of similar style errors in this file.

Our policy is to have new code adhere to the official Coding Style. If
you want, you can do a two-patch series (in which case we ask for a
cover letter...), with the initial one cleaning up the existing style
faults and the second one doing the actual change. But to be clear, such
cleanup of old code is entirely optional.

>> As for the other one you'll need to sort our who sends a pull if Blue
>> doesn't resurface - I note that qemu-trivial is not CC'ed here and the
>> patch probably isn't anyway. Maybe Fabien can help out with that?
>>
>> More generally, independent of this patch with test case in its commit
>> message, you may want to consider setting up tests/tcg/sparc/ for test
>> cases and/or use qtest with accel=tcg similar to what mst did for ACPI
>> regression testing.
> 
> Ok, I will have a look at this.  Is this a required step to get the
> patch committed?

No, that was a forward-looking suggestion, seeing you providing a test
case in the commit message.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg