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

[Alex Bennée]

Aaron Lindsay OS <address@hidden> writes:

> 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.

Only if the API is misused and a call made outside of the hooked memory
operation. An assert would be too strong as the plugin could then bring
down QEMU - I guess we could use some sort of error_report...

>
>> +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?

It might be easier to just keep a __thread local array here and let TLS
deal with it.

--
Alex Bennée