Re: [Qemu-devel] [PATCH v3 2/4] nvdimm acpi: introduce fit buffer

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 1 Nov 2016 15:17:01 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Sat, Oct 29, 2016 at 12:35:38AM +0800, Xiao Guangrong wrote:
> > 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  
> 
> I don't understand the purpose of the QEMUMutex lock.  Don't all threads
> calling nvdimm/acpi code hold the QEMU global mutex (main loop and vcpu
> threads)?
> 
> > -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> > +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> > +{
> > +    qemu_mutex_init(&fit_buf->lock);
> > +    fit_buf->fit = g_array_new(false, true /* clear */, 1);  
> 
> Is it possible to call nvdimm_build_device_structure() here?  That way
> we don't duplicate the g_array_new() details.
> 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 5783442..d835e62 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -945,10 +945,21 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >                  goto child_realize_fail;
> >              }
> >          }
> > +
> >          if (dev->hotplugged) {
> >              device_reset(dev);
> >          }
> >          dev->pending_deleted_event = false;
> > +        dev->realized = value;
> > +
> > +        if (hotplug_ctrl) {
> > +            hotplug_handler_post_plug(hotplug_ctrl, dev, &local_err);
> > +        }
> > +
> > +        if (local_err != NULL) {
> > +            dev->realized = value;  
> 
> dev->realized = value was already set above.
> 
> In order to preserve semantics you would need a bool old_value =
> dev->realized which you can restore in post_realize_fail.  QEMU current
> does not assign dev->realized = value when there is a failure!

Stefan,

as agreed on
 "Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features" thread
this series would be merged as is and then as follow up author
will fixup all issues with it.

For this patch it means a patch on top of Michael pull req
to drop lock and drop hotplug_handler_post_plug() code in favor
of existing hotplug_handler_plug().