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

From: Stefan Hajnoczi <stefanha@gmail.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com,
	gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org,
	stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com,
	dan.j.williams@intel.com, rth@twiddle.net
Subject: Re: [PATCH v3 2/4] nvdimm acpi: introduce fit buffer
Date: Tue, 1 Nov 2016 15:17:01 +0000	[thread overview]
Message-ID: <20161101151701.GC5707@stefanha-x1.localdomain> (raw)
In-Reply-To: <1477672540-27952-3-git-send-email-guangrong.xiao@linux.intel.com>

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]