Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Paolo Bonzini <pbonzini@redhat.com>]

On 03/02/2017 09:06, fred.konrad@greensocs.com wrote:
> +    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
> +
> +    if (!host || !size) {
> +        memory_region_transaction_commit();
> +        return false;
> +    }
> +
> +    sub = g_new(MemoryRegion, 1);
> +    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
> +    memory_region_add_subregion(mr, offset, sub);
> +    memory_region_transaction_commit();
> +    return true;
> +}
> +
> +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
> +                                       unsigned size)
> +{
> +    MemoryRegionSection section = memory_region_find(mr, offset, size);
> +
> +    if (section.mr != mr) {
> +        memory_region_del_subregion(mr, section.mr);
> +        /* memory_region_find add a ref on section.mr */
> +        memory_region_unref(section.mr);
> +        object_unparent(OBJECT(section.mr));

I think this would cause a use-after-free when using MTTCG.  In general,
creating and dropping MemoryRegions dynamically can cause bugs that are
nondeterministic and hard to fix without rewriting everything.

An alternative design could be:

- memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
a pointer, so that the device can map a subset of the device (e.g. a
single page)

- memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
a Notifier

- the device adds the Notifier to a NotifierList.  Before invalidating,
it invokes the Notifier and empties the NotifierList.

- for the TLB case, the Notifier calls tlb_flush_page.

I like the general idea though!

Paolo

> +    }
> +}