From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ypgse-0005dD-Hf for qemu-devel@nongnu.org; Tue, 05 May 2015 13:43:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ypgsa-00025M-1j for qemu-devel@nongnu.org; Tue, 05 May 2015 13:43:40 -0400 Received: from mail-qg0-x22b.google.com ([2607:f8b0:400d:c04::22b]:33552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpgsZ-00025H-T2 for qemu-devel@nongnu.org; Tue, 05 May 2015 13:43:35 -0400 Received: by qgdy78 with SMTP id y78so85223815qgd.0 for ; Tue, 05 May 2015 10:43:35 -0700 (PDT) Sender: Richard Henderson Message-ID: <55490142.5030901@twiddle.net> Date: Tue, 05 May 2015 10:43:30 -0700 From: Richard Henderson MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/7] monitor: "i": Add ARM specifics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Peter Crosthwaite Cc: "claudio.fontana" , Peter Crosthwaite , QEMU Developers , Alexander Graf , Edgar Iglesias , "Edgar E. Iglesias" On 05/05/2015 10:19 AM, Peter Maydell wrote: > On 5 May 2015 at 05:45, Peter Crosthwaite wrote: >> Add the ARM specific disassembly flags setup, so ARM can be correctly >> disassembled from the monitor. >> >> Signed-off-by: Peter Crosthwaite >> --- >> monitor.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/monitor.c b/monitor.c >> index d831d98..9d9f1e2 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -1217,6 +1217,17 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize, >> int flags; >> flags = 0; >> env = mon_get_cpu(); >> +#ifdef TARGET_ARM >> + if (env->thumb) { >> + flags |= 1; >> + } >> + if (env->bswap_code) { >> + flags |= 2; >> + } >> + if (env->aarch64) { >> + flags |= 4; >> + } >> +#endif > > monitor.c has no business poking around in the CPU state > internals like this... You probably want a CPU method > get_disas_flags() or something. > > -- PMM > While this patch set does improve the current dismal state of affairs, I think the ideal solution is a cpu method that takes care of all the disassembly info setup. Indeed, the flags setup becomes less obscure when, instead of #ifdef TARGET_I386 if (wsize == 2) { flags = 1; } else if (wsize == 4) { flags = 0; } else { /* as default we use the current CS size */ flags = 0; if (env) { #ifdef TARGET_X86_64 if ((env->efer & MSR_EFER_LMA) && (env->segs[R_CS].flags & DESC_L_MASK)) flags = 2; else #endif if (!(env->segs[R_CS].flags & DESC_B_MASK)) flags = 1; } } in one place and #if defined(TARGET_I386) if (flags == 2) { s.info.mach = bfd_mach_x86_64; } else if (flags == 1) { s.info.mach = bfd_mach_i386_i8086; } else { s.info.mach = bfd_mach_i386_i386; } print_insn = print_insn_i386; in another, we merge the two so that we get s.info.mach = bfd_mach_i386_i8086; if (env->hflags & (1U << HF_CS32_SHIFT)) { s.info.mach = bfd_mach_i386_i386; } #ifdef TARGET_X86_64 if (env->hflags & (1U << HF_CS64_SHIFT)) { s.info.mach = bfd_mach_x86_64; } #endif I'm not sure what the right interface for this is, exactly. But I'd think that the CPUDebug structure would be initialized in the caller -- target or monitor -- while the mach and whatnot get filled in by the cpu hook. Maybe even have the cpu hook return the print_insn function to use. r~