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

[Frederic Konrad]

On 02/03/2017 06:26 PM, Paolo Bonzini wrote:
> 
> 
> On 03/02/2017 09:06, address@hidden 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.

Hi Paolo,

Thanks for your comment.
Yes, I read in the docs that dynamically dropping MemoryRegions is badly
broken when we use NULL as an owner because the machine owns it.
But it seems nothing said this is the case with an owner.

But I think I see your point here:
  * memory_region_unref will unref the owner.
  * object_unparent will unref the memory region (which should be 1).
  => the region will be dropped immediately.

Doesn't hotplug use dynamic MemoryRegion? In which case we better
make that work with MTTCG. I wonder if we can't simply handle that
with a safe_work for this case?

BTW the tests I have seems to pass without issues.

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

I'm not aware of this MemoryRegionCache yet, it seems pretty new.
I'll take a look.

Thanks,
Fred

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