Re: [Qemu-devel] [PATCH v2 0/8] acpi: make ROMs resizeable

[Michael S. Tsirkin]

On Thu, Dec 18, 2014 at 08:16:26PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
> > On Thu, Dec 18, 2014 at 06:49:09PM +0000, Dr. David Alan Gilbert wrote:
> > > * Michael S. Tsirkin (address@hidden) wrote:
> 
> > > I'm generally happy with this set for what you're using it for,
> > > except that I'd like some big hairy warnings in comments
> > > near the resize functions to make it clear when it's safe
> > > to do it.
> > 
> > Really always: whenever resize callback updates all
> > guest visible state.
> 
> The outgoing side worries me due to previous painful encounters
> with arch_init.c's migration_dirty_pages.  It's a counter that
> knows exactly how many pages it has and it gets incremented/decremented
> and *must* be right otherwise the migration never ends.
> It's derived from used_pages in your world, so that would change.

Yes but we handle it correctly:
+    cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
+    block->used_length = newsize;
+    cpu_physical_memory_set_dirty_range(block->offset, block->used_length);
so the counter will be updated.

> That's just the one that comes to mind;
> A comment saying 'Don't use during an outgoing migration'
> would cover that - but I'm not that confident there aren't other uses
> we have to be careful of.
> 
> > > What I don't really understand is how it would work for anything mapped
> > > into guest address space, how that mapping would be updated.
> > > 
> > > Dave
> > 
> > That would be a job of the resize function: it can update kvm.
> 
> Yes I guess so, I'm just not sure what the guest is going to think about it.
> 
> Dave

I get it.  I think the correct requirement is this:

/*  Only legal before guest might have detected the memory size: e.g. on
 *  incoming migration, or right after reset.
 *  As memory core doesn't know how is memory accessed, it is up to
 *  resize callback to add assertions to detect misuse, if necessary.
 */

Would this comment address your concerns?

> > 
> > > > 
> > > > Michael S. Tsirkin (8):
> > > >   cpu: add cpu_physical_memory_clear_dirty_range_nocode
> > > >   memory: add memory_region_set_size
> > > >   exec: cpu_physical_memory_set/clear_dirty_range
> > > >   exec: split length -> used_length/max_length
> > > >   exec: qemu_ram_alloc_resizeable, qemu_ram_resize
> > > >   arch_init: support resizing on incoming migration
> > > >   memory: API to allocate resizeable RAM MR
> > > >   acpi-build: make ROMs RAM blocks resizeable
> > > > 
> > > >  hw/lm32/lm32_hwsetup.h  |   3 +-
> > > >  include/exec/cpu-all.h  |  12 +++--
> > > >  include/exec/memory.h   |  34 +++++++++++++
> > > >  include/exec/ram_addr.h |  22 +++++++--
> > > >  include/hw/loader.h     |   4 +-
> > > >  arch_init.c             |  28 ++++++-----
> > > >  exec.c                  | 129 
> > > > +++++++++++++++++++++++++++++++++++++-----------
> > > >  hw/core/loader.c        |  18 +++++--
> > > >  hw/i386/acpi-build.c    |  19 +++++--
> > > >  memory.c                |  33 +++++++++++++
> > > >  10 files changed, 242 insertions(+), 60 deletions(-)
> > > > 
> > > > -- 
> > > > MST
> > > > 
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK