On Mon, 31 Oct 2016 17:52:24 +0800
Xiao Guangrong <guangrong.x...@linux.intel.com> wrote:
> On 10/31/2016 05:45 PM, Igor Mammedov wrote:
> > On Sun, 30 Oct 2016 23:25:05 +0200
> > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >
> >> From: Xiao Guangrong <guangrong.x...@linux.intel.com>
> >>
> >> 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 <guangrong.x...@linux.intel.com>
> >> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> >> ---
> >> 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.
