From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60637) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4v0F-00033x-OZ for qemu-devel@nongnu.org; Tue, 16 Jun 2015 13:50:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4v0A-0002np-PE for qemu-devel@nongnu.org; Tue, 16 Jun 2015 13:50:27 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50170 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4v0A-0002mP-FK for qemu-devel@nongnu.org; Tue, 16 Jun 2015 13:50:22 -0400 Message-ID: <558061DC.8080206@suse.de> Date: Tue, 16 Jun 2015 19:50:20 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1434149163-16639-1-git-send-email-aurelien@aurel32.net> <1434149163-16639-16-git-send-email-aurelien@aurel32.net> <5580525F.3010001@suse.de> <20150616174429.GA7379@aurel32.net> In-Reply-To: <20150616174429.GA7379@aurel32.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 15/15] target-s390x: PER: add Breaking-Event-Address register List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: qemu-devel@nongnu.org, Richard Henderson On 16.06.15 19:44, Aurelien Jarno wrote: > On 2015-06-16 18:44, Alexander Graf wrote: >> On 06/13/15 00:46, Aurelien Jarno wrote: >>> This patch adds support for PER Breaking-Event-Address register. Like >>> real hardware, it save the current PSW address when the PSW address is >>> changed by an instruction. We have to take care of optimizations QEMU >>> does, a branch to the next instruction is still a branch. >>> >>> This register is copied to low core memory when a program exception >>> happens. >>> >>> Cc: Richard Henderson >>> Cc: Alexander Graf >>> Signed-off-by: Aurelien Jarno >>> --- >>> target-s390x/cpu.c | 6 ++++++ >>> target-s390x/cpu.h | 12 +++++++----- >>> target-s390x/helper.c | 1 + >>> target-s390x/translate.c | 29 +++++++++++++++++++++++------ >>> 4 files changed, 37 insertions(+), 11 deletions(-) >>> >>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c >>> index 67579e7..98d2081 100644 >>> --- a/target-s390x/cpu.c >>> +++ b/target-s390x/cpu.c >>> @@ -116,6 +116,9 @@ static void s390_cpu_initial_reset(CPUState *s) >>> env->cregs[0] = CR0_RESET; >>> env->cregs[14] = CR14_RESET; >>> + /* architectured initial value for Breaking-Event-Address register */ >>> + env->gbea = 1; >>> + >>> env->pfault_token = -1UL; >>> /* tininess for underflow is detected before rounding */ >>> @@ -145,6 +148,9 @@ static void s390_cpu_full_reset(CPUState *s) >>> env->cregs[0] = CR0_RESET; >>> env->cregs[14] = CR14_RESET; >>> + /* architectured initial value for Breaking-Event-Address register */ >>> + env->gbea = 1; >>> + >>> env->pfault_token = -1UL; >>> /* tininess for underflow is detected before rounding */ >>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h >>> index 61cc5b4..519cef9 100644 >>> --- a/target-s390x/cpu.h >>> +++ b/target-s390x/cpu.h >>> @@ -788,14 +788,16 @@ typedef struct LowCore >>> uint8_t pad5[0xf4-0xf0]; /* 0x0f0 */ >>> uint32_t external_damage_code; /* 0x0f4 */ >>> uint64_t failing_storage_address; /* 0x0f8 */ >>> - uint8_t pad6[0x120-0x100]; /* 0x100 */ >>> + uint8_t pad6[0x110-0x100]; /* 0x100 */ >>> + uint64_t per_breaking_event_addr; /* 0x110 */ >>> + uint8_t pad7[0x120-0x118]; /* 0x118 */ >>> PSW restart_old_psw; /* 0x120 */ >>> PSW external_old_psw; /* 0x130 */ >>> PSW svc_old_psw; /* 0x140 */ >>> PSW program_old_psw; /* 0x150 */ >>> PSW mcck_old_psw; /* 0x160 */ >>> PSW io_old_psw; /* 0x170 */ >>> - uint8_t pad7[0x1a0-0x180]; /* 0x180 */ >>> + uint8_t pad8[0x1a0-0x180]; /* 0x180 */ >>> PSW restart_new_psw; /* 0x1a0 */ >>> PSW external_new_psw; /* 0x1b0 */ >>> PSW svc_new_psw; /* 0x1c0 */ >>> @@ -813,10 +815,10 @@ typedef struct LowCore >>> uint64_t last_update_clock; /* 0x280 */ >>> uint64_t steal_clock; /* 0x288 */ >>> PSW return_mcck_psw; /* 0x290 */ >>> - uint8_t pad8[0xc00-0x2a0]; /* 0x2a0 */ >>> + uint8_t pad9[0xc00-0x2a0]; /* 0x2a0 */ >>> /* System info area */ >>> uint64_t save_area[16]; /* 0xc00 */ >>> - uint8_t pad9[0xd40-0xc80]; /* 0xc80 */ >>> + uint8_t pad10[0xd40-0xc80]; /* 0xc80 */ >>> uint64_t kernel_stack; /* 0xd40 */ >>> uint64_t thread_info; /* 0xd48 */ >>> uint64_t async_stack; /* 0xd50 */ >>> @@ -824,7 +826,7 @@ typedef struct LowCore >>> uint64_t user_asce; /* 0xd60 */ >>> uint64_t panic_stack; /* 0xd68 */ >>> uint64_t user_exec_asce; /* 0xd70 */ >>> - uint8_t pad10[0xdc0-0xd78]; /* 0xd78 */ >>> + uint8_t pad11[0xdc0-0xd78]; /* 0xd78 */ >>> /* SMP info area: defined by DJB */ >>> uint64_t clock_comparator; /* 0xdc0 */ >>> diff --git a/target-s390x/helper.c b/target-s390x/helper.c >>> index 615cccf..d887006 100644 >>> --- a/target-s390x/helper.c >>> +++ b/target-s390x/helper.c >>> @@ -293,6 +293,7 @@ static void do_program_interrupt(CPUS390XState *env) >>> lowcore->program_old_psw.addr = cpu_to_be64(env->psw.addr); >>> mask = be64_to_cpu(lowcore->program_new_psw.mask); >>> addr = be64_to_cpu(lowcore->program_new_psw.addr); >>> + lowcore->per_breaking_event_addr = cpu_to_be64(env->gbea); >>> cpu_unmap_lowcore(lowcore); >>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c >>> index 98e8224..2fde815 100644 >>> --- a/target-s390x/translate.c >>> +++ b/target-s390x/translate.c >>> @@ -150,6 +150,7 @@ void s390_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf, >>> static TCGv_i64 psw_addr; >>> static TCGv_i64 psw_mask; >>> +static TCGv_i64 gbea; >>> static TCGv_i32 cc_op; >>> static TCGv_i64 cc_src; >>> @@ -173,6 +174,9 @@ void s390x_translate_init(void) >>> psw_mask = tcg_global_mem_new_i64(TCG_AREG0, >>> offsetof(CPUS390XState, psw.mask), >>> "psw_mask"); >>> + gbea = tcg_global_mem_new_i64(TCG_AREG0, >>> + offsetof(CPUS390XState, gbea), >>> + "gbea"); >>> cc_op = tcg_global_mem_new_i32(TCG_AREG0, offsetof(CPUS390XState, cc_op), >>> "cc_op"); >>> @@ -252,14 +256,14 @@ static void update_psw_addr(DisasContext *s) >>> static void per_branch(DisasContext *s, bool to_next) >>> { >>> #ifndef CONFIG_USER_ONLY >>> + tcg_gen_movi_i64(gbea, s->pc); >> >> This should probably be a call to per_breaking_event(), no? > > Yes, that's possible, but given gbea is reused below instead of > reloading s->pc, I preferred to make the move more explicit. You're right, it's probably better to be explicit. > > That said given I have to send a rebased version, I can easily change > that. No worries. > >> Also, is there no flag to control this register? I'd assume it to be quite >> some performance penalty to always store the last branched register. > > No this register is always loaded. This is the value you see in dmesg > when a user program crashes, but you can also get it via GDB. Quite > useful in some cases. > > I haven't measured any performance impact, only noise. We are talking > about writing an immediate to a memory location in the env structure > (thus very likely with a cache line already allocated), so it's only > two host instructions more in a TB. That's not a lot given that for > example every TB starts by loading a value from the env structure and > doing a test on it. Ok, let's be correct first and then see what we can do if anyone complains about performance ;) Alex