From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Sementsov-Ogievskiy Subject: Re: [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region Date: Thu, 5 Nov 2015 11:53:14 +0300 Message-ID: <563B18FA.3050506@virtuozzo.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> <56377C10.9080803@linux.intel.com> <56378C44.7060601@virtuozzo.com> <5638C8E8.9090804@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , , , , , , , To: Xiao Guangrong , , Return-path: Received: from relay.parallels.com ([195.214.232.42]:53511 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1032626AbbKEIxg (ORCPT ); Thu, 5 Nov 2015 03:53:36 -0500 In-Reply-To: <5638C8E8.9090804@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 03.11.2015 17:47, Xiao Guangrong wrote: > > > 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); if you don't mind: s/it is never failed/it should never fail and assumed to return valid not-NULL address I'll ok with this if others don't mind, but personally I prefer explicit error handling for such functions. -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50752) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuGIf-0006HT-CD for qemu-devel@nongnu.org; Thu, 05 Nov 2015 03:53:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuGIb-0004Mm-6e for qemu-devel@nongnu.org; Thu, 05 Nov 2015 03:53:41 -0500 Received: from relay.parallels.com ([195.214.232.42]:34880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuGIa-0004L4-Uc for qemu-devel@nongnu.org; Thu, 05 Nov 2015 03:53:37 -0500 Message-ID: <563B18FA.3050506@virtuozzo.com> Date: Thu, 5 Nov 2015 11:53:14 +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> <56378C44.7060601@virtuozzo.com> <5638C8E8.9090804@linux.intel.com> In-Reply-To: <5638C8E8.9090804@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 03.11.2015 17:47, Xiao Guangrong wrote: > > > 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); if you don't mind: s/it is never failed/it should never fail and assumed to return valid not-NULL address I'll ok with this if others don't mind, but personally I prefer explicit error handling for such functions. -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.