Re: [Qemu-devel] Initial commit of AVR cores

[Peter Maydell <peter.maydell@linaro.org>]

On 29 May 2016 at 09:57, Michael Rolnik <mrolnik@gmail.com> wrote:
> 1. Initial commit of AVR 8bit cores support
> 2. all instruction, except BREAK/DES/SPM/SPMX, are implemented
> 3. not fully tested yet, however I was able to execute simple code with
> functions. e.g fibonacci calculation
> 4. the current patch includes a non real, sample board
> 5. this patch does not include any peripheral devices.
> 6. no fuses support yet. PC is set to 0 at reset.

Hi; thanks for this contribution to QEMU. Unfortunately as
it is it isn't really reviewable. If you could take a look
at our documentation on how to submit patches and follow
those recommendations that would be very helpful:
http://wiki.qemu.org/Contribute/SubmitAPatch

In particular, you need to split this single huge
patch up into multiple separate patches which each
do one thing and which aren't too large, and you should
then send those as a set of patch emails, rather than
an email with a patch in an attachment. You should
also make sure there aren't any unnecessary changes
in there too (for instance, what are your changes to
gdbstub.c for?). You might find it helpful to look back
through the qemu-devel archives for where other people
added new-architecture support to see how they
structured their patchsets and what kinds of issues
were raised on code review that might apply to your
patches too.

As a random thing I noticed just scrolling through your
patch, you'll also want to change all these unions:

+typedef union avr_opcode_LDI_1110aaaabbbbcccc_u {
+    uint16_t    opcode;
+    struct { uint16_t    lImm:4;
+        uint16_t    Rd:4;
+        uint16_t    hImm:4;
+        uint16_t:4; /* 1110 */
+    };
+} avr_opcode_LDI_1110aaaabbbbcccc_t;

to something else -- bitfield layouts aren't portable
(there's no guarantees about endianness, for instance).
Most targets use the bitops.h functions like extract32().

thanks
-- PMM