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/03/2015 12:16 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 02.11.2015 18:06, Xiao Guangrong wrote:
>>
>>
>> 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.
>>
> Ok, then it should be documented by a comment in dimm.h, where DIMMDeviceClass is defined, that this
> function should not fail
>

Okay, how about this comment:

     /*
      * get the memory region which will be mapped into guest's address
      * space. It is called after dimm device realized so it is never
      * failed.
      */
     MemoryRegion *(*get_memory_region)(DIMMDevice *dimm);