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

Message ID 20200709141327.14631-7-alex.bennee@linaro.org
State Superseded
Headers show
Series
  • misc rc0 fixes (docs, plugins, docker)
Related show

Commit Message

Alex Bennée July 9, 2020, 2:13 p.m.
This may well end up being anonymous but it should always be unique.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

[r-b provisional given change to g_intern_string]
Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>

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


---
v3
  - return a non-freeable const g_intern_string()
  - checkpatch cleanups
---
 include/qemu/qemu-plugin.h |  6 ++++++
 plugins/api.c              | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

-- 
2.20.1

Comments

Philippe Mathieu-Daudé July 9, 2020, 3:03 p.m. | #1
On 7/9/20 4:13 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>

> [r-b provisional given change to g_intern_string]

> Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>

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

> 

> ---

> v3

>   - return a non-freeable const g_intern_string()

>   - checkpatch cleanups

> ---

>  include/qemu/qemu-plugin.h |  6 ++++++

>  plugins/api.c              | 20 ++++++++++++++++++++

>  2 files changed, 26 insertions(+)

> 

> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h

> index bab8b0d4b3af..c98c18d6b052 100644

> --- a/include/qemu/qemu-plugin.h

> +++ b/include/qemu/qemu-plugin.h

> @@ -335,6 +335,12 @@ 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. The string is valid for

> + * the lifetime of the plugin.

> + */

> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);

> +

>  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 bbdc5a4eb46d..4304e63f0cf8 100644

> --- a/plugins/api.c

> +++ b/plugins/api.c

> @@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr

>      return 0;

>  }

>  

> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)

> +{

> +#ifdef CONFIG_SOFTMMU

> +    if (h && h->is_io) {

> +        MemoryRegionSection *mrs = h->v.io.section;

> +        if (!mrs->mr->name) {

> +            unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;


Why not use uint32_t & PRIx32?

               uint32_t maddr = (uintptr_t) mrs->mr;

> +            g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);

> +            return g_intern_string(temp);


Isn't this illegal? temp is definitively not const...

> +        } else {

> +            return g_intern_string(mrs->mr->name);

> +        }

> +    } else {

> +        return g_intern_string("RAM");

> +    }

> +#else

> +    return g_intern_string("Invalid");

> +#endif

> +}

> +

>  /*

>   * Queries to the number and potential maximum number of vCPUs there

>   * will be. This helps the plugin dimension per-vcpu arrays.

>
Alex Bennée July 9, 2020, 4:30 p.m. | #2
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 7/9/20 4:13 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>

>> [r-b provisional given change to g_intern_string]

>> Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>

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

>> 

>> ---

>> v3

>>   - return a non-freeable const g_intern_string()

>>   - checkpatch cleanups

>> ---

>>  include/qemu/qemu-plugin.h |  6 ++++++

>>  plugins/api.c              | 20 ++++++++++++++++++++

>>  2 files changed, 26 insertions(+)

>> 

>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h

>> index bab8b0d4b3af..c98c18d6b052 100644

>> --- a/include/qemu/qemu-plugin.h

>> +++ b/include/qemu/qemu-plugin.h

>> @@ -335,6 +335,12 @@ 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. The string is valid for

>> + * the lifetime of the plugin.

>> + */

>> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);

>> +

>>  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 bbdc5a4eb46d..4304e63f0cf8 100644

>> --- a/plugins/api.c

>> +++ b/plugins/api.c

>> @@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr

>>      return 0;

>>  }

>>  

>> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)

>> +{

>> +#ifdef CONFIG_SOFTMMU

>> +    if (h && h->is_io) {

>> +        MemoryRegionSection *mrs = h->v.io.section;

>> +        if (!mrs->mr->name) {

>> +            unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;

>

> Why not use uint32_t & PRIx32?

>

>                uint32_t maddr = (uintptr_t) mrs->mr;

>

>> +            g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);

>> +            return g_intern_string(temp);

>

> Isn't this illegal? temp is definitively not const...


We don't mess with it, we return a new string which is the canonical
form.

>

>> +        } else {

>> +            return g_intern_string(mrs->mr->name);

>> +        }

>> +    } else {

>> +        return g_intern_string("RAM");

>> +    }

>> +#else

>> +    return g_intern_string("Invalid");

>> +#endif

>> +}

>> +

>>  /*

>>   * Queries to the number and potential maximum number of vCPUs there

>>   * will be. This helps the plugin dimension per-vcpu arrays.

>> 



-- 
Alex Bennée

Patch

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3af..c98c18d6b052 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,12 @@  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. The string is valid for
+ * the lifetime of the plugin.
+ */
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
+
 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 bbdc5a4eb46d..4304e63f0cf8 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,26 @@  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
     return 0;
 }
 
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
+{
+#ifdef CONFIG_SOFTMMU
+    if (h && h->is_io) {
+        MemoryRegionSection *mrs = h->v.io.section;
+        if (!mrs->mr->name) {
+            unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
+            g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
+            return g_intern_string(temp);
+        } else {
+            return g_intern_string(mrs->mr->name);
+        }
+    } else {
+        return g_intern_string("RAM");
+    }
+#else
+    return g_intern_string("Invalid");
+#endif
+}
+
 /*
  * Queries to the number and potential maximum number of vCPUs there
  * will be. This helps the plugin dimension per-vcpu arrays.