From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L0j1Z-0003v9-AV for qemu-devel@nongnu.org; Thu, 13 Nov 2008 15:42:45 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L0j1Y-0003uk-9r for qemu-devel@nongnu.org; Thu, 13 Nov 2008 15:42:44 -0500 Received: from [199.232.76.173] (port=60271 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L0j1Y-0003uf-1E for qemu-devel@nongnu.org; Thu, 13 Nov 2008 15:42:44 -0500 Received: from qw-out-1920.google.com ([74.125.92.146]:37652) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L0j1X-000523-L5 for qemu-devel@nongnu.org; Thu, 13 Nov 2008 15:42:43 -0500 Received: by qw-out-1920.google.com with SMTP id 5so840374qwc.4 for ; Thu, 13 Nov 2008 12:42:42 -0800 (PST) Message-ID: <491C913F.7060508@codemonkey.ws> Date: Thu, 13 Nov 2008 14:42:39 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling References: <20081103103558.213902776@mchn012c.ww002.siemens.net> <20081103103558.523866919@mchn012c.ww002.siemens.net> In-Reply-To: <20081103103558.523866919@mchn012c.ww002.siemens.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Jan Kiszka Jan Kiszka wrote: > This patch refactors the way the CPU state is handled that is associated > with a TB. The basic motivation is to move more arch specific code out > of generic files. Specifically the long #ifdef clutter in tb_find_fast() > has to be overcome in order to avoid duplicating it for the gdb > watchpoint fixes (patch "Restore pc on watchpoint hits"). > > The approach taken here is to encapsulate the relevant CPU state in a > new structure called TBCPUState which is kept inside TranslationBlock > but also passed around when setting up new TBs. To fill a TBCPUState > based on the current CPUState, each arch has to provide > cpu_get_tb_cpu_state(CPUState *, TBCPUState *). > > At this chance, the patch also converts the not really beautiful macro > CPU_PC_FROM_TB into a clean static inline function. > There are really three patches in one here. The first patch converts CPU_PC_FROM_TB to a function. The second eliminates the #ifdef mess in tb_find_fast by introducing cpu_get_tb_cpu_state(CPUState *, TranslationBlock *), and the third moves the CPU specific state in Translation block to a separate structure and changes the signature of cpu_get_tb_cpu_state() to take the TBCPUState structure. Can you split up this patch along these lines? All of these changes are mechanical and they are easier to review/bisect as separate patches. I'm not 100% convinced that the third patch is really that valuable. Can you explain if there's any benefit to doing this other than readability? Regards, Anthony Liguori