From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44521) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1aw5-000531-51 for qemu-devel@nongnu.org; Tue, 01 Nov 2016 11:25:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1aw2-0003n8-0f for qemu-devel@nongnu.org; Tue, 01 Nov 2016 11:25:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51394) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c1aw1-0003ma-Pn for qemu-devel@nongnu.org; Tue, 01 Nov 2016 11:25:09 -0400 Date: Tue, 1 Nov 2016 16:25:05 +0100 From: Igor Mammedov Message-ID: <20161101162505.57172707@nial.brq.redhat.com> In-Reply-To: <20161101151701.GC5707@stefanha-x1.localdomain> References: <1477672540-27952-1-git-send-email-guangrong.xiao@linux.intel.com> <1477672540-27952-3-git-send-email-guangrong.xiao@linux.intel.com> <20161101151701.GC5707@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Xiao Guangrong , pbonzini@redhat.com, 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 On Tue, 1 Nov 2016 15:17:01 +0000 Stefan Hajnoczi wrote: > On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote: > > The buffer is used to save the FIT info for all the presented nvdimm > > devices which is updated after the nvdimm device is plugged or > > unplugged. In the later patch, it will be used to construct NVDIMM > > ACPI _FIT method which reflects the presented nvdimm devices after > > nvdimm hotplug > > > > As FIT buffer can not completely mapped into guest address space, > > OSPM will exit to QEMU multiple times, however, there is the race > > condition - FIT may be changed during these multiple exits, so that > > some rules are introduced: > > 1) the user should hold the @lock to access the buffer and > > I don't understand the purpose of the QEMUMutex lock. Don't all threads > calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu > threads)? > > > -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, > > +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf) > > +{ > > + qemu_mutex_init(&fit_buf->lock); > > + fit_buf->fit = g_array_new(false, true /* clear */, 1); > > Is it possible to call nvdimm_build_device_structure() here? That way > we don't duplicate the g_array_new() details. > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 5783442..d835e62 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -945,10 +945,21 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > > goto child_realize_fail; > > } > > } > > + > > if (dev->hotplugged) { > > device_reset(dev); > > } > > dev->pending_deleted_event = false; > > + dev->realized = value; > > + > > + if (hotplug_ctrl) { > > + hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err); > > + } > > + > > + if (local_err != NULL) { > > + dev->realized = value; > > dev->realized = value was already set above. > > In order to preserve semantics you would need a bool old_value = > dev->realized which you can restore in post_realize_fail. QEMU current > does not assign dev->realized = value when there is a failure! Stefan, as agreed on "Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread this series would be merged as is and then as follow up author will fixup all issues with it. For this patch it means a patch on top of Michael pull req to drop lock and drop hotplug_handler_post_plug() code in favor of existing hotplug_handler_plug().