diff mbox series

[v1,7/9] plugins: add API to return a name for a IO device

Message ID 20200602154624.4460-8-alex.bennee@linaro.org (mailing list archive)
State New, archived
Headers show
Series plugins/next (bug fixes, hwprofile, lockstep) | expand

Commit Message

Alex Bennée June 2, 2020, 3:46 p.m. UTC
This may well end up being anonymous but it should always be unique.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h |  5 +++++
 plugins/api.c              | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Clement Deschamps June 2, 2020, 4:06 p.m. UTC | #1
On 6/2/20 5:46 PM, Alex Bennée wrote:
> This may well end up being anonymous but it should always be unique.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/qemu/qemu-plugin.h |  5 +++++
>   plugins/api.c              | 18 ++++++++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3a..43c6a9e857f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>   bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>   uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>   
> +/*
> + * Returns a string representing the device. Plugin must free() it

s/free/g_free


> + */
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
> +
>   typedef void
>   (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>                                qemu_plugin_meminfo_t info, uint64_t vaddr,
> diff --git a/plugins/api.c b/plugins/api.c
> index bbdc5a4eb46..3c73de8c1c2 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>       return 0;
>   }
>   
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    if (haddr && haddr->is_io) {
> +        MemoryRegionSection *mrs = haddr->v.io.section;
> +        if (!mrs->mr->name) {
> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
> +        } else {
> +            return g_strdup(mrs->mr->name);
> +        }
> +    } else {
> +        return g_strdup("RAM");
> +    }
> +#else
> +    return g_strdup("Invalid");
> +#endif
> +}
> +
>   /*
>    * Queries to the number and potential maximum number of vCPUs there
>    * will be. This helps the plugin dimension per-vcpu arrays.
> 

Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
Emilio Cota June 8, 2020, 3:45 a.m. UTC | #2
On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote:
> This may well end up being anonymous but it should always be unique.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/qemu/qemu-plugin.h |  5 +++++
>  plugins/api.c              | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3a..43c6a9e857f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>  bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>  
> +/*
> + * Returns a string representing the device. Plugin must free() it
> + */
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
> +
>  typedef void
>  (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>                               qemu_plugin_meminfo_t info, uint64_t vaddr,
> diff --git a/plugins/api.c b/plugins/api.c
> index bbdc5a4eb46..3c73de8c1c2 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>      return 0;
>  }
>  
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    if (haddr && haddr->is_io) {
> +        MemoryRegionSection *mrs = haddr->v.io.section;
> +        if (!mrs->mr->name) {
> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
> +        } else {
> +            return g_strdup(mrs->mr->name);
> +        }
> +    } else {
> +        return g_strdup("RAM");
> +    }
> +#else
> +    return g_strdup("Invalid");
> +#endif
> +}

I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
have to worry about glib, and on the QEMU side we don't have to worry
about plugins calling free() instead of g_free().

Or given that this doesn't look perf-critical, perhaps an easier way out
is to wrap the above with:

char *g_str = above();
char *ret = strdup(g_str);
g_free(g_str);
return ret;

Not sure we should NULL-check ret, since I don't know whether
mrs->mr->name is guaranteed to be non-NULL.

Thanks,
		Emilio
Philippe Mathieu-Daudé June 8, 2020, 6:20 a.m. UTC | #3
On 6/8/20 5:45 AM, Emilio G. Cota wrote:
> On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote:
>> This may well end up being anonymous but it should always be unique.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/qemu/qemu-plugin.h |  5 +++++
>>  plugins/api.c              | 18 ++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index bab8b0d4b3a..43c6a9e857f 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>>  bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>>  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>>  
>> +/*
>> + * Returns a string representing the device. Plugin must free() it
>> + */
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
>> +
>>  typedef void
>>  (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>>                               qemu_plugin_meminfo_t info, uint64_t vaddr,
>> diff --git a/plugins/api.c b/plugins/api.c
>> index bbdc5a4eb46..3c73de8c1c2 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>>      return 0;
>>  }
>>  
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> +    if (haddr && haddr->is_io) {
>> +        MemoryRegionSection *mrs = haddr->v.io.section;
>> +        if (!mrs->mr->name) {
>> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
>> +        } else {
>> +            return g_strdup(mrs->mr->name);
>> +        }
>> +    } else {
>> +        return g_strdup("RAM");
>> +    }
>> +#else
>> +    return g_strdup("Invalid");
>> +#endif
>> +}
> 
> I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> have to worry about glib, and on the QEMU side we don't have to worry
> about plugins calling free() instead of g_free().

It might make sense, but it should be documented in
include/qemu/qemu-plugin.h or docs/devel/tcg-plugins.rst.

> 
> Or given that this doesn't look perf-critical, perhaps an easier way out
> is to wrap the above with:
> 
> char *g_str = above();
> char *ret = strdup(g_str);
> g_free(g_str);

free() ;)

> return ret;
> 
> Not sure we should NULL-check ret, since I don't know whether
> mrs->mr->name is guaranteed to be non-NULL.
> 
> Thanks,
> 		Emilio
>
Alex Bennée June 8, 2020, 8:06 a.m. UTC | #4
Emilio G. Cota <cota@braap.org> writes:

> On Tue, Jun 02, 2020 at 16:46:22 +0100, Alex Bennée wrote:
>> This may well end up being anonymous but it should always be unique.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/qemu/qemu-plugin.h |  5 +++++
>>  plugins/api.c              | 18 ++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>> 
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index bab8b0d4b3a..43c6a9e857f 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -335,6 +335,11 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>>  bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>>  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>>  
>> +/*
>> + * Returns a string representing the device. Plugin must free() it
>> + */
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
>> +
>>  typedef void
>>  (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>>                               qemu_plugin_meminfo_t info, uint64_t vaddr,
>> diff --git a/plugins/api.c b/plugins/api.c
>> index bbdc5a4eb46..3c73de8c1c2 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>>      return 0;
>>  }
>>  
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> +    if (haddr && haddr->is_io) {
>> +        MemoryRegionSection *mrs = haddr->v.io.section;
>> +        if (!mrs->mr->name) {
>> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
>> +        } else {
>> +            return g_strdup(mrs->mr->name);
>> +        }
>> +    } else {
>> +        return g_strdup("RAM");
>> +    }
>> +#else
>> +    return g_strdup("Invalid");
>> +#endif
>> +}
>
> I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> have to worry about glib, and on the QEMU side we don't have to worry
> about plugins calling free() instead of g_free().

AFAIK you can actually mix free/g_free because g_free is just a NULL
checking wrapper around free. However ideally I'd be passing a
non-freeable const char to the plugin but I didn't want to expose
pointers deep inside of QEMU's guts although maybe I'm just being
paranoid there given you can easily gdb the combined operation anyway.

Perhaps there is a need for a separate memory region where we can store
copies of strings we have made for the plugins?

> Or given that this doesn't look perf-critical, perhaps an easier way out
> is to wrap the above with:
>
> char *g_str = above();
> char *ret = strdup(g_str);
> g_free(g_str);
> return ret;
>
> Not sure we should NULL-check ret, since I don't know whether
> mrs->mr->name is guaranteed to be non-NULL.

Experimentation showed you can get null mrs->name has a result of a
region being subdivided due to an some operations. That said in another
thread Peter was uncomfortable about exposing this piece of information
to plugins. Maybe we should only expose something based on the optional
-device foo,id=bar parameter?

>
> Thanks,
> 		Emilio
Emilio Cota June 9, 2020, 4:09 a.m. UTC | #5
On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote:
> Emilio G. Cota <cota@braap.org> writes:
> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> > have to worry about glib, and on the QEMU side we don't have to worry
> > about plugins calling free() instead of g_free().
> 
> AFAIK you can actually mix free/g_free because g_free is just a NULL
> checking wrapper around free.

