From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRdRz-0001Q9-Gr for qemu-devel@nongnu.org; Tue, 08 May 2012 01:59:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SRdRx-0007Rh-JG for qemu-devel@nongnu.org; Tue, 08 May 2012 01:59:07 -0400 Received: from ozlabs.org ([203.10.76.45]:55462) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRdRx-0007NY-7J for qemu-devel@nongnu.org; Tue, 08 May 2012 01:59:05 -0400 From: Rusty Russell In-Reply-To: References: <1334497585-867-1-git-send-email-peter.maydell@linaro.org> <87ehqwe9pl.fsf@rustcorp.com.au> Date: Tue, 08 May 2012 15:27:29 +0930 Message-ID: <87y5p3b4fq.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 00/32] target-arm: refactor copro register implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, Paul Brook , Andreas =?utf-8?Q?F=C3=A4rber?= , patches@linaro.org (Accidentally made first reply to Peter only, fixed that now). On Mon, 7 May 2012 13:25:07 +0100, Peter Maydell = wrote: > On 7 May 2012 08:23, Rusty Russell wrote: > > OK, I reviewed the infrastructure, and it looks excellent. =C2=A0A few = of > > minor quibbles, which I only mention to show that I read it :) > > > > 1) cptype_valid, arm_current_pl and encoded_cp_matches_type could return > > =C2=A0 bool. > > > > 2) the access bits could be an enum type, which could be used in the > > =C2=A0 few places they're handled, for a bit more explicitness. >=20 > Mmm. This kind of thing is my old-school-C heritage showing through > :-) Maybe :) I was delighted by your use of a non-zero terminal value though, so I hardly noticed. > > 3) Perhaps an "assert(!g_hash_table_lookup(env->cp_regs, key));" before > > =C2=A0 the g_hash_table_insert, to avoid overlapping entries. >=20 > Overlapping entries are deliberately permitted (and used in some > cases). The idea is that last entry wins, so you can put in a > "whole region behaves like this" wildcard case and override it > with a few special cases. I feel nervous without flag on either the overridden or overriding one, to show it's deliberate. > > I then skimmed your epic refactoring, and wherever I stopped it looked > > completely sane. > > > > I wondered about an ARM_CP_DEPRECATED flag, which would print out a > > nasty "email the list" message if hit. =C2=A0Maybe it still won't provi= de > > enough confidence to tighten emulation, though. >=20 > Mmm. Really I would like qemu to have a better logging infrastructure > so we could classify things into "debug info/qemu bug/guest has done > something dubious" and let the user turn the logging level up or down. > In the absence of that I tend to just not put in tracing :-( Seems like YA infrastructure adventure we could embark upon. I'll add it to the list :) > The other thing on my todo list is that I don't think we're correctly > handling the hashtable on cpu_delete/cpu_copy. I don't pretend to understand the QEMU Object Model, but it seems like you're missing a level of indirection by putting the cp_regs hash into each CPUARMState directly. More logically each ARMCPU would have a pointer to its ARMCPUType, which would contain the cp_regs hash (and maybe other things). Cheers, Rusty.