Re: [PATCH 01/13] plugins: change signature of qemu_plugin_insn_haddr

[Pierrick Bouvier]

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