Aaron Lindsay OS <aa...@os.amperecomputing.com> 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
