From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHmT-0000HZ-CX for qemu-devel@nongnu.org; Mon, 02 Nov 2015 11:16:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtHmQ-00008C-Lc for qemu-devel@nongnu.org; Mon, 02 Nov 2015 11:16:25 -0500 Received: from relay.parallels.com ([195.214.232.42]:48707) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHmQ-000085-Cv for qemu-devel@nongnu.org; Mon, 02 Nov 2015 11:16:22 -0500 Message-ID: <56378C44.7060601@virtuozzo.com> Date: Mon, 2 Nov 2015 19:16:04 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 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> In-Reply-To: <56377C10.9080803@linux.intel.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: Xiao Guangrong , 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 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 -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.