From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1bb5-0000AL-3O for qemu-devel@nongnu.org; Tue, 01 Nov 2016 12:07:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1baz-00080S-04 for qemu-devel@nongnu.org; Tue, 01 Nov 2016 12:07:35 -0400 Received: from mga02.intel.com ([134.134.136.20]:3054) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c1bay-000804-Mo for qemu-devel@nongnu.org; Tue, 01 Nov 2016 12:07:28 -0400 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> <20161101162505.57172707@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: Date: Wed, 2 Nov 2016 00:00:12 +0800 MIME-Version: 1.0 In-Reply-To: <20161101162505.57172707@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed 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: Igor Mammedov , Stefan Hajnoczi Cc: 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 11/01/2016 11:25 PM, Igor Mammedov wrote: > 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(). > Thank you, Igor, Michael and Stefan. I will do it asap after this patchset is merged. :)