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

[Igor Mammedov]

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.

> >>>> 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.

> 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
  }