It makes more sense to return the same type than qemu_plugin_insn_vaddr.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/qemu/qemu-plugin.h |  2 +-
 plugins/api.c              | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 0fba36ae028..1fbcff6e1d2 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -537,7 +537,7 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn);
  * Returns: hardware (physical) target address of instruction
  */
 QEMU_PLUGIN_API
-void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
+uint64_t qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
 
 /**
  * typedef qemu_plugin_meminfo_t - opaque memory transaction handle
diff --git a/plugins/api.c b/plugins/api.c
index 24ea64e2de5..17b3a65e773 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -283,13 +283,13 @@ uint64_t qemu_plugin_insn_vaddr(const struct qemu_plugin_insn *insn)
     return insn->vaddr;
 }
 
-void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn)
+uint64_t qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn)
 {
     const DisasContextBase *db = tcg_ctx->plugin_db;
     vaddr page0_last = db->pc_first | ~TARGET_PAGE_MASK;
 
     if (db->fake_insn) {
-        return NULL;
+        return 0;
     }
 
     /*
@@ -300,14 +300,14 @@ void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn)
      */
     if (insn->vaddr <= page0_last) {
         if (db->host_addr[0] == NULL) {
-            return NULL;
+            return 0;
         }
-        return db->host_addr[0] + insn->vaddr - db->pc_first;
+        return (uintptr_t) (db->host_addr[0] + insn->vaddr - db->pc_first);
     } else {
         if (db->host_addr[1] == NULL) {
-            return NULL;
+            return 0;
         }
-        return db->host_addr[1] + insn->vaddr - (page0_last + 1);
+        return (uintptr_t) (db->host_addr[1] + insn->vaddr - (page0_last + 1));
     }
 }
 
-- 
2.39.5

On 12/16/24 19:06, Pierrick Bouvier wrote:
> It makes more sense to return the same type than qemu_plugin_insn_vaddr.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/qemu/qemu-plugin.h |  2 +-
>   plugins/api.c              | 12 ++++++------
>   2 files changed, 7 insertions(+), 7 deletions(-)

No, it does not.

qemu_plugin_insn_vaddr is returning a guest virtual address.
qemu_plugin_insn_haddr is returning a host address.

I'm not sure why we decided that returning a host pointer was a good idea.  Probably it 
was the easiest thing to retrieve from softmmu.

One could argue that we should be returning something else, the only question is what.

Perhaps guest physical address, which wasn't possible before, but which is now stored 
within CPUTLBEntryFull. Interpreting this requires you to know the physical address space 
to which it applies. In the case of Arm, the address space varies depending on Secure vs 
Non-Secure state.

Perhaps ram_addr_t, which is *not* a guest physical address because it is not associated 
with any address space. It is more of a globally unique token with which a RAMBlock may be 
found.  It's how we stitch together address spaces under the hood.  The plugin would have 
to treat it as an opaque unique identifier.

But if we're going to return a host address, then void* is the correct type.


r~

On 12/17/24 06:41, Richard Henderson wrote:
> On 12/16/24 19:06, Pierrick Bouvier wrote:
>> It makes more sense to return the same type than qemu_plugin_insn_vaddr.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/qemu/qemu-plugin.h |  2 +-
>>    plugins/api.c              | 12 ++++++------
>>    2 files changed, 7 insertions(+), 7 deletions(-)
> 
> No, it does not.
> 
> qemu_plugin_insn_vaddr is returning a guest virtual address.
> qemu_plugin_insn_haddr is returning a host address.
> 
> I'm not sure why we decided that returning a host pointer was a good idea.  Probably it
> was the easiest thing to retrieve from softmmu.
>

When looking at the implementation of qemu_plugin_insn_haddr, I was a 
bit surprised to see that we return the host pointer indeed. So, I 
thought that returning a uint64_t would act as an "opaque" handle in itself.

The only usage in plugins we have is for cache plugin, to ensure it does 
instrument the same instruction only once, even though it's translated 
several times, or from different virtual addresses.

> One could argue that we should be returning something else, the only question is what.
> 
> Perhaps guest physical address, which wasn't possible before, but which is now stored
> within CPUTLBEntryFull. Interpreting this requires you to know the physical address space
> to which it applies. In the case of Arm, the address space varies depending on Secure vs
> Non-Secure state.
> 
> Perhaps ram_addr_t, which is *not* a guest physical address because it is not associated
> with any address space. It is more of a globally unique token with which a RAMBlock may be
> found.  It's how we stitch together address spaces under the hood.  The plugin would have
> to treat it as an opaque unique identifier.
> 

I'm not sure I want to open this as part of this series, as there will 
probably be corner cases and debates, while the goal here is just to 
compile for 32-bit platforms.

> But if we're going to return a host address, then void* is the correct type.
>

I'll stick to void * then.

> 
> r~

Thanks,
Pierrick