Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-05-07 14:35, Paolo Bonzini wrote:
> Il 06/05/2013 22:46, Peter Maydell ha scritto:
>> On 6 May 2013 15:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> Simplify the sub-page handling by implementing it directly in the
>>> dispatcher instead of using a redirection memory region. We extend the
>>> phys_sections entries to optionally hold a pointer to the sub-section
>>> table that used to reside in the subpage_t structure. IOW, we add one
>>> optional dispatch level below the existing radix tree.
>>>
>>> address_space_lookup_region is extended to take this additional level
>>> into account. This direct dispatching to that target memory region will
>>> also be helpful when we want to add per-region locking control.
>>
>> This patch seems to break vexpress-a9. Test case if you want it:
>> http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
>> (125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb
>> and then run it; before this patch it boots, afterwards it doesn't
>> even manage to start the kernel.
>>
>> My guess is you've broken subregion-sized mmio regions somehow
>> (and/or regions which are larger than a page in size but start
>> or finish at a non-page-aligned address), and probably in particular
>> the arm_gic regions that a9mpcore maps...
> 
> I think we just found out what all the subpage stuff is for. :)
> 
> When I added address_space_translate(), the trickiest conversion to the
> new API was in tlb_set_page.  Hence I added a "you never know"-style assert:
> 
>     assert(size >= TARGET_PAGE_SIZE);
>     if (size != TARGET_PAGE_SIZE) {
>         tlb_add_large_page(env, vaddr, size);
>      }
> -    section = phys_page_find(address_space_memory.dispatch,
> -			     paddr >> TARGET_PAGE_BITS);
> +    sz = size;
> +    section = address_space_translate(&address_space_memory, paddr,
> +				      &xlat, &sz, false);
> +    assert(sz >= TARGET_PAGE_SIZE);
> 
> 
> Now, remember that address_space_translate clamps sz on output to the
> size of the section.  And here's what happens:
> 
> (gdb) p sz
> $4 = 256
> (gdb) p *(section->mr)
> $5 = {ops = 0x7ffff82d33c0, iommu_ops = 0x0, opaque = 0x7ffff91e6b50,
> parent = 0x7ffff9069400, owner = 0x0, size = {lo = 256,
>     hi = 0}, addr = 0, destructor = 0x7ffff7e824d0
> <memory_region_destructor_none>, ram_addr = 18446744073709551615,
> terminates =
>     true, romd_mode = true, ram = false, readonly = false, enabled =
> true, rom_device = false, warning_printed = false,
>   flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, priority
> = 0, may_overlap = false, subregions = {tqh_first = 0x0,
>     tqh_last = 0x7ffff91e8ee8}, subregions_link = {tqe_next = 0x0,
> tqe_prev = 0x7ffff91e64f8}, coalesced = {tqh_first = 0x0,
>     tqh_last = 0x7ffff91e8f08}, name = 0x7ffff906bb60 "a9-scu",
> dirty_log_mask = 0 '\000', ioeventfd_nb = 0, ioeventfds = 0x0,
>   iommu_target_as = 0x0}
> 
> The TLB would only store this region instead of the whole page, and
> subsequent access past the first 256 bytes would be emulated incorrectly.

Good catch. Hmm, and a tricky issue. So we need to preserve the
dispatching read/write handler of sub-pages, and a reference to the
sub-page would continue to end up in the TCG TLBs. But reference
counting of the memory regions that page may point to becomes hairy.

Conceptually, we would have to increment the counter for all regions a
sub-page refers to. Even worse, regions may be added to the sub-page
while it is referenced by some user. Doesn't work.

Well, the alternative is to handle a sub-page dispatch (ie. calling into
subpage_[ram_]read/write just like address_space_rw: take the necessary
lock that protect mapping changes, look into the sub-page and pick up
the target region, invoke memory_region_ref on it, perform the access
and unref the region again. Slow, but that's how sub-pages are. And it
only affects TCG. Hmm, or does your IOMMU core cache translations on a
per-page base as well?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux