On Tue, 1 Nov 2016 15:17:01 +0000
Stefan Hajnoczi <stefanha@xxxxxxxxx> 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().
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
