Re: [Qemu-devel] [PULL v1 0/7] MMIO Exec pull request

[Dr. David Alan Gilbert]

* KONRAD Frederic (address@hidden) wrote:
> 
> 
> On 07/20/2017 12:02 PM, Dr. David Alan Gilbert wrote:
> > * Peter Maydell (address@hidden) wrote:
> > > On 17 July 2017 at 19:58, Dr. David Alan Gilbert <address@hidden> wrote:
> > > > * Edgar E. Iglesias (address@hidden) wrote:
> > > > > Is there a way we can prevent migration of the RAMBlock?
> > > > 
> > > > Not yet, I think we'd have to:
> > > >     a) Add a flag to the RAMBlock
> > > >     b) Set it/clear it on registration
> > > >     c) Have a RAMBLOCK_FOREACH_MIGRATABLE macro
> > > >     d) Replace all of the RAMBLOCK_FOREACH (and the couple of hand coded
> > > >     cases) with the RAMBLOCK_FOREACH_MIGRATABLE
> > > >     e) Worry about the corner cases!
> > > > 
> > > > I've got a few worries about what happens when the kernel tries to
> > > > do dirty yncing - I'm not sure if we have to change anything on that
> > > > interface to skip those RAMBlocks.
> > > 
> > > OK, so what should we do for 2.10 ?
> > > 
> > > We could:
> > >   * implement the changes you suggest above, and mark only
> > >     vmstate_register_ram'd blocks as migratable
> > >     (would probably need to fix some places which buggily
> > >     don't call vmstate_register_ram)
> > >   * implement the changes above, but special case mmio-interface
> > >     so only its ramblock is marked unmigratable
> > 
> > I think either of these is too late for 2.10 - I don't fancy prodding
> > about in all of the migration RAM loops at this stage.
> > 
> > >   * postpone the changes above until 2.11, and for 2.10 register
> > >     a migration-blocker in mmio-interface so that we at least
> > >     give the user a useful error rather than having it fail
> > >     obscurely on vmload (and release note this)
> > 
> > I think that's best, especially because I've just thought of another nasty.
> > If I understand the way mmio-interface is working, you're dynamically
> > changing the RAMBlock list while the guest is running.
> > And while we are using QLIST_FOREACH_RCU I'm not convinced we're
> > actually safe against dynamic modification of that list.
> 
> Hi Dave,
> 
> Hmmm I'm not totally convinced..
> 
> Let's imagine somebody (eg: u-boot guest) wants to execute code
> from the LQSPI area.
> 
> memory_region_request_mmio_ptr is called (the guest is not
> running yet) it will create a mmio-interface which create the
> ram memory region with a pointer provided by the device.
> Then the guest can execute code from that and this somehow is
> breaking migration because this ram memory region is migrated.
> 
> Now something is writing to the device.. The cache is no longer
> valid, we have to drop this mmio-interface. This is done in an
> async_safe work so cpu are stopped at this moment. So I think we
> are safe for that.

I don't fully understand the way memory_region_do_invalidate_mmio_ptr
works; I see it dropping the memory region; if that's also dropping
the RAMBlock then it will upset migration.   Even if the CPU is stopped
I dont think that stops the migration thread walking through the list of
RAMBlocks.
Even then, the problem is migration keeps a 'dirty_pages' count which is
calculated at the start of migration and updated as we dirty and send
pages; if we add/remove a RAMBlock then that dirty_pages count is wrong
and we either never finish migration (since dirty_pages never reaches
zero) or finish early with some unsent data.
And then there's the 'received' bitmap currently being added for
postcopy which tracks each page that's been received (that's not in yet
though).

Dave

> > 
> > > (Or something else?)
> > > 
> > > I do think we definitely need to fix this for 2.11 at latest.
> > 
> > OK, I can do a-e OK, I'm more worried now about that dynamic
> > modification I just thought of.
> > 
> 
> What's an a-e?
> 
> BTW sorry for this pain :( I didn't figure out that the
> ram MemoryRegion would be migrated..
> 
> Thanks,
> Fred
> 
> > Dave
> > 
> > > thanks
> > > -- PMM
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK