From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: [PATCH v2 2/8] nvdimm acpi: prebuild nvdimm devices for available slots Date: Wed, 21 Sep 2016 13:48:07 +0200 Message-ID: <20160921134807.65e9d57c@nial.brq.redhat.com> References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-3-git-send-email-guangrong.xiao@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:50716 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933657AbcIULsL (ORCPT ); Wed, 21 Sep 2016 07:48:11 -0400 In-Reply-To: <1470984850-66891-3-git-send-email-guangrong.xiao@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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/ > + * 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? > + nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea, > + ram_slots); > } > - nvdimm_build_nfit(device_list, table_offsets, table_data, linker); > - nvdimm_build_ssdt(device_list, table_offsets, table_data, linker, > - dsm_dma_arrea); > - g_slist_free(device_list); > } > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index a26a4bb..b1d0ced 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2712,7 +2712,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); > + pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots); > } > > /* Add tables supplied by user (if any) */ > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h > index 1cfe9e0..63a2b20 100644 > --- a/include/hw/mem/nvdimm.h > +++ b/include/hw/mem/nvdimm.h > @@ -112,5 +112,6 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState; > void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, > FWCfgState *fw_cfg, Object *owner); > 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); > #endif From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmg0d-0000Ll-QK for qemu-devel@nongnu.org; Wed, 21 Sep 2016 07:48:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmg0a-00070N-Jf for qemu-devel@nongnu.org; Wed, 21 Sep 2016 07:48:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58832) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmg0a-0006za-9k for qemu-devel@nongnu.org; Wed, 21 Sep 2016 07:48:12 -0400 Date: Wed, 21 Sep 2016 13:48:07 +0200 From: Igor Mammedov Message-ID: <20160921134807.65e9d57c@nial.brq.redhat.com> In-Reply-To: <1470984850-66891-3-git-send-email-guangrong.xiao@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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Xiao Guangrong 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 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/ > + * 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? > + nvdimm_build_ssdt(table_offsets, table_data, linker, dsm_dma_arrea, > + ram_slots); > } > - nvdimm_build_nfit(device_list, table_offsets, table_data, linker); > - nvdimm_build_ssdt(device_list, table_offsets, table_data, linker, > - dsm_dma_arrea); > - g_slist_free(device_list); > } > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index a26a4bb..b1d0ced 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2712,7 +2712,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); > + pcms->acpi_nvdimm_state.dsm_mem, machine->ram_slots); > } > > /* Add tables supplied by user (if any) */ > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h > index 1cfe9e0..63a2b20 100644 > --- a/include/hw/mem/nvdimm.h > +++ b/include/hw/mem/nvdimm.h > @@ -112,5 +112,6 @@ typedef struct AcpiNVDIMMState AcpiNVDIMMState; > void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, > FWCfgState *fw_cfg, Object *owner); > 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); > #endif