Re: [Qemu-devel] [PATCH v4 1/3] nvdimm acpi: introduce fit buffer

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 11/03/2016 07:02 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:28 +0800
> Xiao Guangrong <guangrong.xiao@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.xiao@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/

Okay.

>
>>      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.

Hmm... okay.

>
>> +        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.

As you said "I'd prefer it to make a clean revert for patches 2-4/4  first",
i thought the first patch is okay.

But it does not matter, i will quickly post a new version after you
review all the patches.

>
>>      }
>>  }
>> 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/

Okay.

>
>> +    }
>> +
>>      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

Okay.

>
>> + which is updated after the NVDIMM device is plugged or
>> + * unplugged.
> s/which is/. It is/

Okay.

>
>> + *
>> + * 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
>

Okay, will change it to:

NvdimmFitBuffer:
@fit: FIT structures for present NVDIMMs. It is updated after the NVDIMM device
       is plugged or unplugged.
@dirty: It is set 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