From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots Date: Thu, 22 Sep 2016 10:43:50 +0800 Message-ID: <0b786402-5f64-d355-f3c5-392f271cad01@linux.intel.com> References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-3-git-send-email-guangrong.xiao@linux.intel.com> <20160921134807.65e9d57c@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 To: Igor Mammedov Return-path: Received: from mga05.intel.com ([192.55.52.43]:36541 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932937AbcIVCty (ORCPT ); Wed, 21 Sep 2016 22:49:54 -0400 In-Reply-To: <20160921134807.65e9d57c@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/21/2016 07:48 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:04 +0800 > Xiao Guangrong 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 >> --- >> 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmu5M-0008FZ-MS for qemu-devel@nongnu.org; Wed, 21 Sep 2016 22:50:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmu5C-0000st-CN for qemu-devel@nongnu.org; Wed, 21 Sep 2016 22:49:59 -0400 Received: from mga04.intel.com ([192.55.52.120]:41279) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmu5C-0000sc-3g for qemu-devel@nongnu.org; Wed, 21 Sep 2016 22:49:54 -0400 References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-3-git-send-email-guangrong.xiao@linux.intel.com> <20160921134807.65e9d57c@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <0b786402-5f64-d355-f3c5-392f271cad01@linux.intel.com> Date: Thu, 22 Sep 2016 10:43:50 +0800 MIME-Version: 1.0 In-Reply-To: <20160921134807.65e9d57c@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov 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 On 09/21/2016 07:48 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:04 +0800 > Xiao Guangrong 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 >> --- >> 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.