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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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 <guangrong.xiao@linux.intel.com>
>
> 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
>