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

[Frederic Konrad]

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