On Fri, 16 Aug 2024 at 13:48, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Richard Henderson <richard.hender...@linaro.org> writes:
>
> > On 8/15/24 21:46, Alex Bennée wrote:
> >> Thomas Huth <th...@redhat.com> writes:
> >>
> >>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
> >>>> Found on debian stable (i386).
> >>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
> >>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer
> >>>> of different size [-Werror=pointer-to-int-cast]
> >>>> 477 | effective_addr = (uint64_t)
> >>>> qemu_plugin_insn_haddr(insn);
> >>>> |
> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
> >>>> ---
> >>>> contrib/plugins/cache.c | 6 +++---
> >>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> >>>> index 512ef6776b7..82ed734d6d4 100644
> >>>> --- a/contrib/plugins/cache.c
> >>>> +++ b/contrib/plugins/cache.c
> >>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id,
> >>>> struct qemu_plugin_tb *tb)
> >>>> n_insns = qemu_plugin_tb_n_insns(tb);
> >>>> for (i = 0; i < n_insns; i++) {
> >>>> struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb,
> >>>> i);
> >>>> - uint64_t effective_addr;
> >>>> + uintptr_t effective_addr;
> >>>> if (sys) {
> >>>> - effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
> >>>> + effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
> >>>> } else {
> >>>> - effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
> >>>> + effective_addr = (uintptr_t)
> >>>> qemu_plugin_insn_vaddr(insn);
> >>>> }
> >>>
> >>> Is this the right fix? I assume effective_addr stores an address of
> >>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
> >>> lose the upper bits of the address...?
> >> I think the problem is higher up, it was a mistake to have:
> >> void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn
> >> *insn);
> >> return *void.
> >
> > No, not a bug. This is a host addr, right there in the name.
> > Returning uint64_t would be a bug.
>
> No it's:
>
> * Returns: hardware (physical) target address of instruction
>
> I was kinda assuming that was what the underlying host_addr[] fields in
> DisasContextDB are. Are we just saying its QEMU's vaddr of where the
> guest physical address is mapped into QEMU?
DisasContextBase::host_addr[] are host (virtual) addresses,
i.e. pointers you can dereference in C code in QEMU.
The comment in qemu_plugin_insn_haddr() says:
* ??? The return value is not intended for use of host memory,
* but as a proxy for address space and physical address.
This seems pretty QEMU-implementation-internal to me and
not something we should be allowing to escape into the
plugin API. What, per the comment, we ought to be exposing is
the (address space, guest physaddr) tuple, which we
presumably don't conveniently have to hand here.
thanks
-- PMM
