Re: [Qemu-devel] [RFC PATCH qemu 3/4] memory: Share flat views and dispatch trees between address spaces

[Alexey Kardashevskiy]

On 12/09/17 17:12, Paolo Bonzini wrote:
> On 12/09/2017 07:55, Alexey Kardashevskiy wrote:
>> On 12/09/17 01:30, Paolo Bonzini wrote:
>>> On 11/09/2017 14:08, Alexey Kardashevskiy wrote:
>>>>> Ok, this makes sense.  Maybe it should be a flatview rather than an
>>>>> AddressSpaceDispatch (a FlatView is essentially a list of
>>>>> MemoryRegionSections; attaching the ASD to the FlatView is more or less
>>>>> an implementation detail).
>>>> The helpers I converted from AddressSpace to AddressSpaceDispatch do use
>>>> dispatch structure only and do not use FlatView so it seemed logical.
>>>
>>> Understood, but from a design POV FlatView makes more sense.
>>>
>>>> btw this address_space in MemoryRegionSection - it does not seem to make
>>>> much sense in the PhysPageMap::sections array, it only makes sense when
>>>> MemoryRegionSection uses as a temporary object when calling listeners. Will
>>>> it make sense if we enforce MemoryRegionSection::address_space to be NULL
>>>> in the array and not NULL when used temporary?
>>>
>>> memory_region_section_get_iotlb needs to access the AddressSpaceDispatch
>>> for sections stored in the PhysPageMap array, because
>>> memory_region_section_get_iotlb uses the ASD to compute the section index.
>>
>> Ohhh, not extremely trivial, out of curiosity - is that iotlb encoding
>> described anywhere?
> 
> No, I don't think so.
> 
>> Anyway, this can be simplified (or rather made more straightforward?) -
>> tlb_set_page_with_attrs() can calculate the section index and pass it to
>> memory_region_section_get_iotlb(). Still does not make much sense? It just
>> looks quite useless to keep that address_space pointer alive just for one
>> case which can easily avoid using this pointer.
> 
> Hmm I suppose address_space_translate_for_iotlb knows the ASD and could
> also return the index, basically combining it and
> memory_region_section_get_iotlb() into one function.

Ok, good.

One more question - how do we decide what goes to exec.c and what goes to
memory.c? I have a temptation to simply embed AddressSpaceDispatch into
FlatView instead of allocating it and storing the pointer (I'll probably
avoid doing so for now anyway but still curios).

The header comment for exec.c says it is "Virtual page mapping" but
AddressSpaceDispatch is a physical address space map and it seems to fit
into the memory.c's "Physical memory management" header comment.


-- 
Alexey