[RFC,10/15] memory: Rework sub-page handling

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.

For the record, I attach the patch I was using to fix Jan's.

Paolo

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

On 2013-05-07 19:26, Jan Kiszka wrote:
> 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?

OK, there is no translation caching in the memory core. So I will
preserve the dispatching functions of sub-pages, just like the term
"sub-page" - along with a comment why we depend on page granularity.

Jan

> On 2013-05-07 19:26, Jan Kiszka wrote:
> > 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?
> 
> OK, there is no translation caching in the memory core. So I will
> preserve the dispatching functions of sub-pages, just like the term
> "sub-page" - along with a comment why we depend on page granularity.

Note that TCG will cache translations because the TLB entry is filled
with the page after translation has taken place. In fact, this is the
main change from Avi's series to mine, and a side-effect of centralizing
the translation in address_space_translate.

It shouldn't be a problem though, the TLB entry will point to the subpage
and the compiled code will dispatch to it.

Paolo