Re: [Qemu-devel] [PULL 37/47] nvdimm acpi: introduce fit buffer

[Igor Mammedov]

On Mon, 31 Oct 2016 17:52:24 +0800
Xiao Guangrong <address@hidden> wrote:

> On 10/31/2016 05:45 PM, Igor Mammedov wrote:
> > On Sun, 30 Oct 2016 23:25:05 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >  
> >> From: Xiao Guangrong <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Reviewed-by: Michael S. Tsirkin <address@hidden>
> >> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >> ---
> >>  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.