| Message ID | 20231003183058.1639121-14-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | TCG code quality tracking | expand |
On 3/10/23 20:30, Richard Henderson wrote: > From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com> > > Introduce a MonitorDisasSpace to replace the current is_physical > boolean argument to monitor_disas. Generate an error if we attempt > to read past the end of a single RAMBlock. > > Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Signed-off-by: Fei Wu <fei2.wu@intel.com> > [rth: Split out of a larger patch; validate the RAMBlock size] > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/disas/disas.h | 8 +++++++- > disas/disas-mon.c | 32 ++++++++++++++++++++++++++++++-- > monitor/hmp-cmds-target.c | 27 ++++++++++++++++----------- > 3 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/include/disas/disas.h b/include/disas/disas.h > index 176775eff7..cd99b0ccd0 100644 > --- a/include/disas/disas.h > +++ b/include/disas/disas.h > @@ -5,8 +5,14 @@ > void disas(FILE *out, const void *code, size_t size); > void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size); > > +typedef enum { > + MON_DISAS_GVA, /* virtual */ > + MON_DISAS_GPA, /* physical */ > + MON_DISAS_GRA, /* ram_addr_t */ > +} MonitorDisasSpace; Obviously I'd rather see MonitorDisasSpace = {MON_DISAS_GVA/GPA} introduced in a preliminary patch, but just saying. > static void memory_dump(Monitor *mon, int count, int format, int wsize, > - hwaddr addr, int is_physical) > + hwaddr addr, MonitorDisasSpace space) > { > int l, line_size, i, max_digits, len; > uint8_t buf[16]; > uint64_t v; > CPUState *cs = mon_get_cpu(mon); > > - if (!cs && (format == 'i' || !is_physical)) { Why is the '!cs' check removed? Otherwise LGTM. > + if (space == MON_DISAS_GVA || format == 'i') { > monitor_printf(mon, "Can not dump without CPU\n"); > return; > } > > if (format == 'i') { > - monitor_disas(mon, cs, addr, count, is_physical); > - return; > + monitor_disas(mon, cs, addr, count, space); > } > > len = wsize * count; > @@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize, > } > > while (len > 0) { > - if (is_physical) { > - monitor_printf(mon, HWADDR_FMT_plx ":", addr); > - } else { > + switch (space) { > + case MON_DISAS_GVA: > monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr); > + break; > + case MON_DISAS_GPA: > + monitor_printf(mon, HWADDR_FMT_plx ":", addr); > + break; > + default: > + g_assert_not_reached(); > } > l = len; > - if (l > line_size) > + if (l > line_size) { > l = line_size; > - if (is_physical) { > + } > + if (space == MON_DISAS_GPA) { > AddressSpace *as = cs ? cs->as : &address_space_memory; > MemTxResult r = address_space_read(as, addr, > MEMTXATTRS_UNSPECIFIED, buf, l); > @@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict) > int size = qdict_get_int(qdict, "size"); > target_long addr = qdict_get_int(qdict, "addr"); > > - memory_dump(mon, count, format, size, addr, 0); > + memory_dump(mon, count, format, size, addr, MON_DISAS_GVA); > }
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 3/10/23 20:30, Richard Henderson wrote: >> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com> >> Introduce a MonitorDisasSpace to replace the current is_physical >> boolean argument to monitor_disas. Generate an error if we attempt >> to read past the end of a single RAMBlock. >> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Signed-off-by: Fei Wu <fei2.wu@intel.com> >> [rth: Split out of a larger patch; validate the RAMBlock size] >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/disas/disas.h | 8 +++++++- >> disas/disas-mon.c | 32 ++++++++++++++++++++++++++++++-- >> monitor/hmp-cmds-target.c | 27 ++++++++++++++++----------- >> 3 files changed, 53 insertions(+), 14 deletions(-) >> diff --git a/include/disas/disas.h b/include/disas/disas.h >> index 176775eff7..cd99b0ccd0 100644 >> --- a/include/disas/disas.h >> +++ b/include/disas/disas.h >> @@ -5,8 +5,14 @@ >> void disas(FILE *out, const void *code, size_t size); >> void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size); >> +typedef enum { >> + MON_DISAS_GVA, /* virtual */ >> + MON_DISAS_GPA, /* physical */ >> + MON_DISAS_GRA, /* ram_addr_t */ >> +} MonitorDisasSpace; > > > Obviously I'd rather see MonitorDisasSpace = {MON_DISAS_GVA/GPA} > introduced in a preliminary patch, but just saying. > >> static void memory_dump(Monitor *mon, int count, int format, int wsize, >> - hwaddr addr, int is_physical) >> + hwaddr addr, MonitorDisasSpace space) >> { >> int l, line_size, i, max_digits, len; >> uint8_t buf[16]; >> uint64_t v; >> CPUState *cs = mon_get_cpu(mon); >> - if (!cs && (format == 'i' || !is_physical)) { > > Why is the '!cs' check removed? Otherwise LGTM. Yeah it breaks the monitor I think with should be: if (!cs && (space == MON_DISAS_GVA || format == 'i')) > >> + if (space == MON_DISAS_GVA || format == 'i') { >> monitor_printf(mon, "Can not dump without CPU\n"); >> return; >> } >> if (format == 'i') { >> - monitor_disas(mon, cs, addr, count, is_physical); >> - return; >> + monitor_disas(mon, cs, addr, count, space); >> } >> len = wsize * count; >> @@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize, >> } >> while (len > 0) { >> - if (is_physical) { >> - monitor_printf(mon, HWADDR_FMT_plx ":", addr); >> - } else { >> + switch (space) { >> + case MON_DISAS_GVA: >> monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr); >> + break; >> + case MON_DISAS_GPA: >> + monitor_printf(mon, HWADDR_FMT_plx ":", addr); >> + break; >> + default: >> + g_assert_not_reached(); >> } >> l = len; >> - if (l > line_size) >> + if (l > line_size) { >> l = line_size; >> - if (is_physical) { >> + } >> + if (space == MON_DISAS_GPA) { >> AddressSpace *as = cs ? cs->as : &address_space_memory; >> MemTxResult r = address_space_read(as, addr, >> MEMTXATTRS_UNSPECIFIED, buf, l); >> @@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict) >> int size = qdict_get_int(qdict, "size"); >> target_long addr = qdict_get_int(qdict, "addr"); >> - memory_dump(mon, count, format, size, addr, 0); >> + memory_dump(mon, count, format, size, addr, MON_DISAS_GVA); >> }
diff --git a/include/disas/disas.h b/include/disas/disas.h index 176775eff7..cd99b0ccd0 100644 --- a/include/disas/disas.h +++ b/include/disas/disas.h @@ -5,8 +5,14 @@ void disas(FILE *out, const void *code, size_t size); void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size); +typedef enum { + MON_DISAS_GVA, /* virtual */ + MON_DISAS_GPA, /* physical */ + MON_DISAS_GRA, /* ram_addr_t */ +} MonitorDisasSpace; + void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc, - int nb_insn, bool is_physical); + int nb_insn, MonitorDisasSpace space); char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size); diff --git a/disas/disas-mon.c b/disas/disas-mon.c index 48ac492c6c..d486f64c92 100644 --- a/disas/disas-mon.c +++ b/disas/disas-mon.c @@ -23,9 +23,27 @@ physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length, return res == MEMTX_OK ? 0 : EIO; } +static int +ram_addr_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length, + struct disassemble_info *info) +{ + hwaddr hw_length; + void *p; + + RCU_READ_LOCK_GUARD(); + + hw_length = length; + p = qemu_ram_ptr_length(NULL, memaddr, &hw_length, false); + if (hw_length < length) { + return EIO; + } + memcpy(myaddr, p, length); + return 0; +} + /* Disassembler for the monitor. */ void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc, - int nb_insn, bool is_physical) + int nb_insn, MonitorDisasSpace space) { int count, i; CPUDebug s; @@ -35,8 +53,18 @@ void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc, s.info.fprintf_func = disas_gstring_printf; s.info.stream = (FILE *)ds; /* abuse this slot */ - if (is_physical) { + switch (space) { + case MON_DISAS_GVA: + /* target_read_memory set in disas_initialize_debug_target */ + break; + case MON_DISAS_GPA: s.info.read_memory_func = physical_read_memory; + break; + case MON_DISAS_GRA: + s.info.read_memory_func = ram_addr_read_memory; + break; + default: + g_assert_not_reached(); } s.info.buffer_vma = pc; diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c index d9fbcac08d..476cf68e81 100644 --- a/monitor/hmp-cmds-target.c +++ b/monitor/hmp-cmds-target.c @@ -120,21 +120,20 @@ void hmp_info_registers(Monitor *mon, const QDict *qdict) } static void memory_dump(Monitor *mon, int count, int format, int wsize, - hwaddr addr, int is_physical) + hwaddr addr, MonitorDisasSpace space) { int l, line_size, i, max_digits, len; uint8_t buf[16]; uint64_t v; CPUState *cs = mon_get_cpu(mon); - if (!cs && (format == 'i' || !is_physical)) { + if (space == MON_DISAS_GVA || format == 'i') { monitor_printf(mon, "Can not dump without CPU\n"); return; } if (format == 'i') { - monitor_disas(mon, cs, addr, count, is_physical); - return; + monitor_disas(mon, cs, addr, count, space); } len = wsize * count; @@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize, } while (len > 0) { - if (is_physical) { - monitor_printf(mon, HWADDR_FMT_plx ":", addr); - } else { + switch (space) { + case MON_DISAS_GVA: monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr); + break; + case MON_DISAS_GPA: + monitor_printf(mon, HWADDR_FMT_plx ":", addr); + break; + default: + g_assert_not_reached(); } l = len; - if (l > line_size) + if (l > line_size) { l = line_size; - if (is_physical) { + } + if (space == MON_DISAS_GPA) { AddressSpace *as = cs ? cs->as : &address_space_memory; MemTxResult r = address_space_read(as, addr, MEMTXATTRS_UNSPECIFIED, buf, l); @@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict) int size = qdict_get_int(qdict, "size"); target_long addr = qdict_get_int(qdict, "addr"); - memory_dump(mon, count, format, size, addr, 0); + memory_dump(mon, count, format, size, addr, MON_DISAS_GVA); } void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict) @@ -245,7 +250,7 @@ void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict) int size = qdict_get_int(qdict, "size"); hwaddr addr = qdict_get_int(qdict, "addr"); - memory_dump(mon, count, format, size, addr, 1); + memory_dump(mon, count, format, size, addr, MON_DISAS_GPA); } void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp)