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

[Xiao Guangrong]

On 11/01/2016 06:35 PM, Igor Mammedov wrote:
> On Tue, 1 Nov 2016 11:30:46 +0800
> Xiao Guangrong <address@hidden> wrote:
>
> > On 10/31/2016 07:09 PM, Igor Mammedov wrote:
> > > 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?
> >
> > Yes. These are two things need to be synced between QEMU and OSPM:
> > - fit buffer. QEMU products it and VM will consume it at the same time.
> > - dirty indicator. QEMU sets it and it will be cleared by OSPM, that means.
> >    both sides will modify it.
>
> I understand why 'dirty indicator' is necessary but
> could you describe a concrete call flows in detail
> that would cause concurrent access and need extra
> lock protection.
>
> Note that there is global lock (dubbed BQL) in QEMU,
> which is taken every time guest accesses IO port or
> QMP/monitor control event happens.

Ah. okay. If there is a lock to protect vm-exit handler and
monitor handler, i think it is okay to drop the lock.

> > > > > > 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.
> >
> > It is not good as the device which failed to initialize
> See device_set_realized()
>         [...]
>         if (dc->realize) {
>             dc->realize(dev, &local_err);
>         }
>
>         if (local_err != NULL) {
>             goto fail;
>         }
>
>         DEVICE_LISTENER_CALL(realize, Forward, dev);
>
>         if (hotplug_ctrl) {
>             hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
>         }
>
> i.e. control reaches to hotplug_handler_plug() only if
> call dc->realize(dev, &local_err) is successful.

I mean, for example. if there are two devices, the first one is failed to be
realized, the second one is init-ed successfully, then can
nvdimm_plugged_device_list() get these two devices?

Based the code of pc_dimm_built_list(), i guess yes.

> > or is not being plugged
> > (consider two nvdimm devices directly appended in QEMU command line, when we 
> > plug
> > the first one, both nvdimm devices will be counted in this list) is also 
> > counted in
> > this list...
> nope,
> qdev_device_add() is called sequentially (one time for each -device/device_add)
> so nvdimm_plugged_device_list() sees only devices that's been added
> by previous -device/device_add plus one extra device that's
> being added by in progress qdev_device_add() call.
>
> so for "-device nvdimm,id=nv1 -device nvdimm,id=2" call sequence
> would look like:
> 1:
>   qdev_device_add("nvdimm,id=nv1") {
>      -> set parent // device becomes visible to nvdimm_get_plugged_device_list()
>      // this is the only time where device->realized == false
>      // could be observed, all other call chains would either
>      // see device->realized == true or not see device at all
>      -> realize()
>            -> device_set_realized()
>               -> nvdimm->realize()
>               -> hotplug_handler_plug()
>                       nvdimm_get_plugged_device_list()
>                          // would return list with 1 item (nv1)
>                          // where nv1->realized == false)
>
>   }
> 2:
>   qdev_device_add("nvdimm,id=nv2") {
>      -> set parent // device becomes visible to nvdimm_get_plugged_device_list()
>      -> realize()
>            -> device_set_realized()
>               -> nvdimm->realize()
>               -> hotplug_handler_plug()
>                       nvdimm_get_plugged_device_list()
>                          // would return list with 2 items (nv1 and nv2)
>                          // where nv1->realized == true and nv2->realized = 
> false
>   }

Okay.