diff mbox

[RFC,2/3] nvdimm: use configurable ACPI IO base and size

Message ID 1468975744-12587-3-git-send-email-kwangwoo.lee@sk.com
State New
Headers show

Commit Message

Kwangwoo Lee July 20, 2016, 12:49 a.m. UTC 
This patch uses configurable IO base and size to create NPIO AML for
ACPI NFIT. Since a different architecture like AArch64 does not use
port-mapped IO, a configurable IO base is required to create correct
mapping of ACPI IO address and size.

Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
---
 hw/acpi/nvdimm.c        | 23 +++++++++++++++--------
 hw/i386/acpi-build.c    |  2 +-
 hw/i386/pc_piix.c       |  8 +++++++-
 hw/i386/pc_q35.c        |  8 +++++++-
 include/hw/mem/nvdimm.h | 17 ++++++++++++++++-
 5 files changed, 46 insertions(+), 12 deletions(-)

Comments

Peter Maydell July 25, 2016, 4:12 p.m. UTC | #1 
On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:
> This patch uses configurable IO base and size to create NPIO AML for
> ACPI NFIT. Since a different architecture like AArch64 does not use
> port-mapped IO, a configurable IO base is required to create correct
> mapping of ACPI IO address and size.
>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> ---
>  hw/acpi/nvdimm.c        | 23 +++++++++++++++--------
>  hw/i386/acpi-build.c    |  2 +-
>  hw/i386/pc_piix.c       |  8 +++++++-
>  hw/i386/pc_q35.c        |  8 +++++++-
>  include/hw/mem/nvdimm.h | 17 ++++++++++++++++-
>  5 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index e486128..57e03ee 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -765,8 +765,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
>      memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
> -                          "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
> -    memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
> +                          "nvdimm-acpi-io", state->dsm_io.size);
> +    memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);
>
>      state->dsm_mem = g_array_new(false, true /* clear */, 1);
>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));

Why does this function take a MemoryRegion to insert itself into,
rather than returning a MemoryRegion for the caller to map into
wherever is appropriate, or even being a DeviceState which has
mappable memory regions via the sysbus API ?

I guess the answer is "that's the way it happens to be at the moment",
so I'm not really asking for a change here necessarily.

> @@ -912,9 +912,10 @@ 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)
> +                              AcpiNVDIMMState *acpi_nvdimm_state)
>  {
>      Aml *ssdt, *sb_scope, *dev, *field;
> +    AmlRegionSpace rs;
>      int mem_addr_offset, nvdimm_ssdt;
>
>      acpi_add_table(table_offsets, table_data);
> @@ -940,8 +941,14 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>
>      /* map DSM memory and IO into ACPI namespace. */
> -    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
> -               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
> +    if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {
> +        rs = AML_SYSTEM_IO;
> +    } else {
> +        rs = AML_SYSTEM_MEMORY;
> +    }
> +    aml_append(dev, aml_operation_region("NPIO", rs,
> +               aml_int(acpi_nvdimm_state->dsm_io.base),
> +               acpi_nvdimm_state->dsm_io.size));
>      aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
>                 aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
>
> @@ -1014,7 +1021,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>                                                 NVDIMM_ACPI_MEM_ADDR);
>
>      bios_linker_loader_alloc(linker,
> -                             NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
> +                             NVDIMM_DSM_MEM_FILE, acpi_nvdimm_state->dsm_mem,
>                               sizeof(NvdimmDsmIn), false /* high memory */);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
> @@ -1026,7 +1033,7 @@ 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, AcpiNVDIMMState *acpi_nvdimm_state)
>  {
>      GSList *device_list;
>
> @@ -1037,6 +1044,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>      }
>      nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
>      nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
> -                      dsm_dma_arrea);
> +                      acpi_nvdimm_state);
>      g_slist_free(device_list);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fbba461..54b09a9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2632,7 +2632,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);
>      }
>
>      /* Add tables supplied by user (if any) */
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a07dc81..b624f59 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -298,7 +298,13 @@ static void pc_init1(MachineState *machine,
>      }
>
>      if (pcms->acpi_nvdimm_state.is_enabled) {
> -        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> +        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> +
> +        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> +        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> +        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> +
> +        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c0b9961..779ac32 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -263,7 +263,13 @@ static void pc_q35_init(MachineState *machine)
>      }
>
>      if (pcms->acpi_nvdimm_state.is_enabled) {
> -        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
> +        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
> +
> +        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
> +        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
> +        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
> +
> +        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }

Ideally this would be a QOM object with QOM properties, rather
than an ad-hoc init function.

thanks
-- PMM
Kwangwoo Lee July 26, 2016, 6:55 a.m. UTC | #2 
Hi Peter,

Thanks for reviewing the patches! I added some comments below.

> -----Original Message-----

> From: Peter Maydell [mailto:peter.maydell@linaro.org]

> Sent: Tuesday, July 26, 2016 1:13 AM

> To: 이광우(LEE KWANGWOO) MS SW

> Cc: Xiao Guangrong; Michael S. Tsirkin; Igor Mammedov; Paolo Bonzini; Richard Henderson; Eduardo

