Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 8/15/24 04:46, Alex Bennée wrote:
>> Thomas Huth <thuth@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.bouvier@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, at least vaddr returns an explicit 64 bit value which
>> can
>> hold everything (at a slight expense to 32bit emulation hosts, but
>> seriously stop doing that we've been in the 64bit world for some time
>> now).
>> 
>
> It's an open question I had. When executing 64 bits binaries on a 32
> bits host, are we emulating the full 64 bits address space, or do we
> restrict to 32 bits?

Yes - and having to jump through a few extra hoops to do it.

> For user mode, I don't see how it could be
> possible to have address space beyond the 32 bits range, but system
> mode is probably different.

For user-modes very simple base + addr calculation yes it is tricky and
we often fail to find a gap big enough to map the guest address space
into it. If we implement softmmu for linux-user we could get around this
difficulty for a cost.

>
> The real proper fix is to not encode directly value under udata for
> callbacks, but allocate this and pass a pointer instead.
>
>>>
>>> The casting for qemu_plugin_insn_vaddr is not required at all since it
>>> already returns an uint64_t, so you can remoe that one. For the haddr
>>> part, maybe do a double-cast:
>>>
>>>    effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>>>
>>> ?
>>>
>>>   Thomas
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro