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

From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com,
	stefanha@redhat.com, mst@redhat.com, rth@twiddle.net,
	ehabkost@redhat.com, dan.j.williams@intel.com,
	kvm@vger.kernel.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots
Date: Thu, 22 Sep 2016 10:43:50 +0800	[thread overview]
Message-ID: <0b786402-5f64-d355-f3c5-392f271cad01@linux.intel.com> (raw)
In-Reply-To: <20160921134807.65e9d57c@nial.brq.redhat.com>

On 09/21/2016 07:48 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:04 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> 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@linux.intel.com>
>> ---
>>  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.