Re: [RFC v5 00/86] Memory API

Avi Kivity
2011-07-21T08:37:05Z

On 07/21/2011 12:43 AM, Jan Kiszka wrote:
> On 2011-07-20 19:43, Avi Kivity wrote:
> > On 07/20/2011 08:41 PM, Jan Kiszka wrote:
> >> On 2011-07-20 18:49, Avi Kivity wrote:
> >> > New in this version:
> >> > - more mindless conversions; I believe there are no longer any
> >> destructive
> >> > operations in the tree (IO_MEM_UNASSIGNED)
> >> > - fix memory map generation bug (patch 13)
> >> > - proper 440FX PAM/SMRAM and PCI holes
> >> >
> >>
> >> This on top fixes standard VGA dirty logging:
> >
> > Both work for me without any patches.
>
> Impossible! ;)
>
> VGA frame buffer cannot work as no one enabled dirty logging for that
> range so far. Try -vga std with vga=0x314 in the guest.
>

Right, actually booting into X showed that. But I don't understand how
it worked before - my patches only change how vga_start_dirty_log() is
implemented, not when/where it is called.

> >
> > Maybe the F15 window manager is polling the display?
> >
>
> Only if that continuously enforces a full window refresh.

Turned out to be a tester issue.

> As expected, there were dirty logging issues around removing a
> subregion on cirrus bank pointer updates. This makes linear vram
> mappings work again:

Please sign off patches!

> diff --git a/memory.c b/memory.c
> index a8d4295..14fac8a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1093,9 +1093,26 @@ void
> memory_region_add_subregion_overlap(MemoryRegion *mr,
> void memory_region_del_subregion(MemoryRegion *mr,
> MemoryRegion *subregion)
> {
> + MemoryRegion *target_region;
> + ram_addr_t base, offs;
> +
> assert(subregion->parent == mr);
> subregion->parent = NULL;
> QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
> +
> + if (subregion->alias) {
> + base = subregion->alias_offset;
> + target_region = subregion->alias;
> + } else {
> + base = 0;
> + target_region = subregion;
> + }
> + if (target_region->dirty_log_mask) {
> + for (offs = 0; offs< subregion->size; offs += TARGET_PAGE_SIZE) {
> + memory_region_set_dirty(target_region, base + offs);
> + }
> + }
> +

The subregion may be partially or fully obstructed. This needs to be
done at the FlatRange level (as_memory_range_del(), most likely).

> memory_region_update_topology();
> }
>
>
> Debugging provided some more insights:
> - address_space_update_topology is not very successful in avoiding
> needless mapping updates. kvm_client_set_memory is called much more
> frequently than with the old code.
> - The region update pattern delete old / add new, e.g. to move the
> cirrus bank pointer, will never allow an optimal number of memory
> client calls. We either need some memory_region_update or a
> transaction API. I would favor the former, should also simplify the
> usage.

I had the same thoughts. In-place updates will get more and more
complicated, though. Perhaps we can do a memory_region_replace() (_del
and _add)? That can atomically replace both banks in one go.

Note you can emulate _replace() by adding a new region with a higher
priority, then removing the old one underneath.

> - memory_region_update_topology should take a hint if all or just a
> specific address space need updating.

It's a nice optimization but I want to defer that to later (when
memory.c actually supersedes ram_addr_t).

> - Something makes the startup of graphical grub under cirrus horribly
> slow, likely some bug that prevents linear vram mode during the
> screen setup. But once it is fully painted for the first time, grub
> feels as fast as with the old code.

I haven't seen that. How slow is slow? maybe my eyes aren't fast enough.

--
error compiling committee.c: too many arguments to function