| Message ID | 1486141597-13941-5-git-send-email-fred.konrad@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + } > +}
On 02/03/2017 06:26 PM, 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. 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 > >> + } >> +}
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 > > > + } > > +}
On 03/02/2017 13:09, Frederic Konrad wrote: > On 02/03/2017 06:26 PM, 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. > > 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? Hot-unplug works because the backing memory is only freed when the device gets instance_finalize, so at that point the region cannot have any references. What can go wrong is the following (from the docs): - the memory region's owner had a reference taken via memory_region_ref (for example by address_space_map) - the region is unparented, and has no owner anymore - when address_space_unmap is called, the reference to the memory region's owner is leaked. In your case you have phys_section_add/phys_section_destroy instead of address_space_map/unmap, but the problem is the same. I am a bit unsure about using the same lqspi_buf for caching different addresses, and about using the same buffer for MMIO execution and read. What if you allocate a different buffer every time lqspi_request_mmio_ptr is called (adding a char* argument to lqspi_load_cache) and free the old one from the safe_work item, after a global TLB flush? Then you don't need the Notifiers which were the complicated and handwavy part of my proposal. Paolo > > 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 >> >>> + } >>> +} > > >
On 02/04/2017 01:41 PM, Paolo Bonzini wrote: > ... >> >> 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? > > Hot-unplug works because the backing memory is only freed when the > device gets instance_finalize, so at that point the region cannot have > any references. > > What can go wrong is the following (from the docs): > > - the memory region's owner had a reference taken via memory_region_ref > (for example by address_space_map) > > - the region is unparented, and has no owner anymore > > - when address_space_unmap is called, the reference to the memory > region's owner is leaked. true. > > In your case you have phys_section_add/phys_section_destroy instead of > address_space_map/unmap, but the problem is the same. > > I am a bit unsure about using the same lqspi_buf for caching different > addresses, and about using the same buffer for MMIO execution and read. > What if you allocate a different buffer every time > lqspi_request_mmio_ptr is called (adding a char* argument to > lqspi_load_cache) and free the old one from the safe_work item, after a > global TLB flush? Then you don't need the Notifiers which were the > complicated and handwavy part of my proposal. Actually this was just because it was the way xilinx_qspi worked. It load 1K of SPI data and fill the buffer. In some other case we don't want to free the pointer at all, just make it not accessible for execution / read. (BTW I'll add the read-only property to this). What about a simple Object eg: mmio-execution-interface which has a MemoryRegion? Instead of creating the MemoryRegion in request_pointer, We create a mmio-execution-interface object which create and map the region on the subregion. Then in invalidate: we unplug the mmio-execution-interface, unref it and it is freed all by magic when the map doesn't need it. This avoid the reference issue and probably the bug with MTTCG. Fred > > Paolo > >> >> 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 >>> >>>> + } >>>> +} >> >> >>
On 02/04/2017 02:59 PM, Frederic Konrad wrote: > On 02/04/2017 01:41 PM, Paolo Bonzini wrote: >> > ... >>> >>> 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? >> >> Hot-unplug works because the backing memory is only freed when the >> device gets instance_finalize, so at that point the region cannot have >> any references. >> >> What can go wrong is the following (from the docs): >> >> - the memory region's owner had a reference taken via memory_region_ref >> (for example by address_space_map) >> >> - the region is unparented, and has no owner anymore >> >> - when address_space_unmap is called, the reference to the memory >> region's owner is leaked. > > true. > >> >> In your case you have phys_section_add/phys_section_destroy instead of >> address_space_map/unmap, but the problem is the same. >> >> I am a bit unsure about using the same lqspi_buf for caching different >> addresses, and about using the same buffer for MMIO execution and read. >> What if you allocate a different buffer every time >> lqspi_request_mmio_ptr is called (adding a char* argument to >> lqspi_load_cache) and free the old one from the safe_work item, after a >> global TLB flush? Then you don't need the Notifiers which were the >> complicated and handwavy part of my proposal. > > Actually this was just because it was the way xilinx_qspi worked. > It load 1K of SPI data and fill the buffer. > > In some other case we don't want to free the pointer at all, just make > it not accessible for execution / read. > (BTW I'll add the read-only property to this). > > What about a simple Object eg: mmio-execution-interface which has a > MemoryRegion? Instead of creating the MemoryRegion in request_pointer, > We create a mmio-execution-interface object which create and map the > region on the subregion. > Then in invalidate: we unplug the mmio-execution-interface, unref it and > it is freed all by magic when the map doesn't need it. > > This avoid the reference issue and probably the bug with MTTCG. Does that make sense? Thanks, Fred > > Fred > >> >> Paolo >> >>> >>> 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 >>>> >>>>> + } >>>>> +} >>> >>> >>> > >
diff --git a/cputlb.c b/cputlb.c index 846341e..9077247 100644 --- a/cputlb.c +++ b/cputlb.c @@ -545,6 +545,13 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr) if (memory_region_is_unassigned(mr)) { CPUClass *cc = CPU_GET_CLASS(cpu); + if (memory_region_request_mmio_ptr(mr, addr)) { + /* A MemoryRegion is potentially added so re-run the + * get_page_addr_code. + */ + return get_page_addr_code(env, addr); + } + if (cc->do_unassigned_access) { cc->do_unassigned_access(cpu, addr, false, true, 0, 4); } else { diff --git a/include/exec/memory.h b/include/exec/memory.h index 987f925..36b0eec 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -120,6 +120,15 @@ struct MemoryRegionOps { uint64_t data, unsigned size, MemTxAttrs attrs); + /* Instruction execution pre-callback: + * @addr is the address of the access relative to the @mr. + * @size is the size of the area returned by the callback. + * @offset is the location of the pointer inside @mr. + * + * Returns a pointer to a location which contains guest code. + */ + void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size, + unsigned *offset); enum device_endian endianness; /* Guest-visible constraints: */ @@ -1253,6 +1262,32 @@ void memory_global_dirty_log_stop(void); void mtree_info(fprintf_function mon_printf, void *f, bool flatview); /** + * memory_region_request_mmio_ptr: request a pointer to an mmio + * MemoryRegion. If it is possible map a RAM MemoryRegion with this pointer. + * When the device wants to invalidate the pointer it will call + * memory_region_invalidate_mmio_ptr. + * + * @mr: #MemoryRegion to check + * @addr: address within that region + * + * Returns true on success, false otherwise. + */ +bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr); + +/** + * memory_region_invalidate_mmio_ptr: invalidate the pointer to an mmio + * previously requested. + * In the end that means that if something wants to execute from this area it + * will need to request the pointer again. + * + * @mr: #MemoryRegion associated to the pointer. + * @addr: address within that region + * @size: size of that area. + */ +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, + unsigned size); + +/** * memory_region_dispatch_read: perform a read directly to the specified * MemoryRegion. * diff --git a/memory.c b/memory.c index 6c58373..eb3e8ec 100644 --- a/memory.c +++ b/memory.c @@ -2375,6 +2375,51 @@ void memory_listener_unregister(MemoryListener *listener) QTAILQ_REMOVE(&listener->address_space->listeners, listener, link_as); } +bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr) +{ + void *host; + unsigned size = 0; + unsigned offset = 0; + MemoryRegion *sub; + + if (!mr || !mr->ops->request_ptr) { + return false; + } + + /* + * Avoid an update if the request_ptr call + * memory_region_invalidate_mmio_ptr which seems to be likely when we use + * a cache. + */ + memory_region_transaction_begin(); + + 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)); + } +} + void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) { memory_region_ref(root);