From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45214) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1ATL-00006n-Ty for qemu-devel@nongnu.org; Mon, 31 Oct 2016 07:09:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1ATG-0001w8-8H for qemu-devel@nongnu.org; Mon, 31 Oct 2016 07:09:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39358) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c1ATG-0001vn-03 for qemu-devel@nongnu.org; Mon, 31 Oct 2016 07:09:42 -0400 Date: Mon, 31 Oct 2016 12:09:37 +0100 From: Igor Mammedov Message-ID: <20161031120937.36d000b5@nial.brq.redhat.com> In-Reply-To: <46de99b0-8845-b324-1e5c-06b737fb45a6@linux.intel.com> References: <1477850917-1214-1-git-send-email-mst@redhat.com> <1477850917-1214-38-git-send-email-mst@redhat.com> <20161031104518.7e423640@nial.brq.redhat.com> <46de99b0-8845-b324-1e5c-06b737fb45a6@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 37/47] nvdimm acpi: introduce fit buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: "Michael S. Tsirkin" , Peter Maydell , Richard Henderson , qemu-devel@nongnu.org, Eduardo Habkost , Paolo Bonzini On Mon, 31 Oct 2016 17:52:24 +0800 Xiao Guangrong wrote: > On 10/31/2016 05:45 PM, Igor Mammedov wrote: > > On Sun, 30 Oct 2016 23:25:05 +0200 > > "Michael S. Tsirkin" wrote: > > > >> From: Xiao Guangrong > >> > >> 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 Could you explain why lock is needed? > >> 2) mark @dirty whenever the buffer is updated. > >> > >> @dirty is cleared for the first time OSPM gets fit buffer, if > >> dirty is detected in the later access, OSPM will restart the > >> access > >> > >> As fit should be updated after nvdimm device is successfully realized > >> so that a new hotplug callback, post_hotplug, is introduced > >> > >> Signed-off-by: Xiao Guangrong > >> Reviewed-by: Michael S. Tsirkin > >> Signed-off-by: Michael S. Tsirkin > >> --- > >> include/hw/hotplug.h | 10 +++++++++ > >> include/hw/mem/nvdimm.h | 26 +++++++++++++++++++++- > >> hw/acpi/nvdimm.c | 59 +++++++++++++++++++++++++++++++++++-------------- > >> hw/core/hotplug.c | 11 +++++++++ > >> hw/core/qdev.c | 20 +++++++++++++---- > >> hw/i386/acpi-build.c | 2 +- > >> hw/i386/pc.c | 19 ++++++++++++++++ > >> 7 files changed, 124 insertions(+), 23 deletions(-) > >> > >> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h > >> index c0db869..10ca5b6 100644 > >> --- a/include/hw/hotplug.h > >> +++ b/include/hw/hotplug.h > >> @@ -47,6 +47,7 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler, > >> * @parent: Opaque parent interface. > >> * @pre_plug: pre plug callback called at start of device.realize(true) > >> * @plug: plug callback called at end of device.realize(true). > >> + * @post_pug: post plug callback called after device is successfully plugged. > > this doesn't seem to be necessary, > > why hotplug_handler_plug() isn't sufficient? > > At the point of hotplug_handler_plug(), the device is not realize (realized == 0), > however, nvdimm_get_plugged_device_list() works on realized nvdimm devices. I suggest instead of adding redundant hook, to reuse hotplug_handler_plug() which is called after device::realize() successfully completed and amend nvdimm_plugged_device_list() as follow: diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index e486128..c6d8cbb 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -38,11 +38,7 @@ static int nvdimm_plugged_device_list(Object *obj, void *opaque) GSList **list = opaque; if (object_dynamic_cast(obj, TYPE_NVDIMM)) { - DeviceState *dev = DEVICE(obj); - - if (dev->realized) { /* only realized NVDIMMs matter */ - *list = g_slist_append(*list, DEVICE(obj)); - } + *list = g_slist_append(*list, obj); } object_child_foreach(obj, nvdimm_plugged_device_list, opaque); that should count not only already present nvdimms but also the one that's being hotplugged. > And we can not move "realized = 1" to the front of hotplug_handler_plug() as device > realize routines will check if realized is already been true.