On Fri, Feb 03, 2017 at 09:26:19AM -0800, Paolo Bonzini wrote:
>
>
> On 03/02/2017 09:06, fred.kon...@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
>
> > + }
> > +}
