Re: [Qemu-devel] [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region

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

On 11/02/2015 10:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 02.11.2015 16:08, Xiao Guangrong wrote:
>>
>>
>> On 11/02/2015 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> On 02.11.2015 12:13, Xiao Guangrong wrote:
>>>> Curretly, the memory region of backed memory is directly mapped to
>>>> guest's address space, however, it is not true for nvdimm device
>>>>
>>>> This patch let dimm device realize this fact and use
>>>> DIMMDeviceClass->get_memory_region method to get the mapped memory
>>>> region
>>>>
>>>> Current code did not check the return value of get_memory_region as it
>>>> assumed the backend memory of pc-dimm is always properly initialized,
>>>> we make get_memory_region internally catch the case if something is
>>>> wrong
>
> but here you call not pc-dimm's get_memory_region, but common ddc->get_memory_region, which may be
> nvdimm or possibly other future dimm, so, why not check it here? And than pc_dimm_get_memory_region
> may be left untouched (error_abort is ok, because errp is unused).

Hmm, because 'here' is not the only place calling ->get_memory_region, this method has
multiple callers:

$ git grep "\->get_memory_region"
hw/i386/pc.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);
hw/i386/pc.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);
hw/mem/dimm.c:    mr = ddc->get_memory_region(dimm);
hw/mem/nvdimm.c:    ddc->get_memory_region = nvdimm_get_memory_region;
hw/mem/pc-dimm.c:    ddc->get_memory_region = pc_dimm_get_memory_region;
hw/ppc/spapr.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);

memory region validation is also done for NVDIMM in nvdimm device.