| Message ID | 4E274C06.40902@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On 2011-07-21 10:37, Avi Kivity wrote: > 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. To answer this question as well: You dropped all the vga_start_dirty_log originally performed during PCI mapping. > > >> > >> > 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! Always - once they are done. > >> 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). Makes some sense. I even wonder if this isn't a KVM deficit and should be handled there when a logged region is unmapped. Jan
On 07/21/2011 04:38 PM, Jan Kiszka wrote: > On 2011-07-21 10:37, Avi Kivity wrote: > > 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. > > To answer this question as well: You dropped all the vga_start_dirty_log > originally performed during PCI mapping. > Ah, I see it now, in vga_map(). Thanks. > >> @@ -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). > > Makes some sense. I even wonder if this isn't a KVM deficit and should > be handled there when a logged region is unmapped. What do you mean? There is a known issue with kvm here, this is a just workaround.
On 2011-07-21 15:43, Avi Kivity wrote: >>>> @@ -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). >> >> Makes some sense. I even wonder if this isn't a KVM deficit and should >> be handled there when a logged region is unmapped. > > What do you mean? There is a known issue with kvm here, this is a just > workaround. Then the logic indeed belongs to kvm-all.c, not memory.c. Jan
On 07/21/2011 05:19 PM, Jan Kiszka wrote: > >> Makes some sense. I even wonder if this isn't a KVM deficit and should > >> be handled there when a logged region is unmapped. > > > > What do you mean? There is a known issue with kvm here, this is a just > > workaround. > > Then the logic indeed belongs to kvm-all.c, not memory.c. > Does kvm-all.c have all the information? btw, where do you see the issue? which guest? Somehow you see a lot more problems than I do.
On 2011-07-21 16:31, Avi Kivity wrote: > On 07/21/2011 05:19 PM, Jan Kiszka wrote: >>>> Makes some sense. I even wonder if this isn't a KVM deficit and should >>>> be handled there when a logged region is unmapped. >>> >>> What do you mean? There is a known issue with kvm here, this is a just >>> workaround. >> >> Then the logic indeed belongs to kvm-all.c, not memory.c. >> > > Does kvm-all.c have all the information? If should as it keeps a record of the active slots with the logging flag. So, if a slot with logging on is changed, the dirty bits should be set. > > btw, where do you see the issue? which guest? > > Somehow you see a lot more problems than I do. Stock OpenSUSE 11.4 with grub2 in graphical mode, -vga cirrus. Jan
On 07/21/2011 05:34 PM, Jan Kiszka wrote: > On 2011-07-21 16:31, Avi Kivity wrote: > > On 07/21/2011 05:19 PM, Jan Kiszka wrote: > >>>> Makes some sense. I even wonder if this isn't a KVM deficit and should > >>>> be handled there when a logged region is unmapped. > >>> > >>> What do you mean? There is a known issue with kvm here, this is a just > >>> workaround. > >> > >> Then the logic indeed belongs to kvm-all.c, not memory.c. > >> > > > > Does kvm-all.c have all the information? > > If should as it keeps a record of the active slots with the logging > flag. So, if a slot with logging on is changed, the dirty bits should be > set. > Ok. We'd need to teach it about the memory API as well - there is no longer a global ram_addr_t that can be used. But that's not too hard. > > > > btw, where do you see the issue? which guest? > > > > Somehow you see a lot more problems than I do. > > Stock OpenSUSE 11.4 with grub2 in graphical mode, -vga cirrus. Ah, I use regular grub. Will try g2.
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); + } + } + memory_region_update_topology(); }