Re: [Qemu-devel] Add CMP2 instruction

[Andreas Färber]

Hi,

Am 07.11.2014 um 11:14 schrieb Guo, Lei:
> This patch aims to add CMP2 instruction for m68k family.
> 
>  
> 
> *Description: *Compares the value in Rn to each bound. The effective
> address contains the
> 
> bounds pair: upper bound following the lower bound. For signed
> comparisons, the
> 
> arithmetically smaller value should be used as the lower bound. For unsigned
> 
> comparisons, the logically smaller value should be the lower bound.
> 
> The size of the data and the bounds can be specified as byte, word, or
> long. If Rn is a
> 
> data register and the operation size is byte or word, only the
> appropriate low-order part
> 
> of Rn is checked. If Rn is an address register and the operation size is
> byte or word,
> 
> the bounds operands are sign-extended to 32 bits, and the resultant
> operands are
> 
> compared to the full 32 bits of An.
> 
> If the upper bound equals the lower bound, the valid range is a single
> value.
> 
>  
> 
> signed-off-by: Guolei <address@hidden
> <mailto:address@hidden>>
> 
>  
> 
> ---

Please talk to your Fujitsu colleagues for how to correctly submit
patches to our mailing list. As you can see above, it arrives rather
"funny" due to your use of HTML. As a sanity check, you can mail the
patch to yourself, save it from your inbox to disk and try to apply it
via "git am yourmailfilename".

Why do you write "Description:" in the patch description? Is that a
verbatim copy from the manual? In that case you should name the source.
Otherwise please drop "Description:" and just describe it.

Also, Signed-off-by with a capital S please, and no space after your
email. If you use git commit -s you can avoid such issues from the start.

Please also use a meaningful topic in the subject, "target-m68k: Add
CMP2 instruction" tells us that this is not for x86 or ARM etc.

More information here:
http://wiki.qemu-project.org/Contribute/SubmitAPatch

As for the contents, you should be aware that QEMU currently emulates
ColdFire only and not the full 68k instruction set. So you may need to
limit your new instruction to certain CPU types. CC'ing Laurent+Andreas.

You haven't replied to Thomas' question on your previous submission
either. If you resubmit, please either mark as [PATCH RESEND], or if
things changed (such as adding the s ;)) as [PATCH v2] and include a
summary of changes below ---.

Note that there is currently no dedicated maintainer to review m68k
patches, so only obviously correct bug fixes are likely to get accepted
unless someone (you?) steps up for reviewing, testing and queuing such
patches on a Git branch.

Finally a minor nit: Please add a space between ) and {.
scripts/checkpatch.pl may help you find such style issues.
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

Looking forward to seeing the m68k target improved.

Regards,
Andreas

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