> Habkost; Shannon Zhao; Shannon Zhao; QEMU Developers; qemu-arm; 정우석(CHUNG WOO SUK) MS SW; 김현철(KIM

> HYUNCHUL) MS SW

> Subject: Re: [RFC PATCH 2/3] nvdimm: use configurable ACPI IO base and size

> 

> On 20 July 2016 at 01:49, Kwangwoo Lee <kwangwoo.lee@sk.com> wrote:

> > This patch uses configurable IO base and size to create NPIO AML for

> > ACPI NFIT. Since a different architecture like AArch64 does not use

> > port-mapped IO, a configurable IO base is required to create correct

> > mapping of ACPI IO address and size.

> >

> > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>

> > ---

> >  hw/acpi/nvdimm.c        | 23 +++++++++++++++--------

> >  hw/i386/acpi-build.c    |  2 +-

> >  hw/i386/pc_piix.c       |  8 +++++++-

> >  hw/i386/pc_q35.c        |  8 +++++++-

> >  include/hw/mem/nvdimm.h | 17 ++++++++++++++++-

> >  5 files changed, 46 insertions(+), 12 deletions(-)

> >

> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c

> > index e486128..57e03ee 100644

> > --- a/hw/acpi/nvdimm.c

> > +++ b/hw/acpi/nvdimm.c

> > @@ -765,8 +765,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,

> >                              FWCfgState *fw_cfg, Object *owner)

> >  {

> >      memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,

> > -                          "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);

> > -    memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);

> > +                          "nvdimm-acpi-io", state->dsm_io.size);

> > +    memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);

> >

> >      state->dsm_mem = g_array_new(false, true /* clear */, 1);

> >      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));

> 

> Why does this function take a MemoryRegion to insert itself into,

> rather than returning a MemoryRegion for the caller to map into

> wherever is appropriate, or even being a DeviceState which has

> mappable memory regions via the sysbus API ?


On i386, the region is added to system_io which is ported-mapped IO at 0x0a18.
Therefore, to use memory-mapped IO for NVDIMM DSM(Device Specific Method), a
member variable - dsm_io - which containing type, base address and size need to
be added on ARM64 platform.

> I guess the answer is "that's the way it happens to be at the moment",

> so I'm not really asking for a change here necessarily.


Thanks for your understanding!

> > @@ -912,9 +912,10 @@ 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)

> > +                              AcpiNVDIMMState *acpi_nvdimm_state)

> >  {

> >      Aml *ssdt, *sb_scope, *dev, *field;

> > +    AmlRegionSpace rs;

> >      int mem_addr_offset, nvdimm_ssdt;

> >

> >      acpi_add_table(table_offsets, table_data);

> > @@ -940,8 +941,14 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,

> >      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));

> >

> >      /* map DSM memory and IO into ACPI namespace. */

> > -    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,

> > -               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));

> > +    if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {

> > +        rs = AML_SYSTEM_IO;

> > +    } else {

> > +        rs = AML_SYSTEM_MEMORY;

> > +    }

> > +    aml_append(dev, aml_operation_region("NPIO", rs,

> > +               aml_int(acpi_nvdimm_state->dsm_io.base),

> > +               acpi_nvdimm_state->dsm_io.size));

> >      aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,

> >                 aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));

> >

> > @@ -1014,7 +1021,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,

> >                                                 NVDIMM_ACPI_MEM_ADDR);

> >

> >      bios_linker_loader_alloc(linker,

> > -                             NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,

> > +                             NVDIMM_DSM_MEM_FILE, acpi_nvdimm_state->dsm_mem,

> >                               sizeof(NvdimmDsmIn), false /* high memory */);

> >      bios_linker_loader_add_pointer(linker,

> >          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),

> > @@ -1026,7 +1033,7 @@ 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, AcpiNVDIMMState *acpi_nvdimm_state)

> >  {

> >      GSList *device_list;

> >

> > @@ -1037,6 +1044,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,

> >      }

> >      nvdimm_build_nfit(device_list, table_offsets, table_data, linker);

> >      nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,

> > -                      dsm_dma_arrea);

> > +                      acpi_nvdimm_state);

> >      g_slist_free(device_list);

> >  }

> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c

> > index fbba461..54b09a9 100644

> > --- a/hw/i386/acpi-build.c

> > +++ b/hw/i386/acpi-build.c

> > @@ -2632,7 +2632,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);

> >      }

> >

> >      /* Add tables supplied by user (if any) */

> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c

> > index a07dc81..b624f59 100644

> > --- a/hw/i386/pc_piix.c

> > +++ b/hw/i386/pc_piix.c

> > @@ -298,7 +298,13 @@ static void pc_init1(MachineState *machine,

> >      }

> >

> >      if (pcms->acpi_nvdimm_state.is_enabled) {

> > -        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,

> > +        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;

> > +

> > +        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;

> > +        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;

> > +        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;

> > +

> > +        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,

> >                                 pcms->fw_cfg, OBJECT(pcms));

> >      }

> >  }

> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c

> > index c0b9961..779ac32 100644

