From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region Date: Mon, 2 Nov 2015 23:06:56 +0800 Message-ID: <56377C10.9080803@linux.intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, eblake@redhat.com To: Vladimir Sementsov-Ogievskiy , pbonzini@redhat.com, imammedo@redhat.com Return-path: Received: from mga09.intel.com ([134.134.136.24]:11369 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753841AbbKBPNl (ORCPT ); Mon, 2 Nov 2015 10:13:41 -0500 In-Reply-To: <5637727C.8050205@virtuozzo.com> Sender: kvm-owner@vger.kernel.org List-ID: 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.