I was just going with the documentation, but you're right:

https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196
> void
> g_free (gpointer mem)
> {
>   free (mem);
>   TRACE(GLIB_MEM_FREE((void*) mem));
> }

The NULL-pointer check is done by free(3), though.

> However ideally I'd be passing a
> non-freeable const char to the plugin but I didn't want to expose
> pointers deep inside of QEMU's guts although maybe I'm just being
> paranoid there given you can easily gdb the combined operation anyway.
>
> Perhaps there is a need for a separate memory region where we can store
> copies of strings we have made for the plugins?

I agree with the idea of not exposing internal pointers to plugins
(e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK
with returning a dup'ed string here.

(snip)
> That said in another
> thread Peter was uncomfortable about exposing this piece of information
> to plugins. Maybe we should only expose something based on the optional
> -device foo,id=bar parameter?

I have no opinion on whether exposing this is a good idea. If it turns
out that it is, please have my

Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		Emilio
Alex Bennée June 9, 2020, 11:09 a.m. UTC | #6
Emilio G. Cota <cota@braap.org> writes:

> On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote:
>> Emilio G. Cota <cota@braap.org> writes:
>> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
>> > have to worry about glib, and on the QEMU side we don't have to worry
>> > about plugins calling free() instead of g_free().
>> 
>> AFAIK you can actually mix free/g_free because g_free is just a NULL
>> checking wrapper around free.
>
> I was just going with the documentation, but you're right:
>
> https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196
>> void
>> g_free (gpointer mem)
>> {
>>   free (mem);
>>   TRACE(GLIB_MEM_FREE((void*) mem));
>> }
>
> The NULL-pointer check is done by free(3), though.
>
>> However ideally I'd be passing a
>> non-freeable const char to the plugin but I didn't want to expose
>> pointers deep inside of QEMU's guts although maybe I'm just being
>> paranoid there given you can easily gdb the combined operation anyway.
>>
>> Perhaps there is a need for a separate memory region where we can store
>> copies of strings we have made for the plugins?
>
> I agree with the idea of not exposing internal pointers to plugins
> (e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK
> with returning a dup'ed string here.

How about a g_intern_string() as a non-freeable const char that can also
be treated as canonical?

>
> (snip)
>> That said in another
>> thread Peter was uncomfortable about exposing this piece of information
>> to plugins. Maybe we should only expose something based on the optional
>> -device foo,id=bar parameter?
>
> I have no opinion on whether exposing this is a good idea. If it turns
> out that it is, please have my
>
> Reviewed-by: Emilio G. Cota <cota@braap.org>
>
> Thanks,
>
> 		Emilio
Emilio Cota June 10, 2020, 2:32 a.m. UTC | #7
On Tue, Jun 09, 2020 at 12:09:54 +0100, Alex Bennée wrote:
> How about a g_intern_string() as a non-freeable const char that can also
> be treated as canonical?

I like it. Didn't know about g_intern_string (I see it's
implemented as an append-only hash table protected by a lock).

Cheers,

		Emilio
diff mbox series

Patch

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3a..43c6a9e857f 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,11 @@  struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
 bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
 uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
 
+/*
+ * Returns a string representing the device. Plugin must free() it
+ */
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
+
 typedef void
 (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
                              qemu_plugin_meminfo_t info, uint64_t vaddr,
diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb46..3c73de8c1c2 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,24 @@  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
     return 0;
 }
 
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
+{
+#ifdef CONFIG_SOFTMMU
+    if (haddr && haddr->is_io) {
+        MemoryRegionSection *mrs = haddr->v.io.section;
+        if (!mrs->mr->name) {
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
+        } else {
+            return g_strdup(mrs->mr->name);
+        }
+    } else {
+        return g_strdup("RAM");
+    }
+#else
+    return g_strdup("Invalid");
+#endif
+}
+
 /*
  * Queries to the number and potential maximum number of vCPUs there
  * will be. This helps the plugin dimension per-vcpu arrays.