From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60227) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtBPW-0004SM-N8 for qemu-devel@nongnu.org; Mon, 02 Nov 2015 04:28:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtBPR-0006eR-Ui for qemu-devel@nongnu.org; Mon, 02 Nov 2015 04:28:18 -0500 Received: from mga09.intel.com ([134.134.136.24]:60034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtBPR-0006dv-Oj for qemu-devel@nongnu.org; Mon, 02 Nov 2015 04:28:13 -0500 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-19-git-send-email-guangrong.xiao@linux.intel.com> <56349D6E.8010803@virtuozzo.com> <5634CCEB.4070004@linux.intel.com> From: Xiao Guangrong Message-ID: <56372A61.5070807@linux.intel.com> Date: Mon, 2 Nov 2015 17:18:25 +0800 MIME-Version: 1.0 In-Reply-To: <5634CCEB.4070004@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 18/33] 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 10/31/2015 10:15 PM, Xiao Guangrong wrote: > > > On 10/31/2015 06:52 PM, Vladimir Sementsov-Ogievskiy wrote: >> On 30.10.2015 08:56, 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 >>> >>> Signed-off-by: Xiao Guangrong >>> --- >>> hw/mem/dimm.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/mem/dimm.c b/hw/mem/dimm.c >>> index 4a63409..498d380 100644 >>> --- a/hw/mem/dimm.c >>> +++ b/hw/mem/dimm.c >>> @@ -377,8 +377,9 @@ static void dimm_get_size(Object *obj, Visitor *v, void *opaque, >>> int64_t value; >>> MemoryRegion *mr; >>> DIMMDevice *dimm = DIMM(obj); >>> + DIMMDeviceClass *ddc = DIMM_GET_CLASS(obj); >>> - mr = host_memory_backend_get_memory(dimm->hostmem, errp); >>> + mr = ddc->get_memory_region(dimm); >>> value = memory_region_size(mr); >>> visit_type_int(v, &value, name, errp); >> >> Isn't it better to add Error** parameter to get_memory_region and not mix returning error in errp >> parameter and aborting through &error_abort? >> > > Yes, it's better, will do. Thanks for your suggestion. Vladimir, After checking the current code, the @errp in pc-dimm's get_memory_region() is useless and all its caller did not check the return value as they assumed the backend memory of pc-dimm is always properly initialized. So we catch the error condition internally instead of propagating it to the caller: static MemoryRegion *pc_dimm_get_memory_region(DIMMDevice *dimm) { - return host_memory_backend_get_memory(dimm->hostmem, &error_abort); + Error *local_err = NULL; + MemoryRegion *mr; + + mr = host_memory_backend_get_memory(dimm->hostmem, &local_err); + + /* + * plug a pc-dimm device whose backend memory was not properly + * initialized? + */ + assert(!local_err && mr); + return mr; } please review it in the new version i have CCed you. Thanks for your review.