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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 3 Nov 2016 19:09:35 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 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.
and include in that fixed up v3 1/4

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