Re: [PATCH v3 2/5] plugins: add read-only property for registers

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

Alex Bennée <alex.bennee@linaro.org> writes:

> Florian Hofhammer <florian.hofhammer@epfl.ch> writes:
>
>> Some registers should be marked as read-only from a plugin API
>> perspective, as writing to them via qemu_plugin_write_register has no
>> effect. This includes the program counter, and we expose this fact to
>> the plugins with this patch.
>>
>> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
>> ---
>>  include/qemu/qemu-plugin.h |  2 ++
>>  plugins/api.c              | 17 +++++++++++++++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index f976b62030..1f25fb2b40 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -942,11 +942,13 @@ struct qemu_plugin_register;
>>   *          writing value with qemu_plugin_write_register
>>   * @name: register name
>>   * @feature: optional feature descriptor, can be NULL
>> + * @is_readonly: true if the register cannot be written via qemu_plugin_write_register
>>   */
>>  typedef struct {
>>      struct qemu_plugin_register *handle;
>>      const char *name;
>>      const char *feature;
>> +    bool is_readonly;
>>  } qemu_plugin_reg_descriptor;
>>  
>>  /**
>> diff --git a/plugins/api.c b/plugins/api.c
>> index fc19bdb40b..de8c32db50 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -403,6 +403,12 @@ bool qemu_plugin_bool_parse(const char *name, const char *value, bool *ret)
>>   * ancillary data the plugin might find useful.
>>   */
>>  
>> +static const char pc_str[] = "pc"; // generic name for program counter
>> +static const char eip_str[] = "eip"; // x86 specific name for program counter
>> +static const char rip_str[] = "rip"; // x86_64 specific name for program counter
>> +static const char pswa_str[] = "pswa"; // s390x specific name for program counter
>> +static const char iaoq_str[] = "iaoq"; // HP/PA specific name for program counter
>> +static const char rpc_str[] = "rpc"; // microblaze specific name for
>> program counter
>
> It's ugly but I can't think of anything better as you say in the commit message.
>
>>  static GArray *create_register_handles(GArray *gdbstub_regs)
>>  {
>>      GArray *find_data = g_array_new(true, true,
>> @@ -417,9 +423,20 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
>>              continue;
>>          }
>>  
>> +        gint plugin_ro_bit = 0;
>>          /* Create a record for the plugin */
>>          desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1);
>>          desc.name = g_intern_string(grd->name);
>
> Lets just set desc.is_readonly to false here.
>
>> +        if (!strcmp(desc.name, pc_str)
>> +            || !strcmp(desc.name, eip_str)
>> +            || !strcmp(desc.name, rip_str)
>> +            || !strcmp(desc.name, pswa_str)
>> +            || !strcmp(desc.name, iaoq_str)
>> +            || !strcmp(desc.name, rpc_str)
>> +           ) {
>> +            plugin_ro_bit = 1;
>> +        }
>> +        desc.is_readonly = plugin_ro_bit == 1 ? true : false;
>
> And fold setting it to true into the if statement. I have a marginal
> preference for g_strcmp0(desc.name, eip_str) == 0 style tests.

The option of course would be to skip the register all together. Do we
have code which will have multiple vaddr's for the same TB where using
the TB data wouldn't be sufficient?

>
>>          desc.feature = g_intern_string(grd->feature_name);
>>          g_array_append_val(find_data, desc);
>>      }
>
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro