On Thu, 3 Nov 2016 11:51:28 +0800
Xiao Guangrong <guangrong.x...@linux.intel.com> 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
> we 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
>
> Signed-off-by: Xiao Guangrong <guangrong.x...@linux.intel.com>
> ---
> hw/acpi/nvdimm.c | 57
> ++++++++++++++++++++++++++++++-------------------
> hw/i386/acpi-build.c | 2 +-
> hw/i386/pc.c | 4 ++++
> include/hw/mem/nvdimm.h | 21 +++++++++++++++++-
> 4 files changed, 60 insertions(+), 24 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index b8a2e62..9fee077 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)
s/nvdimm_plugged_device_list/nvdimm_device_list/
> 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, DEVICE(obj));
> }
>
> object_child_foreach(obj, nvdimm_plugged_device_list, opaque);
> @@ -348,8 +344,9 @@ static void nvdimm_build_structure_dcr(GArray
> *structures, DeviceState *dev)
> (DSM) in DSM Spec Rev1.*/);
> }
>
> -static GArray *nvdimm_build_device_structure(GSList *device_list)
> +static GArray *nvdimm_build_device_structure(void)
> {
> + GSList *device_list = nvdimm_get_plugged_device_list();
> GArray *structures = g_array_new(false, true /* clear */, 1);
>
> for (; device_list; device_list = device_list->next) {
> @@ -367,28 +364,50 @@ static GArray *nvdimm_build_device_structure(GSList
> *device_list)
> /* build NVDIMM Control Region Structure. */
> nvdimm_build_structure_dcr(structures, dev);
> }
> + g_slist_free(device_list);
>
> return structures;
> }
>
> -static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets,
> +static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> + fit_buf->fit = g_array_new(false, true /* clear */, 1);
> +}
> +
> +static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> +{
> + g_array_free(fit_buf->fit, true);
> + fit_buf->fit = nvdimm_build_device_structure();
> + fit_buf->dirty = true;
> +}
> +
> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state)
> +{
> + nvdimm_build_fit_buffer(&state->fit_buf);
> +}
> +
> +static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
> GArray *table_data, BIOSLinker *linker)
> {
> - GArray *structures = nvdimm_build_device_structure(device_list);
> + NvdimmFitBuffer *fit_buf = &state->fit_buf;
> unsigned int header;
>
> + /* NVDIMM device is not plugged? */
> + if (!fit_buf->fit->len) {
it's not really obvious way to check for present nvdimms,
maybe dropping this hunk and keeping device_list check at call site would be
clearer.
> + return;
> + }
> +
> acpi_add_table(table_offsets, table_data);
>
> /* NFIT header. */
> header = table_data->len;
> acpi_data_push(table_data, sizeof(NvdimmNfitHeader));
> /* NVDIMM device structures. */
> - g_array_append_vals(table_data, structures->data, structures->len);
> + g_array_append_vals(table_data, fit_buf->fit->data, fit_buf->fit->len);
>
> build_header(linker, table_data,
> (void *)(table_data->data + header), "NFIT",
> - sizeof(NvdimmNfitHeader) + structures->len, 1, NULL, NULL);
> - g_array_free(structures, true);
> + sizeof(NvdimmNfitHeader) + fit_buf->fit->len, 1, NULL,
> NULL);
> }
>
> struct NvdimmDsmIn {
> @@ -771,6 +790,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state,
> MemoryRegion *io,
> acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
> fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
> state->dsm_mem->len);
> +
> + nvdimm_init_fit_buffer(&state->fit_buf);
> }
>
> #define NVDIMM_COMMON_DSM "NCAL"
> @@ -1045,25 +1066,17 @@ static void nvdimm_build_ssdt(GArray *table_offsets,
> GArray *table_data,
> }
>
> void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> - BIOSLinker *linker, GArray *dsm_dma_arrea,
> + BIOSLinker *linker, AcpiNVDIMMState *state,
> uint32_t ram_slots)
> {
> - GSList *device_list;
> -
> - device_list = nvdimm_get_plugged_device_list();
> -
> - /* NVDIMM device is plugged. */
> - if (device_list) {
> - nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> - g_slist_free(device_list);
> - }
> + nvdimm_build_nfit(state, table_offsets, table_data, linker);
>
> /*
> * NVDIMM device is allowed to be plugged only if there is available
> * slot.
> */
> if (ram_slots) {
> - nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea,
> + nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> ram_slots);
you've ignored comments on v3 1/4,
fix it up.
> }
> }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6ae4769..bc49958 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2767,7 +2767,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> }
> if (pcms->acpi_nvdimm_state.is_enabled) {
> nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> - pcms->acpi_nvdimm_state.dsm_mem,
> machine->ram_slots);
> + &pcms->acpi_nvdimm_state, machine->ram_slots);
> }
>
> /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 93ff49c..77ca7f4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1700,6 +1700,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> goto out;
> }
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + nvdimm_acpi_hotplug(&pcms->acpi_nvdimm_state);
s/nvdimm_acpi_hotplug/nvdimm_plug/
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &error_abort);
> out:
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 63a2b20..232437c 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -98,12 +98,30 @@ typedef struct NVDIMMClass NVDIMMClass;
> #define NVDIMM_ACPI_IO_BASE 0x0a18
> #define NVDIMM_ACPI_IO_LEN 4
>
> +/*
> + * The buffer, @fit, saves the FIT info for all the presented NVDIMM
> + * devices
FIT structures for present NVDIMMs
> + which is updated after the NVDIMM device is plugged or
> + * unplugged.
s/which is/. It is/
> + *
> + * Mark @dirty whenever the buffer is updated so that it preserves NVDIMM
> + * ACPI _FIT method to read incomplete or obsolete fit info if fit update
> + * happens during multiple RFIT calls.
> + */
not valid doc comment, see include/qom/object.h for example
> +struct NvdimmFitBuffer {
> + GArray *fit;
> + bool dirty;
> +};
> +typedef struct NvdimmFitBuffer NvdimmFitBuffer;
> +
> struct AcpiNVDIMMState {
> /* detect if NVDIMM support is enabled. */
> bool is_enabled;
>
> /* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
> GArray *dsm_mem;
> +
> + NvdimmFitBuffer fit_buf;
> +
> /* the IO region used by OSPM to transfer control to QEMU. */
> MemoryRegion io_mr;
> };
> @@ -112,6 +130,7 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState;
> void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
> FWCfgState *fw_cfg, Object *owner);
> void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> - BIOSLinker *linker, GArray *dsm_dma_arrea,
> + BIOSLinker *linker, AcpiNVDIMMState *state,
> uint32_t ram_slots);
> +void nvdimm_acpi_hotplug(AcpiNVDIMMState *state);
> #endif
