Re: [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t

["Alex Bennée" <alex.bennee@linaro.org>]

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);
>>   }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro