Re: [Qemu-devel] [PATCH v4 24/54] plugins: implement helpers for resolving hwaddr

[Aaron Lindsay OS via Qemu-devel <qemu-devel@nongnu.org>]

On Jul 31 17:06, Alex Bennée 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.

[...]

> +bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
> +                       bool is_store, struct qemu_plugin_hwaddr *data)
> +{
> +    CPUArchState *env = cpu->env_ptr;
> +    CPUTLBEntry *tlbe = tlb_entry(env, mmu_idx, addr);
> +    target_ulong tlb_addr = is_store ? tlb_addr_write(tlbe) : tlbe->addr_read;
> +
> +    if (tlb_hit(tlb_addr, addr)) {
> +        if (tlb_addr & TLB_MMIO) {
> +            data->hostaddr = 0;
> +            data->is_io = true;
> +            /* XXX: lookup device */
> +        } else {
> +            data->hostaddr = addr + tlbe->addend;
> +            data->is_io = false;
> +        }
> +        return true;
> +    }
> +    return false;
> +}

In what cases do you expect tlb_hit() should not evaluate to true here?
Will returns of false only be in error cases, or do you expect it can
occur during normal operation? In particular, I'm interested in ensuring
this is as reliable as possible, since some plugins may require physical
addresses.

> +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;
> +    struct qemu_plugin_hwaddr *hwaddr;
> +
> +    /* Ensure we have memory allocated for this work */
> +    if (!hwaddr_refs) {
> +        hwaddr_refs = g_array_sized_new(false, true,
> +                                        sizeof(struct qemu_plugin_hwaddr),
> +                                        cpu->cpu_index + 1);
> +    } else if (cpu->cpu_index >= hwaddr_refs->len) {
> +        hwaddr_refs = g_array_set_size(hwaddr_refs, cpu->cpu_index + 1);
> +    }

Are there one or more race conditions with the allocations here? If so,
could they be solved by doing the allocations at plugin initialization
and when the number of online cpu's changes, instead of lazily?

>  uint64_t qemu_plugin_hwaddr_to_raddr(const struct qemu_plugin_hwaddr *haddr)

I was at first confused about the utility of this function until I
(re-?)discovered you had to convert first to hwaddr and then raddr to
get a "true" physical address. Perhaps that could be added to a comment
here or in the API definition in the main plugin header file.

-Aaron