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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Fri, Feb 03, 2017 at 09:26:19AM -0800, Paolo Bonzini wrote:
> 
> 
> 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.

Interesting! I totally missed the MemoryRegionCache patches. Cool concept.
I few lines about it in docs/memory.txt would have been nice too :-)


> I like the general idea though!

I think it's genial. I was expecting a solution for this to get ugly...
It solves some existing issues for us (like the QSPI one addressed in this series).
It also paves the way for certain use-cases when co-simulating with SystemC.
Nice one Fred!

Cheers,
Edgar



> 
> Paolo
> 
> > +    }
> > +}