From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2nzt-0005qm-TY for qemu-devel@nongnu.org; Wed, 19 Nov 2008 09:25:37 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2nzr-0005qJ-Uz for qemu-devel@nongnu.org; Wed, 19 Nov 2008 09:25:36 -0500 Received: from [199.232.76.173] (port=36604 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2nzr-0005qG-RC for qemu-devel@nongnu.org; Wed, 19 Nov 2008 09:25:35 -0500 Received: from lizzard.sbs.de ([194.138.37.39]:16630) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1L2nzr-00033C-5y for qemu-devel@nongnu.org; Wed, 19 Nov 2008 09:25:35 -0500 Received: from mail1.sbs.de (localhost [127.0.0.1]) by lizzard.sbs.de (8.12.11.20060308/8.12.11) with ESMTP id mAJEPWud025923 for ; Wed, 19 Nov 2008 15:25:33 +0100 Received: from [139.25.109.167] (mchn012c.ww002.siemens.net [139.25.109.167] (may be forged)) by mail1.sbs.de (8.12.11.20060308/8.12.11) with ESMTP id mAJEPWbW005060 for ; Wed, 19 Nov 2008 15:25:32 +0100 Message-ID: <492421AF.7060703@siemens.com> Date: Wed, 19 Nov 2008 15:24:47 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <20081117161857.26880.45423.stgit@mchn012c.ww002.siemens.net> <20081117161859.26880.26254.stgit@mchn012c.ww002.siemens.net> <4923314F.2090604@codemonkey.ws> <49234C85.2030701@web.de> In-Reply-To: <49234C85.2030701@web.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers 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 Jan Kiszka wrote: > Anthony Liguori wrote: >> Jan Kiszka wrote: >>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h >>> index eed1f62..b7c8a2f 100644 >>> --- a/target-i386/cpu.h >>> +++ b/target-i386/cpu.h >>> @@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s); >>> /* MSDOS compatibility mode FPU exception support */ >>> void cpu_set_ferr(CPUX86State *s); >>> >>> +static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2) >>> +{ >>> + unsigned int limit; >>> + limit = (e1 & 0xffff) | (e2 & 0x000f0000); >>> + if (e2 & DESC_G_MASK) >>> + limit = (limit << 12) | 0xfff; >>> + return limit; >>> +} >>> + >>> +static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2) >>> +{ >>> + return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000)); >>> +} >>> + >>> >> I like this patch but if you're going to export new x86 helper >> functions, please prefix them with cpu_x86. In this case, these helpers >> are awfully low level (they're taking a GDT entry split into two 32-bit >> words). I would rather see an interface that took a CPUState and a >> segment register index. In the very least, it won't be very obvious to >> anyone what this API expects to take so a comment would be required. > > That was a result of the decision process "quick feature enhancement or > also some more refactoring?". :) > > Will think out a new interface instead, leaving those two where they > came from. > [Now with new, shiny cpu_x86_get_descr_debug interface.] This allows to set segment registers via gdb also in system emulation mode. Basic sanity checks are applied and nothing is changed if they fail. But screwing up the target via this interface will never be complicated, so I avoided being too paranoid here. Signed-off-by: Jan Kiszka --- gdbstub.c | 48 +++++++++++++++++++++++++++++++----------------- target-i386/cpu.h | 4 ++++ target-i386/helper.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 17 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index fd4d5db..89efa3f 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -358,6 +358,31 @@ static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n) return 0; } +static int cpu_x86_gdb_load_seg(CPUState *env, int sreg, uint8_t *mem_buf) +{ + uint16_t selector = ldl_p(mem_buf); + + if (selector != env->segs[sreg].selector) { +#if defined(CONFIG_USER_ONLY) + cpu_x86_load_seg(env, sreg, selector); +#else + unsigned int limit, flags; + target_ulong base; + + if (!(env->cr[0] & CR0_PE_MASK) || (env->eflags & VM_MASK)) { + base = selector << 4; + limit = 0xffff; + flags = 0; + } else { + if (!cpu_x86_get_descr_debug(env, selector, &base, &limit, &flags)) + return 4; + } + cpu_x86_load_seg_cache(env, sreg, selector, base, limit, flags); +#endif + } + return 4; +} + static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n) { uint32_t tmp; @@ -385,23 +410,12 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n) env->eflags = ldl_p(mem_buf); return 4; -#if defined(CONFIG_USER_ONLY) -#define LOAD_SEG(index, sreg)\ - tmp = ldl_p(mem_buf);\ - if (tmp != env->segs[sreg].selector)\ - cpu_x86_load_seg(env, sreg, tmp);\ - return 4 -#else -/* FIXME: Honor segment registers. Needs to avoid raising an exception - when the selector is invalid. */ -#define LOAD_SEG(index, sreg) return 4 -#endif - case IDX_SEG_REGS: LOAD_SEG(10, R_CS); - case IDX_SEG_REGS + 1: LOAD_SEG(11, R_SS); - case IDX_SEG_REGS + 2: LOAD_SEG(12, R_DS); - case IDX_SEG_REGS + 3: LOAD_SEG(13, R_ES); - case IDX_SEG_REGS + 4: LOAD_SEG(14, R_FS); - case IDX_SEG_REGS + 5: LOAD_SEG(15, R_GS); + case IDX_SEG_REGS: return cpu_x86_gdb_load_seg(env, R_CS, mem_buf); + case IDX_SEG_REGS + 1: return cpu_x86_gdb_load_seg(env, R_SS, mem_buf); + case IDX_SEG_REGS + 2: return cpu_x86_gdb_load_seg(env, R_DS, mem_buf); + case IDX_SEG_REGS + 3: return cpu_x86_gdb_load_seg(env, R_ES, mem_buf); + case IDX_SEG_REGS + 4: return cpu_x86_gdb_load_seg(env, R_FS, mem_buf); + case IDX_SEG_REGS + 5: return cpu_x86_gdb_load_seg(env, R_GS, mem_buf); case IDX_FP_REGS + 8: env->fpuc = ldl_p(mem_buf); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index eed1f62..947c178 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -710,6 +710,10 @@ static inline void cpu_x86_load_seg_cache(CPUX86State *env, } } +int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, + target_ulong *base, unsigned int *limit, + unsigned int *flags); + /* wrapper, just in case memory mappings must be changed */ static inline void cpu_x86_set_cpl(CPUX86State *s, int cpl) { diff --git a/target-i386/helper.c b/target-i386/helper.c index 037540d..4194be9 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1614,6 +1614,36 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, } } + +int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector, + target_ulong *base, unsigned int *limit, + unsigned int *flags) +{ + SegmentCache *dt; + target_ulong ptr; + uint32_t e1, e2; + int index; + + if (selector & 0x4) + dt = &env->ldt; + else + dt = &env->gdt; + index = selector & ~7; + ptr = dt->base + index; + if ((index + 7) > dt->limit + || cpu_memory_rw_debug(env, ptr, (uint8_t *)&e1, sizeof(e1), 0) != 0 + || cpu_memory_rw_debug(env, ptr+4, (uint8_t *)&e2, sizeof(e2), 0) != 0) + return 0; + + *base = ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000)); + *limit = (e1 & 0xffff) | (e2 & 0x000f0000); + if (e2 & DESC_G_MASK) + *limit = (*limit << 12) | 0xfff; + *flags = e2; + + return 1; +} + CPUX86State *cpu_x86_init(const char *cpu_model) { CPUX86State *env;