Re: [PATCH v5 17/55] plugins: implement helpers for resolving hwaddr

[Peter Maydell]

On Mon, 14 Oct 2019 at 12:25, Alex Bennée <address@hidden> wrote:
>
> We need to keep a local per-cpu copy of the data as other threads may
> be running. We use a automatically growing array and re-use the space
> for subsequent queries.
>


> +#ifdef CONFIG_SOFTMMU
> +static __thread struct qemu_plugin_hwaddr hwaddr_info;
> +
> +struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
> +                                                  uint64_t vaddr)
> +{
> +    CPUState *cpu = current_cpu;
> +    unsigned int mmu_idx = info >> TRACE_MEM_MMU_SHIFT;
> +
> +    if (!tlb_plugin_lookup(cpu, vaddr, mmu_idx,
> +                           info & TRACE_MEM_ST, &hwaddr_info)) {
> +        error_report("invalid use of qemu_plugin_get_hwaddr");
> +        return NULL;
> +    }
> +
> +    return &hwaddr_info;
> +}

Apologies for dropping into the middle of this patchset, but
this API looks a bit odd. A hwaddr alone isn't a complete
definition of an access -- you need an (AddressSpace, hwaddr)
tuple for that. So this API looks like it doesn't really cope
with things like TrustZone ?

>  uint64_t qemu_plugin_hwaddr_to_raddr(const struct qemu_plugin_hwaddr *haddr)
>  {
> +#ifdef CONFIG_SOFTMMU
> +    ram_addr_t ram_addr = 0;
> +
> +    if (haddr && !haddr->is_io) {
> +        ram_addr = qemu_ram_addr_from_host((void *) haddr->hostaddr);
> +        if (ram_addr == RAM_ADDR_INVALID) {
> +            error_report("Bad ram pointer %"PRIx64"", haddr->hostaddr);
> +            abort();
> +        }
> +    }
> +    return ram_addr;
> +#else
>      return 0;
> +#endif
>  }

This looks odd to see in the plugin API -- ramaddrs should
be a QEMU internal concept, shouldn't they?

thanks
-- PMM