Peter Maydell <peter.mayd...@linaro.org> writes:
> On Thu, 19 Sept 2024 at 14:11, Alex Bennée <alex.ben...@linaro.org> wrote:
>>
>> Peter Maydell <peter.mayd...@linaro.org> writes:
>> > While I'm looking at the code, this caught my eye:
>> >
>> > case QEMU_PLUGIN_MEM_VALUE_U64:
>> > {
>> > uint64_t *p = (uint64_t *) &ri->data[offset];
>> > uint64_t val = be ? htobe64(value.data.u64) :
>> > htole64(value.data.u64);
>> > if (is_store) {
>> > *p = val;
>> > } else if (*p != val) {
>> > unseen_data = true;
>> > }
>> > break;
>> > }
>> >
>> > Casting a random byte pointer to uint64_t* like that
>> > and dereferencing it isn't valid -- it can fault if
>> > it's not aligned correctly.
>>
>> Hmm in the normal case of x86 hosts we will never hit this.
>
> Not necessarily -- some x86 SIMD insns enforce alignment.
>
>> I guess we
>> could do a memcpy step and then the byteswap?
>
> That's what bswap.h does, yes.
>
>> Could it be included directly without bringing in the rest of QEMU's
>> include deps?
>
> It's technically quite close to standalone I think,
> but I think it would be a bad idea to directly include
> it because once you put QEMU's include/ on the plugin
> compile include path then that's a slippery slope to
> the plugins not actually being standalone code any more.
In this case tests/tcg/plugins are built for testing the implementation.
We could let it slide to save on just copy and pasting the whole file:
--8<---------------cut here---------------start------------->8---
modified tests/tcg/plugins/mem.c
@@ -9,10 +9,23 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
-#include <endian.h>
#include <stdio.h>
#include <glib.h>
+/*
+ * plugins should not include anything from QEMU aside from the
+ * API header. However the bswap.h header is sufficiently self
+ * contained that we include it here to avoid code duplication.
+ */
+#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
+#define HOST_LONG_BITS (__SIZEOF_POINTER__ * 8)
+#ifndef glue
+#define xglue(x, y) x ## y
+#define glue(x, y) xglue(x, y)
+#define stringify(s) tostring(s)
+#define tostring(s) #s
+#endif
+#include <bswap.h>
#include <qemu-plugin.h>
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
@@ -154,33 +167,45 @@ static void update_region_info(uint64_t region, uint64_t
offset,
case QEMU_PLUGIN_MEM_VALUE_U16:
{
uint16_t *p = (uint16_t *) &ri->data[offset];
- uint16_t val = be ? htobe16(value.data.u16) : htole16(value.data.u16);
if (is_store) {
- *p = val;
- } else if (*p != val) {
- unseen_data = true;
+ if (be) {
+ stw_be_p(p, value.data.u16);
+ } else {
+ stw_le_p(p, value.data.u16);
+ }
+ } else {
+ uint16_t val = be ? lduw_be_p(p) : lduw_le_p(p);
+ unseen_data = val != value.data.u16;
}
break;
}
case QEMU_PLUGIN_MEM_VALUE_U32:
{
uint32_t *p = (uint32_t *) &ri->data[offset];
- uint32_t val = be ? htobe32(value.data.u32) : htole32(value.data.u32);
if (is_store) {
- *p = val;
- } else if (*p != val) {
- unseen_data = true;
+ if (be) {
+ stl_be_p(p, value.data.u32);
+ } else {
+ stl_le_p(p, value.data.u32);
+ }
+ } else {
+ uint32_t val = be ? ldl_be_p(p) : ldl_le_p(p);
+ unseen_data = val != value.data.u32;
}
break;
}
case QEMU_PLUGIN_MEM_VALUE_U64:
{
uint64_t *p = (uint64_t *) &ri->data[offset];
- uint64_t val = be ? htobe64(value.data.u64) : htole64(value.data.u64);
if (is_store) {
- *p = val;
- } else if (*p != val) {
- unseen_data = true;
+ if (be) {
+ stq_be_p(p, value.data.u64);
+ } else {
+ stq_le_p(p, value.data.u64);
+ }
+ } else {
+ uint64_t val = be ? ldq_be_p(p) : ldq_le_p(p);
+ unseen_data = val != value.data.u64;
}
break;
--8<---------------cut here---------------end--------------->8---
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
