accel/tcg/plugin-gen.c | 30 +++ gdbstub/gdbstub.c | 2 +- include/exec/gdbstub.h | 14 ++ include/hw/core/cpu.h | 1 + include/qemu/plugin.h | 15 ++ include/qemu/qemu-plugin.h | 176 ++++++++++++++-- plugins/api.c | 135 +++++++++++- plugins/core.c | 33 +++ tests/tcg/Makefile.target | 7 +- tests/tcg/plugins/meson.build | 2 +- tests/tcg/plugins/patch.c | 241 ++++++++++++++++++++++ tests/tcg/x86_64/Makefile.softmmu-target | 19 +- tests/tcg/x86_64/system/patch-target.c | 22 ++ tests/tcg/x86_64/system/validate-patch.py | 39 ++++ 14 files changed, 710 insertions(+), 26 deletions(-) create mode 100644 tests/tcg/plugins/patch.c create mode 100644 tests/tcg/x86_64/system/patch-target.c create mode 100755 tests/tcg/x86_64/system/validate-patch.py
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.
The new qemu_plugin_write_register utilizes gdb_write_register, which is now
declared in gdbstub.h for this purpose instead of being static.
qemu_plugin_write_memory_vaddr utilizes cpu_memory_rw_debug much the same as
the existing read_memory_vaddr function does.
The read and write_hwaddr functions are the most different. These functions
use address_space_rw, which works well in most cases. There is an important
caveat that for writes, the page being written will be set dirty by the
write operation. This dirty setting requires locking the page range,
which can contend with an already held lock in page_collection_lock
when called in a tb translate callback with a write to the instruction
memory in the tb. The doc comments warn against doing this, and it's unlikely
anyone would want to do this.
I've also added two test plugins: one that implements a simple hypercall
interface that guest code can use to communicate with the plugin in a
structured way with a test to ensure that this hypercall works and writing
virtual memory works. And one that implements a simple patch utility to patch
memory at runtime. The test for the second plugin ensures the patch applies
successfully to instruction memory, and can use both hw and vaddr methods.
For v3, I've had a few comments from the last submission that I've addressed,
and some that I haven't for one reason or another:
- Enforce QEMU_PLUGIN_CB_ flags in register read/write operations: done!
- Fix my commit messages and add long messages describing commits: done!
- Un-expose AS internals: done! Functions operate on current vCPU, current AS.
- Clean up use of current_cpu: done!
- Make functions take a vcpu_idx: not done. May revisit but it allows footguns.
Even for translation, seems best to not do this now. We can easily add _vcpu
versions of these functions in the future if we change our minds!
For v5, I've just updated the enforcement of the QEMU_PLUGIN_CB_ flags to just
use immediate stores, which simplifies the implementation quite a lot and
should be more efficient too. Thanks Pierrick for the suggestion!
v6 is a formatting pass, I left some whitespace that needed removal, some
license text was wrong, and so forth.
v8 reverts a mistake I made extending the size of arrays of TCGHelperInfo
structs, as I misunderstood their sizes. It preserves adding an explicit
zero as the last entry for clarity, however.
v9 fixes qemu_plugin_read_register to return -1 on parameter or flag state
error instead of 0.
In v10, I relaxed the restriction on when the register r/w functions can be
called, allowing all them to be used from any callback where the CPU is not
currently executing, with additional notes in the documentation for exceptions
(atexit and flush, which do not operate on a specific CPU and in which
current_cpu is not set).
v11 makes the cb flags functions inline and fixes a typo where cpu was asserted
but current_cpu was actually accessed.
v12 removes the hypercalls plugin because the functions it tested are also
tested by the patcher plugin, making it redundant. We'll circle back on a
hypercalls API in the future as a part of the plugin API, not as a plugin
itself.
v13 fixes up some issues Alex pointed out with the patch test. Now, the patch
target only runs with libpatch.so, and libpatch.so only runs with the patch
test.
v14 addresses a build issue Alex pointed out with the patch test, and removes
a few vestigial lines according to Alex's suggestion. I split this out into
another commit as it's not actually coupled to the patch test.
Rowan Hart (8):
gdbstub: Expose gdb_write_register function to consumers of gdbstub
plugins: Add register write API
plugins: Add enforcement of QEMU_PLUGIN_CB flags in register R/W
callbacks
plugins: Add memory virtual address write API
plugins: Add memory hardware address read/write API
tests/tcg: Remove copy-pasted notes and from i386 and add x86_64
system tests to tests
plugins: Add patcher plugin and test
plugins: Update plugin version and add notes
accel/tcg/plugin-gen.c | 30 +++
gdbstub/gdbstub.c | 2 +-
include/exec/gdbstub.h | 14 ++
include/hw/core/cpu.h | 1 +
include/qemu/plugin.h | 15 ++
include/qemu/qemu-plugin.h | 176 ++++++++++++++--
plugins/api.c | 135 +++++++++++-
plugins/core.c | 33 +++
tests/tcg/Makefile.target | 7 +-
tests/tcg/plugins/meson.build | 2 +-
tests/tcg/plugins/patch.c | 241 ++++++++++++++++++++++
tests/tcg/x86_64/Makefile.softmmu-target | 19 +-
tests/tcg/x86_64/system/patch-target.c | 22 ++
tests/tcg/x86_64/system/validate-patch.py | 39 ++++
14 files changed, 710 insertions(+), 26 deletions(-)
create mode 100644 tests/tcg/plugins/patch.c
create mode 100644 tests/tcg/x86_64/system/patch-target.c
create mode 100755 tests/tcg/x86_64/system/validate-patch.py
--
2.49.0
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. -- Alex Bennée Virtualisation Tech Lead @ Linaro
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 -- Alex Bennée Virtualisation Tech Lead @ Linaro
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).
Pierrick
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));
+ 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
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
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 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.
Well we could change the API function signature to return uintptr_t?
>
>> + 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On 6/27/25 11:26 AM, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> 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.
>
> Well we could change the API function signature to return uintptr_t?
>
While it's possible, I personally would favor a fixed ABI for types
independently of which host config we have. It's less surprising and
easier to reason about.
For tcg, we have a reason to optimize this, to avoid manipulating 64
bits data, when we know it's only 32 in reality.
That said, one thing we could enhance on plugins side is to ease passing
immediate data to a callback, without relying on pointer casting, which
can be subtly wrong on 32 bits hosts. It's definitely out of the scope
for this series though.
© 2016 - 2025 Red Hat, Inc.