Re: [PATCH v3 5/8] Add memory hardware address read/write API

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

On 5/21/25 8:34 PM, Rowan Hart wrote:
> Well, first I just noticed that I left a debug print in this function!
> So I'll fix that.
> 
>> Reading this patch, and patch 3 (Add address space API), I am not sure
>> AddressSpace is something we want to leak in plugins interface.
>> It is a concept *very* internal to QEMU, and not reflecting directly
>> something concerning the emulated architecture (it is related, but not
>> officially described for it).
>>
>> The same way qemu_plugin_write_memory_vaddr is only valid in the
>> current page table setup, we could assume the same for current address
>> space, and return an error if memory is not mapped with current AS.
>> Eventually, we could read/write a given hwaddr in all existing address
>> spaces (starting with current mapped one), if it makes sense to do
>> this, which I'm not sure about.
>>
>> What are your thoughts on this?
> 
> I definitely see the arguments for not exposing it even as an opaque
> struct, internality not withstanding it also adds some complexity for
> plugin authors.
> 
> My thought with exposing it is kind of twofold. First, there are
> specific address spaces like the secure address space on ARM or I/O
> memory on x86 that plugins might like to access and I think it's easiest
> to facilitate that if we just let them choose which one they want to r/w
> to. Second, I wanted to lean towards being less restrictive now to avoid
> having to go back and remove restrictions later since even though it's
> very internal, it doesn't seem very likely to change.
> 
> That said, if you think it's more trouble than it's worth I'm totally
> fine with just defaulting to writing to the current AS (or to
> cpu-memory, whichever's more reasonable). Your call, just let me know
> which way you think is best for v4 :)

I understand your point, but to the opposite of registers, I think we 
should refrain from exposing all this.
For now, we can just use the current AS.

Later, we could consider to add a new architecture specific parameter 
for that need (being a union, with fields per base architecture). So 
people writing architecture specific plugins can have a solution.

AddressSpace as = {.arm = ADDRESS_SPACE_ARM_SECURE};
qemu_plugin_read_memory_hwaddr_as(addr, data, as);

>> qemu_plugin_translate_vaddr is fine for me.
> I did have a question about this -- one of the test plugins prints out
> vaddr, haddr from qemu_plugin_insn_haddr, and the translated haddr from
> qemu_plugin_translate_vaddr. When running with direct memory mappings in
> a system test, the vaddr = translated haddr, which is correct, but the
> haddr from qemu_plugin_insn_haddr was incorrect (it was 0x7f....f<actual
> address>). Is this expected behavior?
>

qemu_plugin_insn_haddr returns directly a pointer to instruction in 
(host) memory, which is different from hwaddr, thus the void* signature.
Name is pretty confusing though, qemu_plugin_insn_ptr could have been a 
better name.

> Thanks for the feedback!
> 
>