Re: [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots

[Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>]

On 09/21/2016 07:48 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:04 +0800
> Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
>
> > For each NVDIMM present or intended to be supported by platform,
> > platform firmware also exposes an ACPI Namespace Device under
> > the root device
> >
> > So it builds nvdimm devices for all slots to support vNVDIMM hotplug
> >
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> > ---
> >  hw/acpi/nvdimm.c        | 41 ++++++++++++++++++++++++-----------------
> >  hw/i386/acpi-build.c    |  2 +-
> >  include/hw/mem/nvdimm.h |  3 ++-
> >  3 files changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 5454c0f..0e2b9f0 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -886,12 +886,11 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
> >      aml_append(dev, method);
> >  }
> >
> > -static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
> > +static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
> >  {
> > -    for (; device_list; device_list = device_list->next) {
> > -        DeviceState *dev = device_list->data;
> > -        int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> > -                                           NULL);
> > +    uint32_t slot;
> > +
> > +    for (slot = 0; slot < ram_slots; slot++) {
> >          uint32_t handle = nvdimm_slot_to_handle(slot);
> >          Aml *nvdimm_dev;
> >
> > @@ -912,9 +911,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
> >      }
> >  }
> >
> > -static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> > -                              GArray *table_data, BIOSLinker *linker,
> > -                              GArray *dsm_dma_arrea)
> > +static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> > +                              BIOSLinker *linker, GArray *dsm_dma_arrea,
> > +                              uint32_t ram_slots)
> >  {
> >      Aml *ssdt, *sb_scope, *dev, *field;
> >      int mem_addr_offset, nvdimm_ssdt;
> > @@ -1003,7 +1002,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> >      /* 0 is reserved for root device. */
> >      nvdimm_build_device_dsm(dev, 0);
> >
> > -    nvdimm_build_nvdimm_devices(device_list, dev);
> > +    nvdimm_build_nvdimm_devices(dev, ram_slots);
> >
> >      aml_append(sb_scope, dev);
> >      aml_append(ssdt, sb_scope);
> > @@ -1028,17 +1027,25 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> >  }
> >
> >  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
> > -                       BIOSLinker *linker, GArray *dsm_dma_arrea)
> > +                       BIOSLinker *linker, GArray *dsm_dma_arrea,
> > +                       uint32_t ram_slots)
> >  {
> >      GSList *device_list;
> >
> > -    /* no NVDIMM device is plugged. */
> >      device_list = nvdimm_get_plugged_device_list();
> > -    if (!device_list) {
> > -        return;
> > +
> > +    /* NVDIMM device is plugged. */
> > +    if (device_list) {
> > +        nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> > +        g_slist_free(device_list);
> > +    }
> > +
> > +    /*
> > +     * NVDIMM device is allowed to be plugged only if there has available
> s/has/is/

Will fix. Thank you for pointing it out.

> > +     * slot.
> > +     */
> > +    if (ram_slots) {
> another question:
>  Is NFIT table generated above sufficient without below nvdim SSDT?
>
> maybe you should put
>   if (!ram_slots) {
>     return;
>   }
> at the function start?

Not needed.

As the NFIT table is created only if there is nvdimm device already plugged
that means QEMU must have available ram-slots.


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html