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

[KONRAD Frederic]

On 07/19/2017 06:46 PM, Dr. David Alan Gilbert wrote:
> * KONRAD Frederic (address@hidden) wrote:
> > On 07/19/2017 02:29 PM, Dr. David Alan Gilbert wrote:
> > > * KONRAD Frederic (address@hidden) wrote:
> > > > On 07/17/2017 07:27 PM, Edgar E. Iglesias wrote:
> > > > > On Mon, Jul 17, 2017 at 11:33 PM, Peter Maydell <address@hidden>
> > > > > wrote:
> > > > >
> > > > > > On 14 June 2017 at 18:45, Edgar E. Iglesias <address@hidden>
> > > > > > wrote:
> > > > > > > From: "Edgar E. Iglesias" <address@hidden>
> > > > > > > Paolo suggested offline that we send a pull request for this series.
> > > > > > > Here it is, I've run it through my testsuite + tested the LQSPI testcase
> > > > > > > on Zynq.
> > > > > >
> > > > > > > ----------------------------------------------------------------
> > > > > > > mmio-exec.for-upstream
> > > > > > >
> > > > > > > ----------------------------------------------------------------
> > > > > > > KONRAD Frederic (7):
> > > > > > >          cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT
> > > > > > >          cputlb: move get_page_addr_code
> > > > > > >          cputlb: fix the way get_page_addr_code fills the tlb
> > > > > > >          qdev: add MemoryRegion property
> > > > > > >          introduce mmio_interface
> > > > > > >          exec: allow to get a pointer for some mmio memory region
> > > > > > >          xilinx_spips: allow mmio execution
> > > > > >
> > > > > > Hi Edgar -- can you or Fred explain how this code interacts with
> > > > > > VM migration? The mmio-interface device creates a RAM memory
> > > > > > region with memory_region_init_ram_ptr(), but it doesn't call
> > > > > > vmstate_register_ram(). On the other hand the core migration code
> > > > > > will try to migrate the contents of the RAMBlock anyway, just
> > > > > > without a name.
> > > > > >
> > > > > > It's not clear to me how this works, and it would be nice to
> > > > > > get it clear so that we can make any necessary fixes before the
> > > > > > 2.10 release hits and we lose the opportunity to make any
> > > > > > migration-compatibility-breaking changes.
> > > > > >
> > > > > > thanks
> > > > > > -- PMM
> > > > >
> > > > > Hi Peter,
> > > > >
> > > > > These temporary regions should be read-only and treated as temporary caches
> > > > > AFAIU things.
> > > > > I would say that they don't need to be migrated. After migration, the new
> > > > > VM will recreate the ram areas from device backing.
> > > > >
> > > > > Is there a way we can prevent migration of the RAMBlock?
> > > > >
> > > > > Cheers,
> > > > > Edgar
> > > >
> > > > Hi All,
> > > >
> > > > Yes Edgar is right, they don't need to be migrated (as the old
> > > > Xilinx SPI cache).
> > > > And it will be the case for all the other stuff using this as
> > > > well.
> > > >
> > > > Maybe we can simply drop the region before the migration?
> > >
> > > Dont forget the VM is still running during migration, so you
> > > can't drop anything that's needed for it to be able to execute.
> > >
> > > Dave
> >
> > Scary.. Yes your right. I still think it doesn't need to be
> > migrated.. But how..
>
> One question about the way your device works; being a cache, is
> there something bad that the guest could do which would make
> a reload into the cache fail - i.e the real program could
> be reliant on running out of data in the cache?
> i.e. could the state of the cache actually be system state
> which really does need migrating.
> (e.g. reprogram the flash via another route or something?)
>
> (I've seen real code rely on an ARM TLB like this).

It can be reprogrammed through the SPI device.. At this point the
MemoryRegion is dropped and after the migration when an
instruction fetch happen on this area the cache will be recreated 
and filled with new data from the drive. So I don't think it is
an issue especially for this SPI device.

Fred
> > BTW taking a look at this I found the comment below which seems
> > to implies that the content won't be migrated?
> >
> > /**
> >
> >   * memory_region_init_ram_ptr:  Initialize RAM memory region from a
> >   *                              user-provided pointer.  Accesses into the
> >   *                              region will modify memory directly.
> >   *
> >
> >   * @mr: the #MemoryRegion to be initialized.
> >
> >   * @owner: the object that tracks the region's reference count
> >
> >   * @name: Region name, becomes part of RAMBlock name used in migration
> > stream
> >   *        must be unique within any device
> >
> >   * @size: size of the region.
> >
> >   * @ptr: memory to be mapped; must contain at least @size bytes.
> >
> >   *
> >
> >   * Note that this function does not do anything to cause the data in the
> >   * RAM memory region to be migrated; that is the responsibility of the
> > caller.
> >   */
> >
> > And I guess we have the same issue when we try to unplug a device
> > during migration?
>
> plug/unplugging during a migration is a bad idea; I've sene suggestions
> some of the code is safe for bits of it; I'm not convinced.
>
> Dave
>
> > Fred
> >
> > > > Fred
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK