| Message ID | 1409320338-63098-2-git-send-email-jfrei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 29.08.14 15:52, Jens Freimann wrote: > From: David Hildenbrand <dahi@linux.vnet.ibm.com> > > When reading/writing the psw mask, the condition code may only be touched if > running on tcg. Why? Shouldn't we be able to set CC from gdb as well? Alex
On 02/09/14 00:39, Alexander Graf wrote: > > > On 29.08.14 15:52, Jens Freimann wrote: >> From: David Hildenbrand <dahi@linux.vnet.ibm.com> >> >> When reading/writing the psw mask, the condition code may only be touched if >> running on tcg. > > Why? Shouldn't we be able to set CC from gdb as well? > You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb: The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back. The symptom was that the cc was always shown as zero. Christian
On 02.09.14 09:07, Christian Borntraeger wrote: > On 02/09/14 00:39, Alexander Graf wrote: >> >> >> On 29.08.14 15:52, Jens Freimann wrote: >>> From: David Hildenbrand <dahi@linux.vnet.ibm.com> >>> >>> When reading/writing the psw mask, the condition code may only be touched if >>> running on tcg. >> >> Why? Shouldn't we be able to set CC from gdb as well? >> > > You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb: > The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back. > > The symptom was that the cc was always shown as zero. Ah, I see. I agree with the patch, but the patch description does not actually describe what the patch does. Please rework it. I also wouldn't mind if instead of hard coding this logic in the gdbstub, we'd extract it as helper functions to read and write the PSW.MASK in cpu.h. Alex
On 03/09/14 11:27, Alexander Graf wrote: > > > On 02.09.14 09:07, Christian Borntraeger wrote: >> On 02/09/14 00:39, Alexander Graf wrote: >>> >>> >>> On 29.08.14 15:52, Jens Freimann wrote: >>>> From: David Hildenbrand <dahi@linux.vnet.ibm.com> >>>> >>>> When reading/writing the psw mask, the condition code may only be touched if >>>> running on tcg. >>> >>> Why? Shouldn't we be able to set CC from gdb as well? >>> >> >> You can. What this patch does (and the patch description is a bit vague here) is to not modify the PSW it gets from KVM when passing it to gdb: >> The qemu core gets the PSW from KVM. Without this patch, we use cc_op to calculate the current CC of the PSW (No idea of TCG, I guess its evaluated lazy - at least this is how valgrind works). This is wrong for the KVM case, as cc_op does not contain any useful data for the KVM case. With this patch we simply pass the psw from KVM to gdb and back. >> >> The symptom was that the cc was always shown as zero. > > Ah, I see. I agree with the patch, but the patch description does not > actually describe what the patch does. Please rework it. The patch was already pulled...Well the description is misleaded but with some interpretion still correct ;-). But your point is well taken. We will try to make sure that all patch description are descriptive and not ambiguous. Your question is a good indication that this was not the case in this patch. Christian > I also wouldn't mind if instead of hard coding this logic in the > gdbstub, we'd extract it as helper functions to read and write the > PSW.MASK in cpu.h.
diff --git a/target-s390x/gdbstub.c b/target-s390x/gdbstub.c index a129742..8d55006 100644 --- a/target-s390x/gdbstub.c +++ b/target-s390x/gdbstub.c @@ -31,9 +31,13 @@ int s390_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) switch (n) { case S390_PSWM_REGNUM: - cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst, env->cc_vr); - val = deposit64(env->psw.mask, 44, 2, cc_op); - return gdb_get_regl(mem_buf, val); + if (tcg_enabled()) { + cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst, + env->cc_vr); + val = deposit64(env->psw.mask, 44, 2, cc_op); + return gdb_get_regl(mem_buf, val); + } + return gdb_get_regl(mem_buf, env->psw.mask); case S390_PSWA_REGNUM: return gdb_get_regl(mem_buf, env->psw.addr); case S390_R0_REGNUM ... S390_R15_REGNUM: @@ -62,7 +66,9 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) switch (n) { case S390_PSWM_REGNUM: env->psw.mask = tmpl; - env->cc_op = extract64(tmpl, 44, 2); + if (tcg_enabled()) { + env->cc_op = extract64(tmpl, 44, 2); + } break; case S390_PSWA_REGNUM: env->psw.addr = tmpl;