From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b706a-0001Ba-8f for qemu-devel@nongnu.org; Sun, 29 May 2016 08:46:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b706X-0006ed-Ur for qemu-devel@nongnu.org; Sun, 29 May 2016 08:46:07 -0400 Received: from mail-vk0-x232.google.com ([2607:f8b0:400c:c05::232]:34436) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b706X-0006eT-7w for qemu-devel@nongnu.org; Sun, 29 May 2016 08:46:05 -0400 Received: by mail-vk0-x232.google.com with SMTP id c189so194333361vkb.1 for ; Sun, 29 May 2016 05:46:04 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: Peter Maydell Date: Sun, 29 May 2016 13:45:44 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] Initial commit of AVR cores List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Rolnik Cc: QEMU Developers On 29 May 2016 at 09:57, Michael Rolnik 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