From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fVBes-0001ll-CE for qemu-devel@nongnu.org; Tue, 19 Jun 2018 04:06:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fVBen-0002Bd-BR for qemu-devel@nongnu.org; Tue, 19 Jun 2018 04:06:34 -0400 References: <20180618142536.23995-1-david@redhat.com> <20180618142536.23995-11-david@redhat.com> <20180619085448.65e009a1@redhat.com> From: David Hildenbrand Message-ID: Date: Tue, 19 Jun 2018 10:06:19 +0200 MIME-Version: 1.0 In-Reply-To: <20180619085448.65e009a1@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 10/12] nvdimm: convert nvdimm_mr into a pointer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Eduardo Habkost , "Michael S . Tsirkin" , Marcel Apfelbaum , Paolo Bonzini , Richard Henderson , Xiao Guangrong , David Gibson , Alexander Graf On 19.06.2018 08:54, Igor Mammedov wrote: > On Mon, 18 Jun 2018 16:25:34 +0200 > David Hildenbrand wrote: > >> This way we can easily check if the region has already been inititalized >> without having to rely on the size of an uninitialized region being 0. >> >> Reviewed-by: David Gibson >> Reviewed-by: Igor Mammedov >> Signed-off-by: David Hildenbrand >> --- >> hw/mem/nvdimm.c | 9 +++++---- >> include/hw/mem/nvdimm.h | 2 +- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c >> index 7260c9c6b1..db7d8c3050 100644 >> --- a/hw/mem/nvdimm.c >> +++ b/hw/mem/nvdimm.c >> @@ -43,7 +43,7 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name, >> Error *local_err = NULL; >> uint64_t value; >> >> - if (memory_region_size(&nvdimm->nvdimm_mr)) { >> + if (nvdimm->nvdimm_mr) { >> error_setg(&local_err, "cannot change property value"); >> goto out; >> } >> @@ -75,7 +75,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) >> { >> NVDIMMDevice *nvdimm = NVDIMM(dimm); >> >> - return &nvdimm->nvdimm_mr; >> + return nvdimm->nvdimm_mr; >> } >> >> static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) >> @@ -102,9 +102,10 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) >> return; >> } >> >> - memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm), >> + nvdimm->nvdimm_mr = g_new(MemoryRegion, 1); > missed leak, > you need to free this manually in unrealize() in this patch but > considering that the next patches would move this allocation before > realize, it has to be move to finalize(). So make a note in commit > message explaining why finalize() is used prematurely. Very right, thanks. finalize() seems to be the right spot. > > probably no need to respin whole series, just send v5 of this patch > as reply here if change doesn't break series. > >> + memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm), >> "nvdimm-memory", mr, 0, pmem_size); >> - nvdimm->nvdimm_mr.align = align; >> + nvdimm->nvdimm_mr->align = align; >> } >> >> /* >> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h >> index 9340631cfc..c5c9b3c7f8 100644 >> --- a/include/hw/mem/nvdimm.h >> +++ b/include/hw/mem/nvdimm.h >> @@ -74,7 +74,7 @@ struct NVDIMMDevice { >> * it's the PMEM region in NVDIMM device, which is presented to >> * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported. >> */ >> - MemoryRegion nvdimm_mr; >> + MemoryRegion *nvdimm_mr; >> >> /* >> * The 'on' value results in the unarmed flag set in ACPI NFIT, > -- Thanks, David / dhildenb