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

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 14 Oct 2019 at 12:25, Alex Bennée <alex.bennee@linaro.org> 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 ?

Aren't hwaddr's unique across the bus? Or is this because you would have
two address buses (secure and non-secure) with different address lines to
different chips?

But surely we have all the information we need because we've hooked the
two things that QEMU's softmmu code knows. The mmu_idx and the vaddr
with which the slow path can figure out what it needs.

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

Hmm maybe.. I guess it's a special case of device offset. Do you want to
drop this part for now?

>
> thanks
> -- PMM


--
Alex Bennée