Re: [PATCH v14 0/8] Add additional plugin API functions to read and write memory and registers

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 6/27/25 2:17 AM, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 6/26/25 9:37 AM, Alex Bennée wrote:
>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>
>>>> Rowan Hart <rowanbhart@gmail.com> writes:
>>>>
>>>>> This patch series adds several new API functions focused on enabling use
>>>>> cases around reading and writing guest memory from QEMU plugins. To support
>>>>> these new APIs, some utility functionality around retrieving information about
>>>>> address spaces is added as well.
>>>>
>>>> Queued to plugins/next, thanks.
>>> So this fails a number of the CI tests, mostly due to 32 bit issues:
>>>     https://gitlab.com/stsquad/qemu/-/pipelines/1890883927/failures
>>> The tci failure is easy enough:
>>> --8<---------------cut here---------------start------------->8---
>>> modified   tests/tcg/x86_64/Makefile.softmmu-target
>>> @@ -34,9 +34,11 @@ memory: CFLAGS+=-DCHECK_UNALIGNED=1
>>>    # Running
>>>    QEMU_OPTS+=-device isa-debugcon,chardev=output -device isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
>>>    +ifeq ($(CONFIG_PLUGIN),y)
>>>    run-plugin-patch-target-with-libpatch.so:		\
>>>    	PLUGIN_ARGS=$(COMMA)target=ffc0$(COMMA)patch=9090$(COMMA)use_hwaddr=true
>>>    run-plugin-patch-target-with-libpatch.so:		\
>>>    	CHECK_PLUGIN_OUTPUT_COMMAND=$(X64_SYSTEM_SRC)/validate-patch.py $@.out
>>>    run-plugin-patch-target-with-libpatch.so: patch-target libpatch.so
>>>    EXTRA_RUNS+=run-plugin-patch-target-with-libpatch.so
>>> +endif
>>> --8<---------------cut here---------------end--------------->8---
>>> The other problem is trying to stuff a uint64_t into a void * on 32
>>> bit.
>>> We did disable plugins for 32 bit but then reverted because we were able
>>> to fixup the cases:
>>>    cf2a78cbbb (deprecation: don't enable TCG plugins by default on 32
>>> bit hosts)
>>>    db7a06ade1 (configure: reenable plugins by default for 32-bit hosts)
>>> So I don't what is easier:
>>>    - re-deprecate for 32 bit systems
>>>    - only build libpatch on 64 bit systems
>>>    - fix libpatch to handle being built on 32 bit systems
>>>
>>
>> More context:
>> ../tests/tcg/plugins/patch.c: In function ‘patch_hwaddr’:
>> ../tests/tcg/plugins/patch.c:50:21: error: cast from pointer to
>> integer of different size [-Werror=pointer-to-int-cast]
>>     50 |     uint64_t addr = (uint64_t)userdata;
>>        |                     ^
>> ../tests/tcg/plugins/patch.c: In function ‘patch_vaddr’:
>> ../tests/tcg/plugins/patch.c:93:21: error: cast from pointer to
>> integer of different size [-Werror=pointer-to-int-cast]
>>     93 |     uint64_t addr = (uint64_t)userdata;
>>        |                     ^
>> ../tests/tcg/plugins/patch.c: In function ‘vcpu_tb_trans_cb’:
>> ../tests/tcg/plugins/patch.c:159:54: error: cast to pointer from
>> integer of different size [-Werror=int-to-pointer-cast]
>>    159 |                                                      (void *)addr);
>>        |                                                      ^
>> ../tests/tcg/plugins/patch.c:163:54: error: cast to pointer from
>> integer of different size [-Werror=int-to-pointer-cast]
>>    163 |                                                      (void *)addr);
>>        |
>>
>> Since we disabled 64 bit targets on 32 bit hosts, and that data passed
>> by pointers concern addresses, it should be safe to cast values to
>> (uintptr_t) instead of (uint64_t).
> 
> Something like this?
> 
> --8<---------------cut here---------------start------------->8---
> modified   tests/tcg/plugins/patch.c
> @@ -47,10 +47,10 @@ static GByteArray *str_to_bytes(const char *str)
>   
>   static void patch_hwaddr(unsigned int vcpu_index, void *userdata)
>   {
> -    uint64_t addr = (uint64_t)userdata;
> +    uintptr_t addr = (uintptr_t) userdata;
>       g_autoptr(GString) str = g_string_new(NULL);
>       g_string_printf(str, "patching: @0x%"
> -                    PRIx64 "\n",
> +                    PRIxPTR "\n",
>                       addr);
>       qemu_plugin_outs(str->str);
>   
> @@ -90,7 +90,7 @@ static void patch_hwaddr(unsigned int vcpu_index, void *userdata)
>   
>   static void patch_vaddr(unsigned int vcpu_index, void *userdata)
>   {
> -    uint64_t addr = (uint64_t)userdata;
> +    uintptr_t addr = (uintptr_t) userdata;
>       uint64_t hwaddr = 0;
>       if (!qemu_plugin_translate_vaddr(addr, &hwaddr)) {
>           qemu_plugin_outs("Failed to translate vaddr\n");
> @@ -98,7 +98,7 @@ static void patch_vaddr(unsigned int vcpu_index, void *userdata)
>       }
>       g_autoptr(GString) str = g_string_new(NULL);
>       g_string_printf(str, "patching: @0x%"
> -                    PRIx64 " hw: @0x%" PRIx64 "\n",
> +                    PRIxPTR " hw: @0x%" PRIx64 "\n",
>                       addr, hwaddr);
>       qemu_plugin_outs(str->str);
>   
> @@ -132,19 +132,29 @@ static void patch_vaddr(unsigned int vcpu_index, void *userdata)
>    */
>   static void vcpu_tb_trans_cb(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>   {
> -    uint64_t addr = 0;
>       g_autoptr(GByteArray) insn_data = g_byte_array_new();
> +    uintptr_t addr = 0;
> +
>       for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
>           struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> +        uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
>   
>           if (use_hwaddr) {
> -            uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
> -            if (!qemu_plugin_translate_vaddr(vaddr, &addr)) {
> +            uint64_t hwaddr = 0;
> +            if (!qemu_plugin_translate_vaddr(vaddr, &hwaddr)) {
>                   qemu_plugin_outs("Failed to translate vaddr\n");
>                   continue;
>               }
> +            /*
> +             * As we cannot emulate 64 bit systems on 32 bit hosts we
> +             * should never see the top bits set, hence we can safely
> +             * cast to uintptr_t.
> +             */
> +            g_assert(!(hwaddr & ~UINTPTR_MAX));

We would have so many other problems before plugins if this hypothesis 
was not true (all the usage of vaddr type in the codebase would be 
broken). So the assert will not detect anything we are not aware about 
already.

If we want to mention this assumption for plugins users, the plugins 
documentation is probably a better place than one random plugin.

> +            addr = (uintptr_t) hwaddr;
>           } else {
> -            addr = qemu_plugin_insn_vaddr(insn);
> +            g_assert(!(vaddr & ~UINTPTR_MAX));
> +            addr = (uintptr_t) vaddr;
>           }
>   
>           g_byte_array_set_size(insn_data, qemu_plugin_insn_size(insn));
> @@ -156,11 +166,11 @@ static void vcpu_tb_trans_cb(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>               if (use_hwaddr) {
>                   qemu_plugin_register_vcpu_tb_exec_cb(tb, patch_hwaddr,
>                                                        QEMU_PLUGIN_CB_NO_REGS,
> -                                                     (void *)addr);
> +                                                     (void *) addr);
>               } else {
>                   qemu_plugin_register_vcpu_tb_exec_cb(tb, patch_vaddr,
>                                                        QEMU_PLUGIN_CB_NO_REGS,
> -                                                     (void *)addr);
> +                                                     (void *) addr);
>               }
>           }
>       }
> --8<---------------cut here---------------end--------------->8---
> 
> 
>>
>> Pierrick
> 

For the rest, it looks good to me.

Thanks,
Pierrick