Re: [Qemu-devel] [PATCH v6 19/33] dimm: keep the state of the whole backend memory

[Xiao Guangrong]

On 11/09/2015 07:04 PM, Michael S. Tsirkin wrote:
> On Fri, Oct 30, 2015 at 01:56:13PM +0800, Xiao Guangrong wrote:
> > QEMU keeps the state of memory of dimm device during live migration,
> > however, it is not enough for nvdimm device as its memory does not
> > contain its label data, so that we should protect the whole backend
> > memory instead
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
>
> It looks like there's now a difference between
> host_memory_backend_get_memory and get_memory_region,
> whereas previously they were exactly interchangeable.

Yes, it is. host_memory_backend_get_memory() is used to get the whole
backend memory region. get_memory_region() is used to get the memory region
which will be mapped to guest's address space. They are on the same page
for pc-dimm, but it's different for nvdimm since part of backend memory used
as label data which is not directly mapped to guest.

> This needs some thought, in particular the theoretically
> generic dimm.c has to do tricks to accomodate nvdimm.
>
> The missing piece for NVDIMM is the 128k label space at the end,
> right?  Can't nvdimm specific code just register that as a
> separate RAM chunk in order to migrate it?

Yes, it is.

I thought this before, then we need to have addition callbackes:
- plug(), which is called when the dimm device is plugged, then nvdimm
  has the chance to do it own migration things.

- unplug(), let nvdimm undo the thing of migration.

It needs more codes but the logic seems more clear. If you like this way,
i will do.

> > ---
> >   hw/mem/dimm.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/mem/dimm.c b/hw/mem/dimm.c
> > index 498d380..44447d1 100644
> > --- a/hw/mem/dimm.c
> > +++ b/hw/mem/dimm.c
> > @@ -134,9 +134,16 @@ void dimm_memory_plug(DeviceState *dev, MemoryHotplugState 
> > *hpms,
> >       }
> >
> >       memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > -    vmstate_register_ram(mr, dev);
> >       numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> >
> > +    /*
> > +     * save the state only for @mr is not enough as it does not contain
> > +     * the label data of NVDIMM device, so that we keep the state of
> > +     * whole hostmem instead.
> > +     */
> > +    vmstate_register_ram(host_memory_backend_get_memory(dimm->hostmem, errp),
> > +                         dev);
> > +
> >   out:
> >       error_propagate(errp, local_err);
> >   }
> > @@ -145,10 +152,13 @@ void dimm_memory_unplug(DeviceState *dev, 
> > MemoryHotplugState *hpms,
> >                              MemoryRegion *mr)
> >   {
> >       DIMMDevice *dimm = DIMM(dev);
> > +    MemoryRegion *backend_mr;
> > +
> > +    backend_mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> >
> >       numa_unset_mem_node_id(dimm->addr, memory_region_size(mr), dimm->node);
> >       memory_region_del_subregion(&hpms->mr, mr);
> > -    vmstate_unregister_ram(mr, dev);
> > +    vmstate_unregister_ram(backend_mr, dev);
> >   }
> >
> >   int qmp_dimm_device_list(Object *obj, void *opaque)
> > --
> > 1.8.3.1