On 6/27/25 2:17 AM, Alex Bennée wrote:
Pierrick Bouvier <pierrick.bouv...@linaro.org> writes:
On 6/26/25 9:37 AM, Alex Bennée wrote:
Alex Bennée <alex.ben...@linaro.org> writes:
Rowan Hart <rowanbh...@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
