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

[Frederic Konrad]

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