From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztcy9-0002OJ-Sz for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:53:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ztcy6-0004MN-N9 for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:53:53 -0500 Received: from mga14.intel.com ([192.55.52.115]:9827) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztcy6-0004M5-GZ for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:53:50 -0500 References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com> <1446455617-129562-21-git-send-email-guangrong.xiao@linux.intel.com> <563754D5.2060401@virtuozzo.com> <56376042.4090002@linux.intel.com> <5637727C.8050205@virtuozzo.com> <56377C10.9080803@linux.intel.com> <56378C44.7060601@virtuozzo.com> From: Xiao Guangrong Message-ID: <5638C8E8.9090804@linux.intel.com> Date: Tue, 3 Nov 2015 22:47:04 +0800 MIME-Version: 1.0 In-Reply-To: <56378C44.7060601@virtuozzo.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , pbonzini@redhat.com, imammedo@redhat.com Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net 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);