> > --- a/hw/i386/pc_q35.c

> > +++ b/hw/i386/pc_q35.c

> > @@ -263,7 +263,13 @@ static void pc_q35_init(MachineState *machine)

> >      }

> >

> >      if (pcms->acpi_nvdimm_state.is_enabled) {

> > -        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,

> > +        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;

> > +

> > +        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;

> > +        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;

> > +        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;

> > +

> > +        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,

> >                                 pcms->fw_cfg, OBJECT(pcms));

> >      }

> >  }

> 

> Ideally this would be a QOM object with QOM properties, rather

> than an ad-hoc init function.


On i386, NVDIMM_ACPI_IO_BASE is fixed with 0x0a18. Should it be changed, too?
I'm not sure that I need to change it and can affect the i386 platform..

> thanks

> -- PMM


Best Regards,
Kwangwoo Lee
diff mbox

Patch

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e486128..57e03ee 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -765,8 +765,8 @@  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
                             FWCfgState *fw_cfg, Object *owner)
 {
     memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
-                          "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
-    memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
+                          "nvdimm-acpi-io", state->dsm_io.size);
+    memory_region_add_subregion(io, state->dsm_io.base, &state->io_mr);
 
     state->dsm_mem = g_array_new(false, true /* clear */, 1);
     acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -912,9 +912,10 @@  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)
+                              AcpiNVDIMMState *acpi_nvdimm_state)
 {
     Aml *ssdt, *sb_scope, *dev, *field;
+    AmlRegionSpace rs;
     int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
@@ -940,8 +941,14 @@  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
     /* map DSM memory and IO into ACPI namespace. */
-    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
-               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+    if (acpi_nvdimm_state->dsm_io.type == NVDIMM_ACPI_IO_PORT) {
+        rs = AML_SYSTEM_IO;
+    } else {
+        rs = AML_SYSTEM_MEMORY;
+    }
+    aml_append(dev, aml_operation_region("NPIO", rs,
+               aml_int(acpi_nvdimm_state->dsm_io.base),
+               acpi_nvdimm_state->dsm_io.size));
     aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
                aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
 
@@ -1014,7 +1021,7 @@  static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
-                             NVDIMM_DSM_MEM_FILE, dsm_dma_arrea,
+                             NVDIMM_DSM_MEM_FILE, acpi_nvdimm_state->dsm_mem,
                              sizeof(NvdimmDsmIn), false /* high memory */);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
@@ -1026,7 +1033,7 @@  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, AcpiNVDIMMState *acpi_nvdimm_state)
 {
     GSList *device_list;
 
@@ -1037,6 +1044,6 @@  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
     }
     nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
     nvdimm_build_ssdt(device_list, table_offsets, table_data, linker,
-                      dsm_dma_arrea);
+                      acpi_nvdimm_state);
     g_slist_free(device_list);
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fbba461..54b09a9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2632,7 +2632,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);
     }
 
     /* Add tables supplied by user (if any) */
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a07dc81..b624f59 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -298,7 +298,13 @@  static void pc_init1(MachineState *machine,
     }
 
     if (pcms->acpi_nvdimm_state.is_enabled) {
-        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
+
+        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
+        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
+        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
+
+        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c0b9961..779ac32 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -263,7 +263,13 @@  static void pc_q35_init(MachineState *machine)
     }
 
     if (pcms->acpi_nvdimm_state.is_enabled) {
-        nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io,
+        AcpiNVDIMMState *acpi_nvdimm_state = &pcms->acpi_nvdimm_state;
+
+        acpi_nvdimm_state->dsm_io.type = NVDIMM_ACPI_IO_PORT;
+        acpi_nvdimm_state->dsm_io.base = NVDIMM_ACPI_IO_BASE;
+        acpi_nvdimm_state->dsm_io.size = NVDIMM_ACPI_IO_LEN;
+
+        nvdimm_init_acpi_state(acpi_nvdimm_state, system_io,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 1cfe9e0..8c917c3 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -98,10 +98,25 @@  typedef struct NVDIMMClass NVDIMMClass;
 #define NVDIMM_ACPI_IO_BASE     0x0a18
 #define NVDIMM_ACPI_IO_LEN      4
 
+typedef enum {
+    NVDIMM_ACPI_IO_PORT = 0X00,
+    NVDIMM_ACPI_IO_MEMORY = 0X01,
+} AcpiNVDIMMIOType;
+
+struct AcpiNVDIMMIOEntry {
+    AcpiNVDIMMIOType type;
+    hwaddr base;
+    hwaddr size;
+};
+typedef struct AcpiNVDIMMIOEntry AcpiNVDIMMIOEntry;
+
 struct AcpiNVDIMMState {
     /* detect if NVDIMM support is enabled. */
     bool is_enabled;
 
+    /* NVDIMM IO Type, Address, and Size */
+    AcpiNVDIMMIOEntry dsm_io;
+
     /* the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. */
     GArray *dsm_mem;
     /* the IO region used by OSPM to transfer control to QEMU. */
@@ -112,5 +127,5 @@  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, AcpiNVDIMMState *acpi_nvdimm_state);
 #endif