From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtG4I-0006ee-EB for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:26:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtG4F-0007tA-6v for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:26:42 -0500 Received: from relay.parallels.com ([195.214.232.42]:36226) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtG4E-0007qH-Ue for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:26:39 -0500 Message-ID: <5637727C.8050205@virtuozzo.com> Date: Mon, 2 Nov 2015 17:26: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> In-Reply-To: <56376042.4090002@linux.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit 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 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). >>> >>> Signed-off-by: Xiao Guangrong >>> --- >>> hw/mem/dimm.c | 3 ++- >>> hw/mem/pc-dimm.c | 12 +++++++++++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> 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); >>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >>> index 38323e9..e6b6a9f 100644 >>> --- a/hw/mem/pc-dimm.c >>> +++ b/hw/mem/pc-dimm.c >>> @@ -22,7 +22,17 @@ >>> 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; >>> } >> >> this should squashed into previous patch, I think > > You mean merger this patch with 19/35 (dimm: abstract dimm device from > pc-dimm)? > The 19/35 mostly ‘moves’ the things, this one changes the core logic, > it is not > a big deal. :D stupid me, you are right) > >> >>> static void pc_dimm_class_init(ObjectClass *oc, void *data) >> >> I've discovered suddenly, that >> >> MemoryRegion * >> host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp) >> { >> return memory_region_size(&backend->mr) ? &backend->mr : NULL; >> } >> >> - it doesn't use errp at all. In my opinion, this should be fixed >> globally, by deleting useless >> parameter in separate patch. Or just squash your function into >> previous patch. >> > > Yup, this is a globally interface so i prefer to make a separate patch > to do the > cleanup after this patchset. > > -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.