* Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr
2024-08-28 6:32 ` [PATCH 1/1] " Rowan Hart
@ 2024-08-28 14:41 ` Rowan Hart
2024-08-30 19:30 ` Pierrick Bouvier
2025-01-09 11:38 ` Alex Bennée
2 siblings, 0 replies; 8+ messages in thread
From: Rowan Hart @ 2024-08-28 14:41 UTC (permalink / raw)
To: qemu-devel
Cc: Alexandre Iooss, Alex Bennée, Pierrick Bouvier,
Mahmoud Mandour
[-- Attachment #1: Type: text/plain, Size: 265 bytes --]
> + qemu_plugin_read_cpu_memory_hwaddr;
> + qemu_plugin_read_io_memory_hwaddr;
This second symbol name should be removed, I initially wanted to implement
for IO as well but there is no good generic way I can see to access a list
of IO AddressSpace to read from.
[-- Attachment #2: Type: text/html, Size: 348 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr
2024-08-28 6:32 ` [PATCH 1/1] " Rowan Hart
2024-08-28 14:41 ` Rowan Hart
@ 2024-08-30 19:30 ` Pierrick Bouvier
2024-08-30 19:33 ` Pierrick Bouvier
2025-01-09 11:38 ` Alex Bennée
2 siblings, 1 reply; 8+ messages in thread
From: Pierrick Bouvier @ 2024-08-30 19:30 UTC (permalink / raw)
To: Rowan Hart, qemu-devel; +Cc: Alexandre Iooss, Alex Bennée, Mahmoud Mandour
Hi Rowan,
thanks for this good complement on the virt address read function.
However, to be able to merge a new plugins API function, we must have a
concrete usage of it, through one of the existing plugin.
What could be a good demonstration of value brought by being able to
read a physical address?
Thanks,
Pierrick
On 8/27/24 23:32, Rowan Hart wrote:
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
> include/qemu/qemu-plugin.h | 22 ++++++++++++++++++++++
> plugins/api.c | 17 +++++++++++++++++
> plugins/qemu-plugins.symbols | 2 ++
> 3 files changed, 41 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index c71c705b69..25f39c0960 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -868,6 +868,28 @@ QEMU_PLUGIN_API
> int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> GByteArray *buf);
>
> +/**
> + * qemu_plugin_read_cpu_memory_hwaddr() - read CPU memory from hwaddr
> + *
> + * @addr: A virtual address to read from
> + * @data: A byte array to store data into
> + * @len: The number of bytes to read, starting from @addr
> + *
> + * @len bytes of data is read starting at @addr and stored into @data. If @data
> + * is not large enough to hold @len bytes, it will be expanded to the necessary
> + * size, reallocating if necessary. @len must be greater than 0.
> + *
> + * This function does not ensure writes are flushed prior to reading, so
> + * callers should take care when calling this function in plugin callbacks to
> + * avoid attempting to read data which may not yet be written and should use
> + * the memory callback API instead.
> + *
> + * Returns true on success and false on failure.
> + */
> +QEMU_PLUGIN_API
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> + GByteArray *data, size_t len);
> +
> /**
> * qemu_plugin_scoreboard_new() - alloc a new scoreboard
> *
> diff --git a/plugins/api.c b/plugins/api.c
> index 2ff13d09de..c87bed6641 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -527,6 +527,22 @@ GArray *qemu_plugin_get_registers(void)
> return create_register_handles(regs);
> }
>
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> + GByteArray *data, uint64_t len)
> +{
> +#ifndef CONFIG_USER_ONLY
> + if (len == 0) {
> + return false;
> + }
> +
> + g_byte_array_set_size(data, len);
> + cpu_physical_memory_rw(addr, (void *)data->data, len, 0);
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> {
> g_assert(current_cpu);
> @@ -534,6 +550,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
> }
>
> +
> struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
> {
> return plugin_scoreboard_new(element_size);
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index ca773d8d9f..5d9cfd71bb 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -20,6 +20,8 @@
> qemu_plugin_num_vcpus;
> qemu_plugin_outs;
> qemu_plugin_path_to_binary;
> + qemu_plugin_read_cpu_memory_hwaddr;
> + qemu_plugin_read_io_memory_hwaddr;
As you mentioned, you can remove the second one for v2.
> qemu_plugin_read_register;
> qemu_plugin_register_atexit_cb;
> qemu_plugin_register_flush_cb;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr
2024-08-30 19:30 ` Pierrick Bouvier
@ 2024-08-30 19:33 ` Pierrick Bouvier
0 siblings, 0 replies; 8+ messages in thread
From: Pierrick Bouvier @ 2024-08-30 19:33 UTC (permalink / raw)
To: Rowan Hart, qemu-devel; +Cc: Alexandre Iooss, Alex Bennée, Mahmoud Mandour
And by the way, feel free to integrate this with your other series (as
it's a very similar topic) in a v3, so we can review both at the same time.
Thanks,
Pierrick
On 8/30/24 12:30, Pierrick Bouvier wrote:
> Hi Rowan,
>
> thanks for this good complement on the virt address read function.
>
> However, to be able to merge a new plugins API function, we must have a
> concrete usage of it, through one of the existing plugin.
> What could be a good demonstration of value brought by being able to
> read a physical address?
>
> Thanks,
> Pierrick
>
> On 8/27/24 23:32, Rowan Hart wrote:
>> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
>> ---
>> include/qemu/qemu-plugin.h | 22 ++++++++++++++++++++++
>> plugins/api.c | 17 +++++++++++++++++
>> plugins/qemu-plugins.symbols | 2 ++
>> 3 files changed, 41 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index c71c705b69..25f39c0960 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -868,6 +868,28 @@ QEMU_PLUGIN_API
>> int qemu_plugin_read_register(struct qemu_plugin_register *handle,
>> GByteArray *buf);
>>
>> +/**
>> + * qemu_plugin_read_cpu_memory_hwaddr() - read CPU memory from hwaddr
>> + *
>> + * @addr: A virtual address to read from
>> + * @data: A byte array to store data into
>> + * @len: The number of bytes to read, starting from @addr
>> + *
>> + * @len bytes of data is read starting at @addr and stored into @data. If @data
>> + * is not large enough to hold @len bytes, it will be expanded to the necessary
>> + * size, reallocating if necessary. @len must be greater than 0.
>> + *
>> + * This function does not ensure writes are flushed prior to reading, so
>> + * callers should take care when calling this function in plugin callbacks to
>> + * avoid attempting to read data which may not yet be written and should use
>> + * the memory callback API instead.
>> + *
>> + * Returns true on success and false on failure.
>> + */
>> +QEMU_PLUGIN_API
>> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
>> + GByteArray *data, size_t len);
>> +
>> /**
>> * qemu_plugin_scoreboard_new() - alloc a new scoreboard
>> *
>> diff --git a/plugins/api.c b/plugins/api.c
>> index 2ff13d09de..c87bed6641 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -527,6 +527,22 @@ GArray *qemu_plugin_get_registers(void)
>> return create_register_handles(regs);
>> }
>>
>> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
>> + GByteArray *data, uint64_t len)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> + if (len == 0) {
>> + return false;
>> + }
>> +
>> + g_byte_array_set_size(data, len);
>> + cpu_physical_memory_rw(addr, (void *)data->data, len, 0);
>> + return true;
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
>> {
>> g_assert(current_cpu);
>> @@ -534,6 +550,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
>> return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
>> }
>>
>> +
>> struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
>> {
>> return plugin_scoreboard_new(element_size);
>> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
>> index ca773d8d9f..5d9cfd71bb 100644
>> --- a/plugins/qemu-plugins.symbols
>> +++ b/plugins/qemu-plugins.symbols
>> @@ -20,6 +20,8 @@
>> qemu_plugin_num_vcpus;
>> qemu_plugin_outs;
>> qemu_plugin_path_to_binary;
>> + qemu_plugin_read_cpu_memory_hwaddr;
>> + qemu_plugin_read_io_memory_hwaddr;
>
> As you mentioned, you can remove the second one for v2.
>
>> qemu_plugin_read_register;
>> qemu_plugin_register_atexit_cb;
>> qemu_plugin_register_flush_cb;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr
2024-08-28 6:32 ` [PATCH 1/1] " Rowan Hart
2024-08-28 14:41 ` Rowan Hart
2024-08-30 19:30 ` Pierrick Bouvier
@ 2025-01-09 11:38 ` Alex Bennée
2 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2025-01-09 11:38 UTC (permalink / raw)
To: Rowan Hart; +Cc: qemu-devel, Alexandre Iooss, Pierrick Bouvier, Mahmoud Mandour
Rowan Hart <rowanbhart@gmail.com> writes:
Apologies for the delay, I realise this has been sitting on my review
queue too long.
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
> include/qemu/qemu-plugin.h | 22 ++++++++++++++++++++++
> plugins/api.c | 17 +++++++++++++++++
> plugins/qemu-plugins.symbols | 2 ++
> 3 files changed, 41 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index c71c705b69..25f39c0960 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -868,6 +868,28 @@ QEMU_PLUGIN_API
> int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> GByteArray *buf);
>
> +/**
> + * qemu_plugin_read_cpu_memory_hwaddr() - read CPU memory from hwaddr
> + *
> + * @addr: A virtual address to read from
A physical address
> + * @data: A byte array to store data into
> + * @len: The number of bytes to read, starting from @addr
> + *
> + * @len bytes of data is read starting at @addr and stored into @data. If @data
> + * is not large enough to hold @len bytes, it will be expanded to the necessary
> + * size, reallocating if necessary. @len must be greater than 0.
> + *
> + * This function does not ensure writes are flushed prior to reading, so
> + * callers should take care when calling this function in plugin callbacks to
> + * avoid attempting to read data which may not yet be written and should use
> + * the memory callback API instead.
Maybe we should be clear this can only be called from a vCPU context?
See bellow.
> + *
> + * Returns true on success and false on failure.
> + */
> +QEMU_PLUGIN_API
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> + GByteArray *data, size_t len);
> +
> /**
> * qemu_plugin_scoreboard_new() - alloc a new scoreboard
> *
> diff --git a/plugins/api.c b/plugins/api.c
> index 2ff13d09de..c87bed6641 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -527,6 +527,22 @@ GArray *qemu_plugin_get_registers(void)
> return create_register_handles(regs);
> }
>
> +bool qemu_plugin_read_cpu_memory_hwaddr(uint64_t addr,
> + GByteArray *data, uint64_t len)
> +{
> +#ifndef CONFIG_USER_ONLY
> + if (len == 0) {
> + return false;
> + }
> +
> + g_byte_array_set_size(data, len);
> + cpu_physical_memory_rw(addr, (void *)data->data, len, 0);
One concern here is that cpu_physical_memory_* swallows any error
conditions so the user might not get what they are expecting even if we
return true here.
It would be safer I think to use address_space_read_full with
&address_space_memory and MEMTXATTRS_UNSPECIFIED and check the result so
we can signal to the user when it fails.
However that does gloss over some details because you can have multiple
address spaces. As each vCPU can potentially have its own maybe we want:
g_assert(current_cpu);
res = address_space_read_full(current_cpu->as, addr, attrs, buf, len);
return res == MEMTX_OK ? true : false;
However even that elides a complexity because a CPU can have multiple
AddressSpaces depending on what mode it is in.
For example Arm can have normal, secure and potentially normal and
secure versions of tag memory. Currently we have no way of exposing that
information to plugins (and maybe we don't want to, currently
arm_addressspace() is only used for internal page table walking and some
stack stuff).
x86 has two potential address spaces with the extra one being used for
I think System Management Mode.
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> {
> g_assert(current_cpu);
> @@ -534,6 +550,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
> }
>
> +
> struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
> {
> return plugin_scoreboard_new(element_size);
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index ca773d8d9f..5d9cfd71bb 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -20,6 +20,8 @@
> qemu_plugin_num_vcpus;
> qemu_plugin_outs;
> qemu_plugin_path_to_binary;
> + qemu_plugin_read_cpu_memory_hwaddr;
> + qemu_plugin_read_io_memory_hwaddr;
> qemu_plugin_read_register;
> qemu_plugin_register_atexit_cb;
> qemu_plugin_register_flush_cb;
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 8+ messages in thread