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