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 21:08:18 +0800 Message-ID: <56376042.4090002@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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE 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 mga02.intel.com ([134.134.136.20]:10017 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750856AbbKBNPC (ORCPT ); Mon, 2 Nov 2015 08:15:02 -0500 In-Reply-To: <563754D5.2060401@virtuozzo.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> >> 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 =3D DIMM(obj); >> + DIMMDeviceClass *ddc =3D DIMM_GET_CLASS(obj); >> - mr =3D host_memory_backend_get_memory(dimm->hostmem, errp); >> + mr =3D ddc->get_memory_region(dimm); >> value =3D 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_abo= rt); >> + Error *local_err =3D NULL; >> + MemoryRegion *mr; >> + >> + mr =3D 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 =E2=80=98moves=E2=80=99 the things, this one changes t= he core logic, it is not a big deal. :D > >> static void pc_dimm_class_init(ObjectClass *oc, void *data) > > I've discovered suddenly, that > > MemoryRegion * > host_memory_backend_get_memory(HostMemoryBackend *backend, Error **er= rp) > { > return memory_region_size(&backend->mr) ? &backend->mr : NULL; > } > > - it doesn't use errp at all. In my opinion, this should be fixed glo= bally, by deleting useless > parameter in separate patch. Or just squash your function into previo= us patch. > Yup, this is a globally interface so i prefer to make a separate patch = to do the cleanup after this patchset.