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

From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
To: Igor Mammedov <imammedo@redhat.com>,
	Stefan Hajnoczi <stefanha@gmail.com>
Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com,
	stefanha@redhat.com, mst@redhat.com, rth@twiddle.net,
	ehabkost@redhat.com, dan.j.williams@intel.com,
	kvm@vger.kernel.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
Date: Wed, 2 Nov 2016 00:00:12 +0800	[thread overview]
Message-ID: <d982cf67-53b1-4611-11d8-a28021623bde@linux.intel.com> (raw)
In-Reply-To: <20161101162505.57172707@nial.brq.redhat.com>

On 11/01/2016 11:25 PM, Igor Mammedov wrote:
> 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().
>

Thank you, Igor, Michael and Stefan. I will do it asap after
this patchset is merged. :)