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

[Julian Ganz <neither@nut.email>]

Hi Rowan,

> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index eff8430b4a..d4f229abd9 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -1014,6 +1014,102 @@ QEMU_PLUGIN_API
>  bool qemu_plugin_write_memory_vaddr(uint64_t addr,
>                                     GByteArray *data);
>  
> <snip>
> +/**
> + * qemu_plugin_translate_vaddr() - translate a virtual address to a physical one
> + *
> + * @vaddr: virtual address to translate
> + * @hwaddr: pointer to store the physical address
> + *
> + * This function is only valid in vCPU context (i.e. in callbacks) and is only
> + * valid for softmmu targets.
> + *
> + * Returns true on success and false on failure.
> + */
> +QEMU_PLUGIN_API
> +bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr);

As a (testing) plugin author, I'm quite interested in this function in
particular. For the next iteration, would you be willing to split this
one out into its own patch? This would enhance the chance of it getting
picked up even if other parts of the series should not make it.

> diff --git a/plugins/api.c b/plugins/api.c
> index 19c10bb39e..5983768783 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -569,6 +569,106 @@ bool qemu_plugin_write_memory_vaddr(uint64_t addr, GByteArray *data)
> <snip> 
> +bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    g_assert(current_cpu);
> +
> +    CPUState *cpu = current_cpu;
> +
> +    uint64_t res = cpu_get_phys_page_debug(cpu, vaddr);
> +
> +    if (res == (uint64_t)-1) {
> +        return false;
> +    }
> +
> +    *hwaddr = res | (vaddr & ~TARGET_PAGE_MASK);
> +
> +    return true;
> +#else
> +    return false;
> +#endif
> +}

This definition strikes me as odd. What was your reason to assert
`current_cpu` here, but not in the other two functions? Also a bit
surprising is the declaration of `cpu` if you use it in just one place
(rather than just use `current_cpu` directly as for the assertion).

And there is no reason in particular why the vCPU could not be a
function parameter of `qemu_plugin_translate_vaddr`, right? You don't
have the same restrictions as in `qemu_plugin_read_memory_hwaddr` or
`qemu_plugin_hwaddr_operation_result` where you actually touch memory?

Regards,
Julian