[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/i386: expose more MSRs to GDB
|
From: |
Alex Bennée |
|
Subject: |
Re: [PATCH] target/i386: expose more MSRs to GDB |
|
Date: |
Mon, 08 Feb 2021 12:12:39 +0000 |
|
User-agent: |
mu4e 1.5.7; emacs 28.0.50 |
Dominik Glöß <dominik.gloess@tum.de> writes:
> This patch adds 7 more model-specific registers to be usable while remote
> debugging in gdb. Accessing these registers can for example be useful for
> tracing Linux Systemcalls.
>
> Signed-off-by: Dominik Glöß <dominik.gloess@tum.de>
> ---
>
> Adding registers to GDB like this works fine for now. Should there
> arise the need to add more MSRs, a rework of the code that reads
> the xml file should be considered. Hard coding the amount of registers and
> matching the offsets in gdbstub and the xml seems prone to error.
There are a number of places where the register state of the guest could
be read:
- gdbstub
- hmp (info registers) - does QMP also expose this?
- logging (-d cpu)
and eventually
- tcg plugins
so in the medium term I think it would make sense to have a generic
register facility that front-ends could register their registers with
which could then handle the various bits of glue to the various
consumers of register data. For example we could generate the gdb XML in
a similar manner to the current arm_register_sysreg_for_xml code.
I doubt the x86 front-end will see anything like the extensive handling
of the current ARMCPRegInfo code but having data driven machine
descriptions is certainly handy when you have to deal with multiple
architecture variants. Perhaps there is some scope for some logic
sharing or maybe those mechanisms will always be intimately tied to the
implementation details of the front-end?
This is all a rather long winded way of saying it looks ok to me so if
the arch maintainers are happy I am:
Acked-by: Alex Bennée <alex.bennee@linaro.org>
>
> This is similar to the patch by Elias Djossou to allow outputting the same
> registers via HMP. Both patches are however independent from each other.
>
> gdb-xml/i386-32bit.xml | 7 +++
> gdb-xml/i386-64bit.xml | 7 +++
> target/i386/cpu.c | 4 +-
> target/i386/gdbstub.c | 122 ++++++++++++++++++++++++++++++++++++-----
> 4 files changed, 125 insertions(+), 15 deletions(-)
>
> diff --git a/gdb-xml/i386-32bit.xml b/gdb-xml/i386-32bit.xml
> index 872fcea9c2..0e650c9027 100644
> --- a/gdb-xml/i386-32bit.xml
> +++ b/gdb-xml/i386-32bit.xml
> @@ -61,6 +61,13 @@
> <reg name="es_base" bitsize="32" type="int32"/-->
> <reg name="fs_base" bitsize="32" type="int32"/>
> <reg name="gs_base" bitsize="32" type="int32"/>
> + <reg name="sysenter_cs" bitsize="32" type="int32"/>
> + <reg name="sysenter_esp" bitsize="32" type="int32"/>
> + <reg name="sysenter_eip" bitsize="32" type="int32"/>
> + <reg name="star" bitsize="32" type="int32"/>
> + <reg name="lstar" bitsize="32" type="int32"/>
> + <reg name="cstar" bitsize="32" type="int32"/>
> + <reg name="fmask" bitsize="32" type="int32"/>
> <reg name="k_gs_base" bitsize="32" type="int32"/>
>
> <flags id="i386_cr0" size="4">
> diff --git a/gdb-xml/i386-64bit.xml b/gdb-xml/i386-64bit.xml
> index 6d88969211..d7ca2d8586 100644
> --- a/gdb-xml/i386-64bit.xml
> +++ b/gdb-xml/i386-64bit.xml
> @@ -74,6 +74,13 @@
> <reg name="es_base" bitsize="64" type="int64"/-->
> <reg name="fs_base" bitsize="64" type="int64"/>
> <reg name="gs_base" bitsize="64" type="int64"/>
> + <reg name="sysenter_cs" bitsize="64" type="int64"/>
> + <reg name="sysenter_esp" bitsize="64" type="int64"/>
> + <reg name="sysenter_eip" bitsize="64" type="int64"/>
> + <reg name="star" bitsize="64" type="int64"/>
> + <reg name="lstar" bitsize="64" type="int64"/>
> + <reg name="cstar" bitsize="64" type="int64"/>
> + <reg name="fmask" bitsize="64" type="int64"/>
> <reg name="k_gs_base" bitsize="64" type="int64"/>
>
> <!-- Control registers -->
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ae89024d36..2b7be1c248 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7321,10 +7321,10 @@ static void x86_cpu_common_class_init(ObjectClass
> *oc, void *data)
> cc->gdb_arch_name = x86_gdb_arch_name;
> #ifdef TARGET_X86_64
> cc->gdb_core_xml_file = "i386-64bit.xml";
> - cc->gdb_num_core_regs = 66;
> + cc->gdb_num_core_regs = 73;
> #else
> cc->gdb_core_xml_file = "i386-32bit.xml";
> - cc->gdb_num_core_regs = 50;
> + cc->gdb_num_core_regs = 57;
> #endif
> cc->disas_set_info = x86_disas_set_info;
>
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index 41e265fc67..5743ba39b3 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -46,7 +46,8 @@ static const int gpr_map32[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };
> */
> #define IDX_NB_IP 1
> #define IDX_NB_FLAGS 1
> -#define IDX_NB_SEG (6 + 3)
> +#define IDX_NB_SEG 6
> +#define IDX_NB_MSR 10
> #define IDX_NB_CTL 6
> #define IDX_NB_FP 16
> /*
> @@ -54,13 +55,14 @@ static const int gpr_map32[8] = { 0, 1, 2, 3, 4, 5, 6, 7
> };
> */
> #define IDX_NB_MXCSR 1
> /*
> - * total ----> 8+1+1+9+6+16+8+1=50 or 16+1+1+9+6+16+16+1=66
> + * total ----> 8+1+1+6+10+6+16+8+1=57 or 16+1+1+6+10+6+16+16+1=73
> */
>
> #define IDX_IP_REG CPU_NB_REGS
> #define IDX_FLAGS_REG (IDX_IP_REG + IDX_NB_IP)
> #define IDX_SEG_REGS (IDX_FLAGS_REG + IDX_NB_FLAGS)
> -#define IDX_CTL_REGS (IDX_SEG_REGS + IDX_NB_SEG)
> +#define IDX_MSR_REGS (IDX_SEG_REGS + IDX_NB_SEG)
> +#define IDX_CTL_REGS (IDX_MSR_REGS + IDX_NB_MSR)
> #define IDX_FP_REGS (IDX_CTL_REGS + IDX_NB_CTL)
> #define IDX_XMM_REGS (IDX_FP_REGS + IDX_NB_FP)
> #define IDX_MXCSR_REG (IDX_XMM_REGS + CPU_NB_REGS)
> @@ -143,25 +145,56 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray
> *mem_buf, int n)
> case IDX_SEG_REGS + 5:
> return gdb_get_reg32(mem_buf, env->segs[R_GS].selector);
>
> - case IDX_SEG_REGS + 6:
> + case IDX_MSR_REGS:
> if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> return gdb_get_reg64(mem_buf, env->segs[R_FS].base);
> }
> return gdb_get_reg32(mem_buf, env->segs[R_FS].base);
>
> - case IDX_SEG_REGS + 7:
> + case IDX_MSR_REGS + 1:
> if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> return gdb_get_reg64(mem_buf, env->segs[R_GS].base);
> }
> return gdb_get_reg32(mem_buf, env->segs[R_GS].base);
>
> - case IDX_SEG_REGS + 8:
> -#ifdef TARGET_X86_64
> + case IDX_MSR_REGS + 2:
> + if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> + return gdb_get_reg64(mem_buf, env->sysenter_cs);
> + }
> + return gdb_get_reg32(mem_buf, env->sysenter_cs);
> +
> + case IDX_MSR_REGS + 3:
> + if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> + return gdb_get_reg64(mem_buf, env->sysenter_esp);
> + }
> + return gdb_get_reg32(mem_buf, env->sysenter_esp);
> +
> + case IDX_MSR_REGS + 4:
> if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> - return gdb_get_reg64(mem_buf, env->kernelgsbase);
> + return gdb_get_reg64(mem_buf, env->sysenter_eip);
> }
> - return gdb_get_reg32(mem_buf, env->kernelgsbase);
> + return gdb_get_reg32(mem_buf, env->sysenter_eip);
> +
> + case IDX_MSR_REGS + 5:
> + if ((env->hflags & HF_CS64_MASK) || GDB_FORCE_64) {
> + return gdb_get_reg64(mem_buf, env->star);
> + }
> + return gdb_get_reg32(mem_buf, env->star);
> +
> +#ifdef TARGET_X86_64
> + case IDX_MSR_REGS + 6:
> + return gdb_get_reg64(mem_buf, env->fmask);
> + case IDX_MSR_REGS + 7:
> + return gdb_get_reg64(mem_buf, env->lstar);
> + case IDX_MSR_REGS + 8:
> + return gdb_get_reg64(mem_buf, env->cstar);
> + case IDX_MSR_REGS + 9:
> + return gdb_get_reg64(mem_buf, env->kernelgsbase);
> #else
> + case IDX_MSR_REGS + 6:
> + case IDX_MSR_REGS + 7:
> + case IDX_MSR_REGS + 8:
> + case IDX_MSR_REGS + 9:
> return gdb_get_reg32(mem_buf, 0);
> #endif
>
> @@ -330,7 +363,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> case IDX_SEG_REGS + 5:
> return x86_cpu_gdb_load_seg(cpu, R_GS, mem_buf);
>
> - case IDX_SEG_REGS + 6:
> + case IDX_MSR_REGS:
> if (env->hflags & HF_CS64_MASK) {
> env->segs[R_FS].base = ldq_p(mem_buf);
> return 8;
> @@ -338,7 +371,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> env->segs[R_FS].base = ldl_p(mem_buf);
> return 4;
>
> - case IDX_SEG_REGS + 7:
> + case IDX_MSR_REGS + 1:
> if (env->hflags & HF_CS64_MASK) {
> env->segs[R_GS].base = ldq_p(mem_buf);
> return 8;
> @@ -346,16 +379,79 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> env->segs[R_GS].base = ldl_p(mem_buf);
> return 4;
>
> - case IDX_SEG_REGS + 8:
> + case IDX_MSR_REGS + 2:
> + if (env->hflags & HF_CS64_MASK) {
> + env->sysenter_cs = ldq_p(mem_buf);
> + return 8;
> + }
> + env->sysenter_cs = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 3:
> + if (env->hflags & HF_CS64_MASK) {
> + env->sysenter_esp = ldq_p(mem_buf);
> + return 8;
> + }
> + env->sysenter_esp = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 4:
> + if (env->hflags & HF_CS64_MASK) {
> + env->sysenter_eip = ldq_p(mem_buf);
> + return 8;
> + }
> + env->sysenter_eip = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 5:
> + if (env->hflags & HF_CS64_MASK) {
> + env->star = ldq_p(mem_buf);
> + return 8;
> + }
> + env->star = ldl_p(mem_buf);
> + return 4;
> +
> #ifdef TARGET_X86_64
> + case IDX_MSR_REGS + 6:
> + if (env->hflags & HF_CS64_MASK) {
> + env->lstar = ldq_p(mem_buf);
> + return 8;
> + }
> + env->lstar = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 7:
> + if (env->hflags & HF_CS64_MASK) {
> + env->cstar = ldq_p(mem_buf);
> + return 8;
> + }
> + env->cstar = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 8:
> + if (env->hflags & HF_CS64_MASK) {
> + env->fmask = ldq_p(mem_buf);
> + return 8;
> + }
> + env->fmask = ldl_p(mem_buf);
> + return 4;
> +
> + case IDX_MSR_REGS + 9:
> if (env->hflags & HF_CS64_MASK) {
> env->kernelgsbase = ldq_p(mem_buf);
> return 8;
> }
> env->kernelgsbase = ldl_p(mem_buf);
> -#endif
> return 4;
> +#else
> + case IDX_MSR_REGS + 6:
> + case IDX_MSR_REGS + 7:
> + case IDX_MSR_REGS + 8:
> + case IDX_MSR_REGS + 9:
> + return 4;
> +#endif
>
> + /* The first 8 registers have been addressed in an if block above */
> case IDX_FP_REGS + 8:
> cpu_set_fpuc(env, ldl_p(mem_buf));
> return 4;
--
Alex Bennée