[PATCH 0/5] ARM virt: Add NVDIMM support

[PATCH 0/5] ARM virt: Add NVDIMM support

[Shameer Kolothum]

This series adds NVDIMM support to arm/virt platform.
This has a dependency on [0] and make use of the GED
device for NVDIMM hotplug events. The series reuses
some of the patches posted by Eric in his earlier
attempt here[1].

Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
hot add case described here[2].

I have done basic sanity testing of NVDIMM deviecs with
both ACPI and DT Guest boot. Further testing is always
welcome.

Please let me know your feedback.

Thanks,
Shameer

[0] https://patchwork.kernel.org/cover/11150345/
[1] https://patchwork.kernel.org/cover/10830777/
[2] https://patchwork.kernel.org/patch/11154757/

Eric Auger (1):
  hw/arm/boot: Expose the pmem nodes in the DT

Kwangwoo Lee (2):
  nvdimm: Use configurable ACPI IO base and size
  hw/arm/virt: Add nvdimm hot-plug infrastructure

Shameer Kolothum (2):
  hw/arm: Align ACPI blob len to PAGE size
  hw/arm/virt: Add nvdimm hotplug support

 docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
 hw/acpi/generic_event_device.c         | 13 ++++++++
 hw/acpi/nvdimm.c                       | 32 ++++++++++++------
 hw/arm/Kconfig                         |  1 +
 hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c               | 20 ++++++++++++
 hw/arm/virt.c                          | 42 ++++++++++++++++++++----
 hw/i386/acpi-build.c                   |  6 ++++
 hw/i386/acpi-build.h                   |  3 ++
 hw/i386/pc_piix.c                      |  2 ++
 hw/i386/pc_q35.c                       |  2 ++
 hw/mem/Kconfig                         |  2 +-
 include/hw/acpi/generic_event_device.h |  1 +
 include/hw/arm/virt.h                  |  1 +
 include/hw/mem/nvdimm.h                |  3 ++
 15 files changed, 157 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size

[Shameer Kolothum]

If ACPI blob length modifications happens after the initial
virt_acpi_build() call, and the changed blob length is within
the PAGE size boundary, then the revised size is not seen by
the firmware on Guest reboot. The is because in the
virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
path, qemu_ram_resize() uses ram_block size which is aligned
to PAGE size and the "resize callback" to update the size seen
by firmware is not getting invoked. Hence align ACPI blob sizes
to PAGE boundary.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
More details on this issue can be found here,
https://patchwork.kernel.org/patch/11154757/

---
 hw/arm/virt-acpi-build.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4cd50175e0..074e0c858e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    GArray *cmd_blob = tables->linker->cmd_blob;
     MachineState *ms = MACHINE(vms);
 
     table_offsets = g_array_new(false, true /* clear */,
@@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
     }
 
+    /*
+     * Align the ACPI blob lengths to PAGE size so that on ACPI table
+     * regeneration, the length that firmware sees really gets updated
+     * through 'resize' callback in qemu_ram_resize() in the
+     * virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
+     * path.
+     */
+    g_array_set_size(tables_blob,
+                     TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
+    g_array_set_size(tables->rsdp,
+                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
+    g_array_set_size(cmd_blob,
+                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
 }
-- 
2.17.1

Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size

[Igor Mammedov]

On Fri, 4 Oct 2019 16:52:58 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> If ACPI blob length modifications happens after the initial
> virt_acpi_build() call, and the changed blob length is within
> the PAGE size boundary, then the revised size is not seen by
> the firmware on Guest reboot. The is because in the
> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> path, qemu_ram_resize() uses ram_block size which is aligned
> to PAGE size and the "resize callback" to update the size seen
> by firmware is not getting invoked. Hence align ACPI blob sizes
> to PAGE boundary.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> More details on this issue can be found here,
> https://patchwork.kernel.org/patch/11154757/
re-read it again and it seems to me that this patch is workaround
rather than a solution to the problem.
CCing Michael as an author this code.

on x86 we have crazy history of manually aligning acpi blobs, see code under comment

  /* We'll expose it all to Guest so we want to reduce

so used_length endups with over-sized value which includes table and padding
and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page size
so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next padded table
size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't trigger
  block->used_length == HOST_PAGE_ALIGN(newsize)
condition so fwcfg gets updated value.


> ---
>  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4cd50175e0..074e0c858e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      GArray *table_offsets;
>      unsigned dsdt, xsdt;
>      GArray *tables_blob = tables->table_data;
> +    GArray *cmd_blob = tables->linker->cmd_blob;
>      MachineState *ms = MACHINE(vms);
>  
>      table_offsets = g_array_new(false, true /* clear */,
> @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>      }
>  
> +    /*
> +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> +     * regeneration, the length that firmware sees really gets updated
> +     * through 'resize' callback in qemu_ram_resize() in the
> +     * virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> +     * path.
> +     */
> +    g_array_set_size(tables_blob,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN relation
so depending on host it could flip it's behavior to opposite.

one thing we could do is dropping (block->used_length == newsize) condition
another is to use value of block->used_length for s->files->f[index].size.

Michael,
what's your take in this?

> +    g_array_set_size(tables->rsdp,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> +    g_array_set_size(cmd_blob,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }

RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 08 November 2019 16:18
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; Linuxarm <linuxarm@huawei.com>; Michael S. Tsirkin
> <mst@redhat.com>
> Subject: Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> 
> On Fri, 4 Oct 2019 16:52:58 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > If ACPI blob length modifications happens after the initial
> > virt_acpi_build() call, and the changed blob length is within
> > the PAGE size boundary, then the revised size is not seen by
> > the firmware on Guest reboot. The is because in the
> > virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> > path, qemu_ram_resize() uses ram_block size which is aligned
> > to PAGE size and the "resize callback" to update the size seen
> > by firmware is not getting invoked. Hence align ACPI blob sizes
> > to PAGE boundary.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > More details on this issue can be found here,
> > https://patchwork.kernel.org/patch/11154757/
> re-read it again and it seems to me that this patch is workaround
> rather than a solution to the problem.

Thanks for taking a look at this. Yes, I was also not very sure about this approach
as the root cause of the issue is in qemu_ram_resize().

> CCing Michael as an author this code.
> on x86 we have crazy history of manually aligning acpi blobs, see code under
> comment
> 
>   /* We'll expose it all to Guest so we want to reduce
> 
> so used_length endups with over-sized value which includes table and padding
> and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page
> size
> so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next padded
> table
> size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't
> trigger
>   block->used_length == HOST_PAGE_ALIGN(newsize)
> condition so fwcfg gets updated value.

Yes, this is the reason why the issue is not visible on x86.
 
> 
> > ---
> >  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 4cd50175e0..074e0c858e 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >      GArray *table_offsets;
> >      unsigned dsdt, xsdt;
> >      GArray *tables_blob = tables->table_data;
> > +    GArray *cmd_blob = tables->linker->cmd_blob;
> >      MachineState *ms = MACHINE(vms);
> >
> >      table_offsets = g_array_new(false, true /* clear */,
> > @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >      }
> >
> > +    /*
> > +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> > +     * regeneration, the length that firmware sees really gets updated
> > +     * through 'resize' callback in qemu_ram_resize() in the
> > +     * virt_acpi_build_update() -> acpi_ram_update() ->
> qemu_ram_resize()
> > +     * path.
> > +     */
> > +    g_array_set_size(tables_blob,
> > +
> TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
> here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN relation
> so depending on host it could flip it's behavior to opposite.

Ok.

> 
> one thing we could do is dropping (block->used_length == newsize) condition

I tried this before and strangely for some reason on reboot path,

virt_acpi_build_update() is called with build_state being NULL and no acpi_ram_update()
happens. Not sure what causes this behavior when we drop the above condition.

> another is to use value of block->used_length for s->files->f[index].size.

I just tried this by passing block->used_length to fw_cfg_add_file_callback() .
This could work for this case. But not sure there will be any corner cases
and also there isn't any easy way to access the mr->ram_balck->used_length from
hw/core/loader.c.

> 
> Michael,
> what's your take in this?

Thanks,
Shameer

> 
> > +    g_array_set_size(tables->rsdp,
> > +
> TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> > +    g_array_set_size(cmd_blob,
> > +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
> >      /* Cleanup memory that's no longer used. */
> >      g_array_free(table_offsets, true);
> >  }

RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size

[Shameerali Kolothum Thodi]

Hi Igor/ Michael,

> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 11 November 2019 12:47
> To: Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; Michael S. Tsirkin
> <mst@redhat.com>; qemu-devel@nongnu.org; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; lersek@redhat.com
> Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> 
> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 08 November 2019 16:18
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org;
> > shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com; Linuxarm <linuxarm@huawei.com>; Michael S. Tsirkin
> > <mst@redhat.com>
> > Subject: Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> >
> > On Fri, 4 Oct 2019 16:52:58 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > If ACPI blob length modifications happens after the initial
> > > virt_acpi_build() call, and the changed blob length is within
> > > the PAGE size boundary, then the revised size is not seen by
> > > the firmware on Guest reboot. The is because in the
> > > virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> > > path, qemu_ram_resize() uses ram_block size which is aligned
> > > to PAGE size and the "resize callback" to update the size seen
> > > by firmware is not getting invoked. Hence align ACPI blob sizes
> > > to PAGE boundary.
> > >
> > > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > > More details on this issue can be found here,
> > > https://patchwork.kernel.org/patch/11154757/
> > re-read it again and it seems to me that this patch is workaround
> > rather than a solution to the problem.
> 
> Thanks for taking a look at this. Yes, I was also not very sure about this
> approach
> as the root cause of the issue is in qemu_ram_resize().
> 
> > CCing Michael as an author this code.
> > on x86 we have crazy history of manually aligning acpi blobs, see code under
> > comment
> >
> >   /* We'll expose it all to Guest so we want to reduce
> >
> > so used_length endups with over-sized value which includes table and
> padding
> > and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page
> > size
> > so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next
> padded
> > table
> > size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't
> > trigger
> >   block->used_length == HOST_PAGE_ALIGN(newsize)
> > condition so fwcfg gets updated value.
> 
> Yes, this is the reason why the issue is not visible on x86.
> 
> >
> > > ---
> > >  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 4cd50175e0..074e0c858e 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >      GArray *table_offsets;
> > >      unsigned dsdt, xsdt;
> > >      GArray *tables_blob = tables->table_data;
> > > +    GArray *cmd_blob = tables->linker->cmd_blob;
> > >      MachineState *ms = MACHINE(vms);
> > >
> > >      table_offsets = g_array_new(false, true /* clear */,
> > > @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > >      }
> > >
> > > +    /*
> > > +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> > > +     * regeneration, the length that firmware sees really gets updated
> > > +     * through 'resize' callback in qemu_ram_resize() in the
> > > +     * virt_acpi_build_update() -> acpi_ram_update() ->
> > qemu_ram_resize()
> > > +     * path.
> > > +     */
> > > +    g_array_set_size(tables_blob,
> > > +
> > TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
> > here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN
> relation
> > so depending on host it could flip it's behavior to opposite.
> 
> Ok.
> 
> >
> > one thing we could do is dropping (block->used_length == newsize) condition
> 
> I tried this before and strangely for some reason on reboot path,
> 
> virt_acpi_build_update() is called with build_state being NULL and no
> acpi_ram_update()
> happens. Not sure what causes this behavior when we drop the above
> condition.
> 
> > another is to use value of block->used_length for s->files->f[index].size.
> 
> I just tried this by passing block->used_length to fw_cfg_add_file_callback() .
> This could work for this case. But not sure there will be any corner cases
> and also there isn't any easy way to access the mr->ram_balck->used_length
> from
> hw/core/loader.c.
> 
> >
> > Michael,
> > what's your take in this?
> 

This is how(below) I tried to use the RAMBlock used_length for s->files->f[index].size.
As used_length is abstracted here, I had to introduce a new api to retrieve the
same. Please take a look and let me know if there is a better way of achieving this.

Thanks.
Shameer


---8---

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 5099f27dc8..e862c8c0e1 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1055,6 +1055,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
     if (fw_file_name && fw_cfg) {
         char devpath[100];
         void *data;
+        size_t size;
 
         if (read_only) {
             snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
@@ -1065,13 +1066,15 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
         if (mc->rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
             mr = rom->mr;
+            size = memory_region_get_used_length(mr);
         } else {
             data = rom->data;
+            size = rom->datasize;
         }
 
         fw_cfg_add_file_callback(fw_cfg, fw_file_name,
                                  fw_callback, NULL, callback_opaque,
-                                 data, rom->datasize, read_only);
+                                 data, size, read_only);
     }
     return mr;
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e499dc215b..c51e6cdb9a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1584,6 +1584,12 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
  */
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr);
 
+/**
+ * memory_region_get_used_length: Get the used length associated with a memory
+ *                             region
+ */
+ram_addr_t memory_region_get_used_length(MemoryRegion *mr);
+
 uint64_t memory_region_get_alignment(const MemoryRegion *mr);
 /**
  * memory_region_del_subregion: Remove a subregion.
diff --git a/memory.c b/memory.c
index 06484c2bff..d1f60c0c9a 100644
--- a/memory.c
+++ b/memory.c
@@ -2200,6 +2200,11 @@ ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
     return mr->ram_block ? mr->ram_block->offset : RAM_ADDR_INVALID;
 }
 
+ram_addr_t memory_region_get_used_length(MemoryRegion *mr)
+{
+    return mr->ram_block ? mr->ram_block->used_length : RAM_ADDR_INVALID;
+}
+
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp)
 {
     assert(mr->ram_block);
---8--

[PATCH 2/5] nvdimm: Use configurable ACPI IO base and size

[Shameer Kolothum]

From: Kwangwoo Lee <kwangwoo.lee@sk.com>

This patch makes IO base and size configurable 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>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/acpi/nvdimm.c        | 32 ++++++++++++++++++++++----------
 hw/i386/acpi-build.c    |  6 ++++++
 hw/i386/acpi-build.h    |  3 +++
 hw/i386/pc_piix.c       |  2 ++
 hw/i386/pc_q35.c        |  2 ++
 include/hw/mem/nvdimm.h |  3 +++
 6 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fdad6dc3f..f91eea3802 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
 }
 
 void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+                            struct AcpiGenericAddress dsm_io,
                             FWCfgState *fw_cfg, Object *owner)
 {
+    state->dsm_io = dsm_io;
     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", dsm_io.bit_width >> 3);
+    memory_region_add_subregion(io, dsm_io.address, &state->io_mr);
 
     state->dsm_mem = g_array_new(false, true /* clear */, 1);
     acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -959,12 +961,14 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
 
 #define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
 
-static void nvdimm_build_common_dsm(Aml *dev)
+static void nvdimm_build_common_dsm(Aml *dev,
+                                    NVDIMMState *nvdimm_state)
 {
     Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
     Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
     uint8_t byte_list[1];
+    AmlRegionSpace rs;
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
     uuid = aml_arg(0);
@@ -975,9 +979,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
     aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
 
+    if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
+        rs = AML_SYSTEM_IO;
+    } else {
+        rs = AML_SYSTEM_MEMORY;
+    }
+
     /* map DSM memory and IO into ACPI namespace. */
-    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
-               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
+               aml_int(nvdimm_state->dsm_io.address),
+               nvdimm_state->dsm_io.bit_width >> 3));
     aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
                AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
 
@@ -992,7 +1003,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
                       AML_PRESERVE);
     aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
-               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
+              (nvdimm_state->dsm_io.bit_width >> 3) * BITS_PER_BYTE));
     aml_append(method, field);
 
     /*
@@ -1260,7 +1271,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 }
 
 static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
-                              BIOSLinker *linker, GArray *dsm_dma_area,
+                              BIOSLinker *linker,
+                              NVDIMMState *nvdimm_state,
                               uint32_t ram_slots)
 {
     Aml *ssdt, *sb_scope, *dev;
@@ -1288,7 +1300,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
-    nvdimm_build_common_dsm(dev);
+    nvdimm_build_common_dsm(dev, nvdimm_state);
 
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
@@ -1307,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
                                                NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker,
-                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
+                             NVDIMM_DSM_MEM_FILE, 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),
@@ -1329,7 +1341,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
         return;
     }
 
-    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
+    nvdimm_build_ssdt(table_offsets, table_data, linker, state,
                       ram_slots);
 
     device_list = nvdimm_get_device_list();
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1d077a7cb7..b5170912a8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -126,6 +126,12 @@ typedef struct FwCfgTPMConfig {
 
 static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
 
+const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
+    .space_id = AML_AS_SYSTEM_IO,
+    .address = NVDIMM_ACPI_IO_BASE,
+    .bit_width = NVDIMM_ACPI_IO_LEN << 3
+};
+
 static void init_common_fadt_data(MachineState *ms, Object *o,
                                   AcpiFadtData *data)
 {
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 007332e51c..74df5fc612 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -1,6 +1,9 @@
 
 #ifndef HW_I386_ACPI_BUILD_H
 #define HW_I386_ACPI_BUILD_H
+#include "hw/acpi/acpi-defs.h"
+
+extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
 
 void acpi_setup(void);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6824b72124..78521cf017 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -58,6 +58,7 @@
 #include "migration/misc.h"
 #include "kvm_i386.h"
 #include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
 
 #define MAX_IDE_BUS 2
 
@@ -303,6 +304,7 @@ else {
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+                               x86_nvdimm_acpi_dsmio,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8fad20f314..d53ee8de84 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -53,6 +53,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -330,6 +331,7 @@ static void pc_q35_init(MachineState *machine)
 
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+                               x86_nvdimm_acpi_dsmio,
                                pcms->fw_cfg, OBJECT(pcms));
     }
 }
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 523a9b3d4a..5fe440861e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,6 +25,7 @@
 
 #include "hw/mem/pc-dimm.h"
 #include "hw/acpi/bios-linker-loader.h"
+#include "hw/acpi/aml-build.h"
 
 #define NVDIMM_DEBUG 0
 #define nvdimm_debug(fmt, ...)                                \
@@ -140,10 +141,12 @@ struct NVDIMMState {
      */
     int32_t persistence;
     char    *persistence_string;
+    struct AcpiGenericAddress dsm_io;
 };
 typedef struct NVDIMMState NVDIMMState;
 
 void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+                            struct AcpiGenericAddress dsm_io,
                             FWCfgState *fw_cfg, Object *owner);
 void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                        BIOSLinker *linker, NVDIMMState *state,
-- 
2.17.1

Re: [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size

[Igor Mammedov]

On Fri, 4 Oct 2019 16:52:59 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
> 
> This patch makes IO base and size configurable 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>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/acpi/nvdimm.c        | 32 ++++++++++++++++++++++----------
>  hw/i386/acpi-build.c    |  6 ++++++
>  hw/i386/acpi-build.h    |  3 +++
>  hw/i386/pc_piix.c       |  2 ++
>  hw/i386/pc_q35.c        |  2 ++
>  include/hw/mem/nvdimm.h |  3 +++
>  6 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fdad6dc3f..f91eea3802 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
>  }
>  
>  void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> +                            struct AcpiGenericAddress dsm_io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
> +    state->dsm_io = dsm_io;
>      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", dsm_io.bit_width >> 3);
> +    memory_region_add_subregion(io, dsm_io.address, &state->io_mr);
>  
>      state->dsm_mem = g_array_new(false, true /* clear */, 1);
>      acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
> @@ -959,12 +961,14 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
>  
>  #define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
>  
> -static void nvdimm_build_common_dsm(Aml *dev)
> +static void nvdimm_build_common_dsm(Aml *dev,
> +                                    NVDIMMState *nvdimm_state)
>  {
>      Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
>      Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
>      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>      uint8_t byte_list[1];
> +    AmlRegionSpace rs;
>  
>      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
>      uuid = aml_arg(0);
> @@ -975,9 +979,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
>  
>      aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
>  
> +    if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
> +        rs = AML_SYSTEM_IO;
> +    } else {
> +        rs = AML_SYSTEM_MEMORY;
> +    }
> +
>      /* map DSM memory and IO into ACPI namespace. */
> -    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
> -               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
> +    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> +               aml_int(nvdimm_state->dsm_io.address),
> +               nvdimm_state->dsm_io.bit_width >> 3));
>      aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
>                 AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
>  
> @@ -992,7 +1003,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
>                        AML_PRESERVE);
>      aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> -               NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
> +              (nvdimm_state->dsm_io.bit_width >> 3) * BITS_PER_BYTE));
Why are you converting bits to bytes and then back to bits, here?


>      aml_append(method, field);
>  
>      /*
> @@ -1260,7 +1271,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  }
>  
>  static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> -                              BIOSLinker *linker, GArray *dsm_dma_area,
> +                              BIOSLinker *linker,
> +                              NVDIMMState *nvdimm_state,
>                                uint32_t ram_slots)
>  {
>      Aml *ssdt, *sb_scope, *dev;
> @@ -1288,7 +1300,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>       */
>      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>  
> -    nvdimm_build_common_dsm(dev);
> +    nvdimm_build_common_dsm(dev, nvdimm_state);
>  
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
> @@ -1307,7 +1319,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>                                                 NVDIMM_ACPI_MEM_ADDR);
>  
>      bios_linker_loader_alloc(linker,
> -                             NVDIMM_DSM_MEM_FILE, dsm_dma_area,
> +                             NVDIMM_DSM_MEM_FILE, 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),
> @@ -1329,7 +1341,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>          return;
>      }
>  
> -    nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
> +    nvdimm_build_ssdt(table_offsets, table_data, linker, state,
>                        ram_slots);
>  
>      device_list = nvdimm_get_device_list();
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1d077a7cb7..b5170912a8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -126,6 +126,12 @@ typedef struct FwCfgTPMConfig {
>  
>  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
>  
> +const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
> +    .space_id = AML_AS_SYSTEM_IO,
> +    .address = NVDIMM_ACPI_IO_BASE,
> +    .bit_width = NVDIMM_ACPI_IO_LEN << 3
> +};
> +
>  static void init_common_fadt_data(MachineState *ms, Object *o,
>                                    AcpiFadtData *data)
>  {
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 007332e51c..74df5fc612 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -1,6 +1,9 @@
>  
>  #ifndef HW_I386_ACPI_BUILD_H
>  #define HW_I386_ACPI_BUILD_H
> +#include "hw/acpi/acpi-defs.h"
> +
> +extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>  
>  void acpi_setup(void);
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6824b72124..78521cf017 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -58,6 +58,7 @@
>  #include "migration/misc.h"
>  #include "kvm_i386.h"
>  #include "sysemu/numa.h"
> +#include "hw/i386/acpi-build.h"
>  
>  #define MAX_IDE_BUS 2
>  
> @@ -303,6 +304,7 @@ else {
>  
>      if (machine->nvdimms_state->is_enabled) {
>          nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> +                               x86_nvdimm_acpi_dsmio,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 8fad20f314..d53ee8de84 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -53,6 +53,7 @@
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/numa.h"
> +#include "hw/i386/acpi-build.h"
>  
>  /* ICH9 AHCI has 6 ports */
>  #define MAX_SATA_PORTS     6
> @@ -330,6 +331,7 @@ static void pc_q35_init(MachineState *machine)
>  
>      if (machine->nvdimms_state->is_enabled) {
>          nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
> +                               x86_nvdimm_acpi_dsmio,
>                                 pcms->fw_cfg, OBJECT(pcms));
>      }
>  }
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 523a9b3d4a..5fe440861e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -25,6 +25,7 @@
>  
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/acpi/bios-linker-loader.h"
> +#include "hw/acpi/aml-build.h"
>  
>  #define NVDIMM_DEBUG 0
>  #define nvdimm_debug(fmt, ...)                                \
> @@ -140,10 +141,12 @@ struct NVDIMMState {
>       */
>      int32_t persistence;
>      char    *persistence_string;
> +    struct AcpiGenericAddress dsm_io;
>  };
>  typedef struct NVDIMMState NVDIMMState;
>  
>  void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
> +                            struct AcpiGenericAddress dsm_io,
>                              FWCfgState *fw_cfg, Object *owner);
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         BIOSLinker *linker, NVDIMMState *state,

[PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure

[Shameer Kolothum]

From: Kwangwoo Lee <kwangwoo.lee@sk.com>

Pre-plug and plug handlers are prepared for NVDIMM support.
Please note nvdimm_support is not yet enabled.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/Kconfig           |  1 +
 hw/arm/virt-acpi-build.c |  6 ++++++
 hw/arm/virt.c            | 22 +++++++++++++++++++++-
 hw/mem/Kconfig           |  2 +-
 include/hw/arm/virt.h    |  1 +
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index c6e7782580..851dd81289 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM_VIRT
     select DIMM
     select ACPI_MEMORY_HOTPLUG
     select ACPI_HW_REDUCED
+    select ACPI_NVDIMM
 
 config CHEETAH
     bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 074e0c858e..4e63f5da48 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
+#include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "kvm_arm.h"
@@ -835,6 +836,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         }
     }
 
+    if (ms->nvdimms_state->is_enabled) {
+        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+                          ms->nvdimms_state, ms->ram_slots);
+    }
+
     if (its_class_name() && !vmc->no_its) {
         acpi_add_table(table_offsets, tables_blob);
         build_iort(tables_blob, tables->linker, vms);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2607..30bc8a7803 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
+    [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -1750,6 +1751,18 @@ static void machvirt_init(MachineState *machine)
 
     create_platform_bus(vms, pic);
 
+    if (machine->nvdimms_state->is_enabled) {
+        const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
+            .space_id = AML_AS_SYSTEM_MEMORY,
+            .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
+            .bit_width = NVDIMM_ACPI_IO_LEN << 3
+        };
+
+        nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
+                               arm_virt_nvdimm_acpi_dsmio,
+                               vms->fw_cfg, OBJECT(vms));
+    }
+
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.nb_cpus = smp_cpus;
     vms->bootinfo.board_id = -1;
@@ -1916,9 +1929,10 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                  Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    MachineState *ms = MACHINE(hotplug_dev);
     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    if (is_nvdimm) {
+    if (is_nvdimm && (!ms->nvdimms_state->is_enabled)) {
         error_setg(errp, "nvdimm is not yet supported");
         return;
     }
@@ -1937,6 +1951,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 {
     HotplugHandlerClass *hhc;
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    MachineState *ms = MACHINE(hotplug_dev);
+    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
     Error *local_err = NULL;
 
     pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
@@ -1944,6 +1960,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (is_nvdimm) {
+        nvdimm_plug(ms->nvdimms_state);
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
 out:
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index 620fd4cb59..0d5f8f321a 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -8,4 +8,4 @@ config MEM_DEVICE
 config NVDIMM
     bool
     default y
-    depends on PC
+    depends on PC || ARM_VIRT
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 0b41083e9d..06d5e75611 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -79,6 +79,7 @@ enum {
     VIRT_SECURE_MEM,
     VIRT_PCDIMM_ACPI,
     VIRT_ACPI_GED,
+    VIRT_NVDIMM_ACPI,
     VIRT_LOWMEMMAP_LAST,
 };
 
-- 
2.17.1

Re: [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure

[Igor Mammedov]

On Fri, 4 Oct 2019 16:53:00 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
> 
> Pre-plug and plug handlers are prepared for NVDIMM support.

Prepare pre-plug and plug handlers for NVDIMM support.

> Please note nvdimm_support is not yet enabled.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/Kconfig           |  1 +
>  hw/arm/virt-acpi-build.c |  6 ++++++
>  hw/arm/virt.c            | 22 +++++++++++++++++++++-
>  hw/mem/Kconfig           |  2 +-
>  include/hw/arm/virt.h    |  1 +
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index c6e7782580..851dd81289 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -24,6 +24,7 @@ config ARM_VIRT
>      select DIMM
>      select ACPI_MEMORY_HOTPLUG
>      select ACPI_HW_REDUCED
> +    select ACPI_NVDIMM
>  
>  config CHEETAH
>      bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 074e0c858e..4e63f5da48 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> +#include "hw/mem/nvdimm.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "kvm_arm.h"
> @@ -835,6 +836,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>          }
>      }
>  
> +    if (ms->nvdimms_state->is_enabled) {
> +        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> +                          ms->nvdimms_state, ms->ram_slots);
> +    }
> +
>      if (its_class_name() && !vmc->no_its) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_iort(tables_blob, tables->linker, vms);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4bedc2607..30bc8a7803 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
>      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> +    [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -1750,6 +1751,18 @@ static void machvirt_init(MachineState *machine)
>  
>      create_platform_bus(vms, pic);
>  
> +    if (machine->nvdimms_state->is_enabled) {
> +        const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
> +            .space_id = AML_AS_SYSTEM_MEMORY,
> +            .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
> +            .bit_width = NVDIMM_ACPI_IO_LEN << 3
> +        };
> +
> +        nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
> +                               arm_virt_nvdimm_acpi_dsmio,
> +                               vms->fw_cfg, OBJECT(vms));
> +    }
> +
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.nb_cpus = smp_cpus;
>      vms->bootinfo.board_id = -1;
> @@ -1916,9 +1929,10 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                   Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +    MachineState *ms = MACHINE(hotplug_dev);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (is_nvdimm) {
> +    if (is_nvdimm && (!ms->nvdimms_state->is_enabled)) {
shouldn't shouldn't this hunk go to 5/5 where functionality is eventually
enabled and use the same error message as in pc_memory_pre_plug()

>          error_setg(errp, "nvdimm is not yet supported");
>          return;
>      }
> @@ -1937,6 +1951,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>  {
>      HotplugHandlerClass *hhc;
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +    MachineState *ms = MACHINE(hotplug_dev);
> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      Error *local_err = NULL;
>  
>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> @@ -1944,6 +1960,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> +    if (is_nvdimm) {
> +        nvdimm_plug(ms->nvdimms_state);
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
>  out:
> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> index 620fd4cb59..0d5f8f321a 100644
> --- a/hw/mem/Kconfig
> +++ b/hw/mem/Kconfig
> @@ -8,4 +8,4 @@ config MEM_DEVICE
>  config NVDIMM
>      bool
>      default y
> -    depends on PC
> +    depends on PC || ARM_VIRT
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 0b41083e9d..06d5e75611 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -79,6 +79,7 @@ enum {
>      VIRT_SECURE_MEM,
>      VIRT_PCDIMM_ACPI,
>      VIRT_ACPI_GED,
> +    VIRT_NVDIMM_ACPI,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  

[PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT

[Shameer Kolothum]

From: Eric Auger <eric.auger@redhat.com>

In case of NV-DIMM slots, let's add /pmem DT nodes.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/boot.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c264864c11..bd6d72b33e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -20,6 +20,7 @@
 #include "hw/boards.h"
 #include "sysemu/reset.h"
 #include "hw/loader.h"
+#include "hw/mem/memory-device.h"
 #include "elf.h"
 #include "sysemu/device_tree.h"
 #include "qemu/config-file.h"
@@ -523,6 +524,44 @@ static void fdt_add_psci_node(void *fdt)
     qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
 }
 
+static int fdt_add_pmem_node(void *fdt, uint32_t acells, uint32_t scells)
+{
+    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
+    MemoryDeviceInfo *mi;
+    int ret;
+
+    for (info = info_list; info != NULL; info = info->next) {
+        mi = info->value;
+
+        if (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM) {
+            PCDIMMDeviceInfo *di = mi->u.nvdimm.data;
+            char *nodename;
+
+            nodename = g_strdup_printf("/pmem@%" PRIx64, di->addr);
+            qemu_fdt_add_subnode(fdt, nodename);
+            qemu_fdt_setprop_string(fdt, nodename, "compatible", "pmem-region");
+            ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells,
+                                               di->addr, scells, di->size);
+            /* only set the NUMA ID if it is specified */
+            if (!ret && di->node >= 0) {
+                ret = qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id",
+                                            di->node);
+            }
+
+            g_free(nodename);
+
+            if (ret < 0) {
+                fprintf(stderr, "couldn't add NVDIMM /memory@%"PRIx64" node\n",
+                        di->addr);
+                goto out;
+            }
+        }
+    }
+out:
+    qapi_free_MemoryDeviceInfoList(info_list);
+    return ret;
+}
+
 int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as, MachineState *ms)
 {
@@ -622,6 +661,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
     }
 
+    rc = fdt_add_pmem_node(fdt, acells, scells);
+    if (rc < 0) {
+        fprintf(stderr, "couldn't add pmem memory nodes\n");
+        goto fail;
+    }
+
     rc = fdt_path_offset(fdt, "/chosen");
     if (rc < 0) {
         qemu_fdt_add_subnode(fdt, "/chosen");
-- 
2.17.1

Re: [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT

[Igor Mammedov]

On Fri, 4 Oct 2019 16:53:01 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Eric Auger <eric.auger@redhat.com>
> 
> In case of NV-DIMM slots, let's add /pmem DT nodes
Why should we do it for NVDIMM but not for PC-DIMM?

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/boot.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c264864c11..bd6d72b33e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -20,6 +20,7 @@
>  #include "hw/boards.h"
>  #include "sysemu/reset.h"
>  #include "hw/loader.h"
> +#include "hw/mem/memory-device.h"
>  #include "elf.h"
>  #include "sysemu/device_tree.h"
>  #include "qemu/config-file.h"
> @@ -523,6 +524,44 @@ static void fdt_add_psci_node(void *fdt)
>      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
>  }
>  
> +static int fdt_add_pmem_node(void *fdt, uint32_t acells, uint32_t scells)
> +{
> +    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
> +    MemoryDeviceInfo *mi;
> +    int ret;
> +
> +    for (info = info_list; info != NULL; info = info->next) {
> +        mi = info->value;
> +
> +        if (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM) {
> +            PCDIMMDeviceInfo *di = mi->u.nvdimm.data;
> +            char *nodename;
> +
> +            nodename = g_strdup_printf("/pmem@%" PRIx64, di->addr);
> +            qemu_fdt_add_subnode(fdt, nodename);
> +            qemu_fdt_setprop_string(fdt, nodename, "compatible", "pmem-region");
> +            ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells,
> +                                               di->addr, scells, di->size);
> +            /* only set the NUMA ID if it is specified */
> +            if (!ret && di->node >= 0) {
> +                ret = qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id",
> +                                            di->node);
> +            }
> +
> +            g_free(nodename);
> +
> +            if (ret < 0) {
> +                fprintf(stderr, "couldn't add NVDIMM /memory@%"PRIx64" node\n",
shouldn't it be s:/memory:/pmem:

and maybe move error printing outside like it's done in fdt_add_memory_node()
to be consistent with current code.

> +                        di->addr);
> +                goto out;
> +            }
> +        }
> +    }
> +out:
> +    qapi_free_MemoryDeviceInfoList(info_list);
> +    return ret;
> +}
> +
>  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as, MachineState *ms)
>  {
> @@ -622,6 +661,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          }
>      }
>  
> +    rc = fdt_add_pmem_node(fdt, acells, scells);
> +    if (rc < 0) {
> +        fprintf(stderr, "couldn't add pmem memory nodes\n");
> +        goto fail;
> +    }
> +
>      rc = fdt_path_offset(fdt, "/chosen");
>      if (rc < 0) {
>          qemu_fdt_add_subnode(fdt, "/chosen");

[PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support

[Shameer Kolothum]

This adds support for nvdimm hotplug events through GED
and enables nvdimm for the arm/virt. Now Guests with DT boot
can have nvdimm cold plug and with ACPI both cold/hot plug.

Hot removal functionality is not yet supported.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
 hw/acpi/generic_event_device.c         | 13 +++++++++++++
 hw/arm/virt.c                          | 20 +++++++++++++++-----
 include/hw/acpi/generic_event_device.h |  1 +
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
index 911a98255b..e3abe975bf 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -63,6 +63,7 @@ GED IO interface (4 byte access)
     bits:
        0: Memory hotplug event
        1: System power down event
+       2: NVDIMM hotplug event
     2-31: Reserved
 
 **write_access:**
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 9cee90cc70..ad1b684304 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -16,6 +16,7 @@
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
@@ -23,6 +24,7 @@
 static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
     ACPI_GED_PWR_DOWN_EVT,
+    ACPI_GED_NVDIMM_HOTPLUG_EVT,
 };
 
 /*
@@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                            aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
                                       aml_int(0x80)));
                 break;
+            case ACPI_GED_NVDIMM_HOTPLUG_EVT:
+                aml_append(if_ctx,
+                           aml_notify(aml_name("\\_SB.NVDR"),
+                                      aml_int(0x80)));
+                break;
             default:
                 /*
                  * Please make sure all the events in ged_supported_events[]
@@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
     AcpiGedState *s = ACPI_GED(hotplug_dev);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+            nvdimm_acpi_plug_cb(hotplug_dev, dev);
+        } else {
             acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
+        }
     } else {
         error_setg(errp, "virt: device plug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_MEM_HOTPLUG_EVT;
     } else if (ev & ACPI_POWER_DOWN_STATUS) {
         sel = ACPI_GED_PWR_DOWN_EVT;
+    } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
+        sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 30bc8a7803..acdcba9638 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
         event |= ACPI_GED_MEM_HOTPLUG_EVT;
     }
 
+    if (ms->nvdimms_state->is_enabled) {
+        event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
+    }
+
     dev = qdev_create(NULL, TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
 
@@ -1938,9 +1942,12 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 
     if (!vms->acpi_dev) {
-        error_setg(errp,
-                   "memory hotplug is not enabled: missing acpi-ged device");
-        return;
+        /* Allow nvdimm DT or cold plug */
+        if (!(is_nvdimm && !dev->hotplugged)) {
+            error_setg(errp,
+                       "memory hotplug is not enabled: missing acpi-ged device");
+            return;
+         }
     }
 
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
@@ -1964,8 +1971,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
         nvdimm_plug(ms->nvdimms_state);
     }
 
-    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
-    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
+    if (vms->acpi_dev) {
+        hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
+        hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
+    }
 out:
     error_propagate(errp, local_err);
 }
@@ -2073,6 +2082,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->plug = virt_machine_device_plug_cb;
     hc->unplug_request = virt_machine_device_unplug_request_cb;
     mc->numa_mem_supported = true;
+    mc->nvdimm_supported = true;
     mc->auto_enable_numa_with_memhp = true;
 }
 
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index d157eac088..9eb86ca4fd 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -82,6 +82,7 @@
  */
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
 #define ACPI_GED_PWR_DOWN_EVT      0x2
+#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
 
 typedef struct GEDState {
     MemoryRegion io;
-- 
2.17.1

Re: [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support

[Igor Mammedov]

On Fri, 4 Oct 2019 16:53:02 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This adds support for nvdimm hotplug events through GED
> and enables nvdimm for the arm/virt. Now Guests with DT boot
> can have nvdimm cold plug and with ACPI both cold/hot plug.
> 
> Hot removal functionality is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

I'd move out DT cold-plug out of this patch is it's not related
to this patch at all

otherwise patch looks good, so 
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 +++++++++++++
>  hw/arm/virt.c                          | 20 +++++++++++++++-----
>  include/hw/acpi/generic_event_device.h |  1 +
>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
> index 911a98255b..e3abe975bf 100644
> --- a/docs/specs/acpi_hw_reduced_hotplug.rst
> +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
> @@ -63,6 +63,7 @@ GED IO interface (4 byte access)
>      bits:
>         0: Memory hotplug event
>         1: System power down event
> +       2: NVDIMM hotplug event
>      2-31: Reserved
>  
>  **write_access:**
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..ad1b684304 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -16,6 +16,7 @@
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/irq.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>  #include "qemu/error-report.h"
> @@ -23,6 +24,7 @@
>  static const uint32_t ged_supported_events[] = {
>      ACPI_GED_MEM_HOTPLUG_EVT,
>      ACPI_GED_PWR_DOWN_EVT,
> +    ACPI_GED_NVDIMM_HOTPLUG_EVT,
>  };
>  
>  /*
> @@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
>                             aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>                                        aml_int(0x80)));
>                  break;
> +            case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> +                aml_append(if_ctx,
> +                           aml_notify(aml_name("\\_SB.NVDR"),
> +                                      aml_int(0x80)));
> +                break;
>              default:
>                  /*
>                   * Please make sure all the events in ged_supported_events[]
> @@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>      AcpiGedState *s = ACPI_GED(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +            nvdimm_acpi_plug_cb(hotplug_dev, dev);
> +        } else {
>              acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
> +        }
>      } else {
>          error_setg(errp, "virt: device plug request for unsupported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>          sel = ACPI_GED_MEM_HOTPLUG_EVT;
>      } else if (ev & ACPI_POWER_DOWN_STATUS) {
>          sel = ACPI_GED_PWR_DOWN_EVT;
> +    } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>      } else {
>          /* Unknown event. Return without generating interrupt. */
>          warn_report("GED: Unsupported event %d. No irq injected", ev);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 30bc8a7803..acdcba9638 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
>          event |= ACPI_GED_MEM_HOTPLUG_EVT;
>      }
>  
> +    if (ms->nvdimms_state->is_enabled) {
> +        event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> +    }
> +
>      dev = qdev_create(NULL, TYPE_ACPI_GED);
>      qdev_prop_set_uint32(dev, "ged-event", event);
>  
> @@ -1938,9 +1942,12 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  
>      if (!vms->acpi_dev) {
> -        error_setg(errp,
> -                   "memory hotplug is not enabled: missing acpi-ged device");
> -        return;
> +        /* Allow nvdimm DT or cold plug */
> +        if (!(is_nvdimm && !dev->hotplugged)) {
> +            error_setg(errp,
> +                       "memory hotplug is not enabled: missing acpi-ged device");
> +            return;
> +         }



>      }
>  
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> @@ -1964,8 +1971,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>          nvdimm_plug(ms->nvdimms_state);
>      }
>  
> -    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> -    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
> +    if (vms->acpi_dev) {
> +        hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> +        hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
> +    }
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -2073,6 +2082,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      hc->plug = virt_machine_device_plug_cb;
>      hc->unplug_request = virt_machine_device_unplug_request_cb;
>      mc->numa_mem_supported = true;
> +    mc->nvdimm_supported = true;
>      mc->auto_enable_numa_with_memhp = true;
>  }
>  
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d157eac088..9eb86ca4fd 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -82,6 +82,7 @@
>   */
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  #define ACPI_GED_PWR_DOWN_EVT      0x2
> +#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>  
>  typedef struct GEDState {
>      MemoryRegion io;

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Auger Eric]

Hi Shameer,

On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> This series adds NVDIMM support to arm/virt platform.
> This has a dependency on [0] and make use of the GED
> device for NVDIMM hotplug events. The series reuses
> some of the patches posted by Eric in his earlier
> attempt here[1].
> 
> Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2].
> 
> I have done basic sanity testing of NVDIMM deviecs with
devcies
> both ACPI and DT Guest boot. Further testing is always
> welcome.
> 
> Please let me know your feedback.

I tested it on my side. Looks to work pretty well.

one question: I noticed that when a NVDIMM slot is hotplugged one get
the following trace on guest:

nfit ACPI0012:00: found a zero length table '0' parsing nfit
pmem0: detected capacity change from 0 to 1073741824

Have you experienced the 0 length trace?

Besides when we reset the system we find the namespaces again using
"ndctl list -u" so the original bug seems to be fixed.

Did you try to mount a DAX FS. I can mount but with DAX forced off.

sudo mkdir /mnt/mem0
mkfs.xfs -f -m reflink=0 /dev/pmem0
sudo mount -o dax /dev/pmem0 /mnt/mem0
[ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
your own risk
[ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
DAX.
[ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
[ 2610.189797] XFS (pmem0): Ending clean mount

I fail to remember if it was the case months ago. I am not sure if it is
an issue in my guest .config or if there is something not yet supported
on aarch64? Did you try on your side?

Also if you forget to put the ",nvdimm" to the machvirt options you get,
on hotplug:
{"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
which is not correct anymore ;-)

Thanks

Eric


> 
> Thanks,
> Shameer
> 
> [0] https://patchwork.kernel.org/cover/11150345/
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
> 
> Eric Auger (1):
>   hw/arm/boot: Expose the pmem nodes in the DT
> 
> Kwangwoo Lee (2):
>   nvdimm: Use configurable ACPI IO base and size
>   hw/arm/virt: Add nvdimm hot-plug infrastructure
> 
> Shameer Kolothum (2):
>   hw/arm: Align ACPI blob len to PAGE size
>   hw/arm/virt: Add nvdimm hotplug support
> 
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 ++++++++
>  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
>  hw/arm/Kconfig                         |  1 +
>  hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
>  hw/arm/virt.c                          | 42 ++++++++++++++++++++----
>  hw/i386/acpi-build.c                   |  6 ++++
>  hw/i386/acpi-build.h                   |  3 ++
>  hw/i386/pc_piix.c                      |  2 ++
>  hw/i386/pc_q35.c                       |  2 ++
>  hw/mem/Kconfig                         |  2 +-
>  include/hw/acpi/generic_event_device.h |  1 +
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/mem/nvdimm.h                |  3 ++
>  15 files changed, 157 insertions(+), 17 deletions(-)
> 

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 18 October 2019 17:40
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> Hi Shameer,
> 
> On 10/4/19 5:52 PM, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > This has a dependency on [0] and make use of the GED
> > device for NVDIMM hotplug events. The series reuses
> > some of the patches posted by Eric in his earlier
> > attempt here[1].
> >
> > Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> > hot add case described here[2].
> >
> > I have done basic sanity testing of NVDIMM deviecs with
> devcies
> > both ACPI and DT Guest boot. Further testing is always
> > welcome.
> >
> > Please let me know your feedback.
> 
> I tested it on my side. Looks to work pretty well.

Thanks for giving this a spin.
 
> one question: I noticed that when a NVDIMM slot is hotplugged one get
> the following trace on guest:
> 
> nfit ACPI0012:00: found a zero length table '0' parsing nfit
> pmem0: detected capacity change from 0 to 1073741824
> 
> Have you experienced the 0 length trace?

I double checked and yes that trace is there. And I did a quick check with
x86 and it is not there. 

The reason looks like, ARM64 kernel receives an additional 8 bytes size when
the kernel evaluates the "_FIT" object. 

For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and 

X86 Guest kernel,
[    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8

ARM64 Guest,
[    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0

I am not sure how that size gets changed for ARM which results in
the above mentioned 0 length trace. I need to debug this further.

Please let me know if you have any pointers...
 
> Besides when we reset the system we find the namespaces again using
> "ndctl list -u" so the original bug seems to be fixed.
> 
> Did you try to mount a DAX FS. I can mount but with DAX forced off.
> sudo mkdir /mnt/mem0

Yes. I did try with DAX FS. But do we need to change the namespace mode to 
file system DAX mode?

I used the below command before attempting to mount with -o dax,

ndctl create-namespace -f -e namespace0.0 --mode=fsdax

And in order to do the above you might need the ZONE_DEVICE support
in the Kernel which in turn has dependency on hot remove. Hence I tried with
"arm64/mm: Enable memory hot remove" patches,

https://patchwork.kernel.org/cover/11185169/

> mkfs.xfs -f -m reflink=0 /dev/pmem0
> sudo mount -o dax /dev/pmem0 /mnt/mem0
> [ 2610.051830] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
> your own risk
> [ 2610.178580] XFS (pmem0): DAX unsupported by block device. Turning off
> DAX.
> [ 2610.180871] XFS (pmem0): Mounting V5 Filesystem
> [ 2610.189797] XFS (pmem0): Ending clean mount
> 
> I fail to remember if it was the case months ago. I am not sure if it is
> an issue in my guest .config or if there is something not yet supported
> on aarch64? Did you try on your side?
> 
> Also if you forget to put the ",nvdimm" to the machvirt options you get,
> on hotplug:
> {"error": {"class": "GenericError", "desc": "nvdimm is not yet supported"}}
> which is not correct anymore ;-)

Ok. I will check this.

Thanks,
Shameer
 
> Thanks
> 
> Eric
> 
> 
> >
> > Thanks,
> > Shameer
> >
> > [0] https://patchwork.kernel.org/cover/11150345/
> > [1] https://patchwork.kernel.org/cover/10830777/
> > [2] https://patchwork.kernel.org/patch/11154757/
> >
> > Eric Auger (1):
> >   hw/arm/boot: Expose the pmem nodes in the DT
> >
> > Kwangwoo Lee (2):
> >   nvdimm: Use configurable ACPI IO base and size
> >   hw/arm/virt: Add nvdimm hot-plug infrastructure
> >
> > Shameer Kolothum (2):
> >   hw/arm: Align ACPI blob len to PAGE size
> >   hw/arm/virt: Add nvdimm hotplug support
> >
> >  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
> >  hw/acpi/generic_event_device.c         | 13 ++++++++
> >  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
> >  hw/arm/Kconfig                         |  1 +
> >  hw/arm/boot.c                          | 45
> ++++++++++++++++++++++++++
> >  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
> >  hw/arm/virt.c                          | 42
> ++++++++++++++++++++----
> >  hw/i386/acpi-build.c                   |  6 ++++
> >  hw/i386/acpi-build.h                   |  3 ++
> >  hw/i386/pc_piix.c                      |  2 ++
> >  hw/i386/pc_q35.c                       |  2 ++
> >  hw/mem/Kconfig                         |  2 +-
> >  include/hw/acpi/generic_event_device.h |  1 +
> >  include/hw/arm/virt.h                  |  1 +
> >  include/hw/mem/nvdimm.h                |  3 ++
> >  15 files changed, 157 insertions(+), 17 deletions(-)
> >

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

[-- Attachment #1: Type: text/plain, Size: 4993 bytes --]

Hi Eric/Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 22 October 2019 15:05
> To: 'Auger Eric' <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> <linuxarm@huawei.com>
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 

[..]

> > one question: I noticed that when a NVDIMM slot is hotplugged one get
> > the following trace on guest:
> >
> > nfit ACPI0012:00: found a zero length table '0' parsing nfit
> > pmem0: detected capacity change from 0 to 1073741824
> >
> > Have you experienced the 0 length trace?
> 
> I double checked and yes that trace is there. And I did a quick check with
> x86 and it is not there.
> 
> The reason looks like, ARM64 kernel receives an additional 8 bytes size when
> the kernel evaluates the "_FIT" object.
> 
> For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and
> 
> X86 Guest kernel,
> [    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8
> 
> ARM64 Guest,
> [    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0
> 
> I am not sure how that size gets changed for ARM which results in
> the above mentioned 0 length trace. I need to debug this further.
> 
> Please let me know if you have any pointers...

I spend some time debugging this further and it looks like the AML code
behaves differently on x86 and ARM64.

Booted guest with nvdimm mem, and used SSDT override with dbg prints
added,

-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \

On X86,
-----------

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0

[AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
[AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
[AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
[AML]"NVDIMM-FIT: Rcvd buf size 000000B8"

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
[AML]"NVDIMM-RFIT Rcvd buf size 00000004"
[AML]"NVDIMM-FIT: Rcvd buf size 00000000"
[AML]"NVDIMM-FIT: _FIT returned size 000000B8"

[ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  --> Guest receives correct size(0xb8) here 

On ARM64,
---------------

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
[AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
[AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
[AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

[AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All looks same as x86 up to here.
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  ---> The size goes wrong. 8 bytes instead of 4!.
[AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
[AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
[AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes wrong. 8 bytes instead of 4!.

[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.  -->Another read is attempted 
[Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3  --> Error status returned

[AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
[AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
[AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
[AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
[AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong size returned.
[ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0   -->Kernel gets 0xc0 instead of 0xb8


It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and not
sure this is a 32/64bit issue or not. I am attaching the SSDT files with the above
dbg prints added. Could you please take a look and let me know what actually is
going on here...

Much appreciated,
Shameer.



[-- Attachment #2: SSDT-dbg-arm64.dsl --]
[-- Type: application/octet-stream, Size: 7096 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Mon Nov 18 16:09:53 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002EF (751)
 *     Revision         0x01
 *     Checksum         0xA4
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemMemory, 0x09090000, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }

                NTFI = Local6
                printf("NVDIMM-NCAL: Rcvd RLEN %o", RLEN)
                Local1 = (RLEN - 0x04)
                Local1 = (Local1 << 0x03)
                printf("NVDIMM-NCAL: Creating OBUF with %o bits", Local1)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                printf("NVDIMM-NCAL: Created  BUF(Local7) size %o", SizeOf(Local7))
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = Buffer (Zero){}
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                           printf("NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }
                      
                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xFFFF0000)
}


[-- Attachment #3: SSDT-dbg-x86.dsl --]
[-- Type: application/octet-stream, Size: 7088 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Mon Nov 18 15:53:14 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002ED (749)
 *     Revision         0x01
 *     Checksum         0x3D
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }

                NTFI = Local6
                printf("NVDIMM-NCAL: Rcvd RLEN %o", RLEN)
                Local1 = (RLEN - 0x04)
                Local1 = (Local1 << 0x03)
                printf("NVDIMM-NCAL: Creating OBUF with %o bits", Local1)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                printf("NVDIMM-NCAL: Created  BUF(Local7) size %o", SizeOf(Local7))
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = Buffer (Zero){}
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                           printf("NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }
                      
                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xBFBFD000)
}

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Igor Mammedov]

On Mon, 25 Nov 2019 13:20:02 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> Hi Eric/Igor,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 22 October 2019 15:05
> > To: 'Auger Eric' <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org; imammedo@redhat.com
> > Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> > <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> > <linuxarm@huawei.com>
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support

not related to problem discussed in this patch but you probably
need to update docs/specs/acpi_nvdimm.txt to account for your changes

> >   
> 
> [..]
> 
> > > one question: I noticed that when a NVDIMM slot is hotplugged one get
> > > the following trace on guest:
> > >
> > > nfit ACPI0012:00: found a zero length table '0' parsing nfit
> > > pmem0: detected capacity change from 0 to 1073741824
> > >
> > > Have you experienced the 0 length trace?  
> > 
> > I double checked and yes that trace is there. And I did a quick check with
> > x86 and it is not there.
> > 
> > The reason looks like, ARM64 kernel receives an additional 8 bytes size when
> > the kernel evaluates the "_FIT" object.
> > 
> > For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and
> > 
> > X86 Guest kernel,
> > [    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8
> > 
> > ARM64 Guest,
> > [    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0
> > 
> > I am not sure how that size gets changed for ARM which results in
> > the above mentioned 0 length trace. I need to debug this further.
> > 
> > Please let me know if you have any pointers...  
> 
> I spend some time debugging this further and it looks like the AML code
> behaves differently on x86 and ARM64.
FIT table is built dynamically and you are the first to debug
such issue.
(apart from the author the NVDIMM code.
 btw: why NVDIMM author is not on CC list???)


> Booted guest with nvdimm mem, and used SSDT override with dbg prints
> added,
> 
> -object memory-backend-ram,id=mem1,size=1G \
> -device nvdimm,id=dimm1,memdev=mem1 \
> 
> On X86,
> -----------
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> 
> [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  --> Guest receives correct size(0xb8) here 
> 
> On ARM64,
> ---------------
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
> [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
> 
> [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All looks same as x86 up to here.
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  ---> The size goes wrong. 8 bytes instead of 4!.

> [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes wrong. 8 bytes instead of 4!.

                Local1 = SizeOf (Local0)
                printf("NVDIMM-RFIT Rcvd buf size %o", Local1)

                Local1 -= 0x04
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you see are consistent

                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)

 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.  -->Another read is attempted 
> [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3  --> Error status returned

status 3 means that QEMU didn't like content of NRAM, and there is only
1 place like this in nvdimm_dsm_func_read_fit()
    if (read_fit->offset > fit->len) {                                           
        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;                         
        goto exit;                                                               
    }

so I'd start looking from here and check that QEMU gets expected data
in nvdimm_dsm_write(). In other words I'd try to trace/compare
content of DSM buffer (from qemu side).


> [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
RFIT returns 0-sized buffer in case of error

> [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong size returned.
after that it goes

                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
  --> here, probably on the second iteration of the loop taking Zero
      size as EOF sign but without checking for any error (RSTA !=0)
      and returning partial buffer only

                        {
                           printf("NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }

relevant code in QEMU that builds this AML is in nvdimm_build_fit()

> [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0   -->Kernel gets 0xc0 instead of 0xb8
> 
> 
> It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and not
> sure this is a 32/64bit issue or not. I am attaching the SSDT files with the above
> dbg prints added. Could you please take a look and let me know what actually is
> going on here...

diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl 
--- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
+++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
@@[...]
@@ -28,7 +28,7 @@
             Method (NCAL, 5, Serialized)
             {
                 Local6 = MEMA /* \MEMA */
-                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
+                OperationRegion (NPIO, SystemMemory, 0x09090000, 0x04)
that's fine and matches your code

                 OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                 Field (NPIO, DWordAcc, NoLock, Preserve)
                 {
@@ -210,6 +210,6 @@
         }
     }
 
-    Name (MEMA, 0xBFBFD000)
+    Name (MEMA, 0xFFFF0000)

However value here is suspicious. If I recall right it should
point to DMS buffer allocated by firmware somewhere in the guest RAM.


 }
 
> Much appreciated,
> Shameer.
> 
> 

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 25 November 2019 15:46
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Auger Eric <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> On Mon, 25 Nov 2019 13:20:02 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Hi Eric/Igor,
> >
> > > -----Original Message-----
> > > From: Shameerali Kolothum Thodi
> > > Sent: 22 October 2019 15:05
> > > To: 'Auger Eric' <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> > > qemu-arm@nongnu.org; imammedo@redhat.com
> > > Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> > > <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> > > <linuxarm@huawei.com>
> > > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> not related to problem discussed in this patch but you probably
> need to update docs/specs/acpi_nvdimm.txt to account for your changes

Ok.

> > >
> >
> > [..]
> >
> > > > one question: I noticed that when a NVDIMM slot is hotplugged one get
> > > > the following trace on guest:
> > > >
> > > > nfit ACPI0012:00: found a zero length table '0' parsing nfit
> > > > pmem0: detected capacity change from 0 to 1073741824
> > > >
> > > > Have you experienced the 0 length trace?
> > >
> > > I double checked and yes that trace is there. And I did a quick check with
> > > x86 and it is not there.
> > >
> > > The reason looks like, ARM64 kernel receives an additional 8 bytes size
> when
> > > the kernel evaluates the "_FIT" object.
> > >
> > > For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and
> > >
> > > X86 Guest kernel,
> > > [    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8
> > >
> > > ARM64 Guest,
> > > [    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0
> > >
> > > I am not sure how that size gets changed for ARM which results in
> > > the above mentioned 0 length trace. I need to debug this further.
> > >
> > > Please let me know if you have any pointers...
> >
> > I spend some time debugging this further and it looks like the AML code
> > behaves differently on x86 and ARM64.
> FIT table is built dynamically and you are the first to debug
> such issue.
> (apart from the author the NVDIMM code.
:)
>  btw: why NVDIMM author is not on CC list???)

Right. Missed that. CCd.
 
> 
> > Booted guest with nvdimm mem, and used SSDT override with dbg prints
> > added,
> >
> > -object memory-backend-ram,id=mem1,size=1G \
> > -device nvdimm,id=dimm1,memdev=mem1 \
> >
> > On X86,
> > -----------
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8
> Dirty Yes.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> > [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> > [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size
> 0xb8 Dirty No.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> > [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> > [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> > [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> >
> > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  -->
> Guest receives correct size(0xb8) here
> >
> > On ARM64,
> > ---------------
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8
> Dirty Yes.
> > [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> > [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> > [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> >
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size
> 0xb8 Dirty No.
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 0
> >
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All
> looks same as x86 up to here.
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  --->
> The size goes wrong. 8 bytes instead of 4!.
> 
> > [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes
> wrong. 8 bytes instead of 4!.
> 
>                 Local1 = SizeOf (Local0)
>                 printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
> 
>                 Local1 -= 0x04
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you
> see are consistent

Hmm...IIUC, Method(RFIT, ) creates a buffer of size 0x4 as per 
this,

"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"

And this is the buffer received by Method(_FIT, ), 

                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("NVDIMM-FIT: Rcvd buf size %o", Local1)

But "NVDIMM-FIT: Rcvd buf size 0000000000000008".

As you can see above, in the first iteration the buffer size received by _FIT is
actually what RFIT created(0xB8).

> 
>                 If ((Local1 == Zero))
>                 {
>                     Return (Buffer (Zero){})
>                 }
> 
>                 CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
>                 printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o",
> Local1)
> 
> 
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size
> 0xb8 Dirty No.  -->Another read is attempted
> > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> func_ret_status 3  --> Error status returned
> 
> status 3 means that QEMU didn't like content of NRAM, and there is only
> 1 place like this in nvdimm_dsm_func_read_fit()
>     if (read_fit->offset > fit->len) {
>         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
>         goto exit;
>     }
> 
> so I'd start looking from here and check that QEMU gets expected data
> in nvdimm_dsm_write(). In other words I'd try to trace/compare
> content of DSM buffer (from qemu side).

I had printed the DSM buffer previously and it looked same, I will double check
that.
 
> 
> > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"
> RFIT returns 0-sized buffer in case of error

Yes.
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }
 
> > [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong
> size returned.
> after that it goes
> 
>                     Local0 = Buffer (Zero){}
>                     Local0 = RFIT (Local3)
>                     Local1 = SizeOf (Local0)
>                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
>                     If ((RSTA == 0x0100))
>                     {
>                         Local2 = Buffer (Zero){}
>                         Local3 = Zero
>                     }
>                     Else
>                     {
>                         If ((Local1 == Zero))
>   --> here, probably on the second iteration of the loop taking Zero
>       size as EOF sign but without checking for any error (RSTA !=0)
>       and returning partial buffer only

I guess you meant the third iteration. Yes, it doesn't check for RSTA != 0
case. And in the second iteration, since status == 0 and buffer len received is
8 bytes, 
                        Concatenate (Local2, Local0, Local2)

The final buffer returned to Guest becomes 0xC0 (0xb8 + 0x8).
 
>                         {
>                            printf("NVDIMM-FIT: _FIT returned size %o",
> SizeOf(Local2))
>                            Return (Local2)
>                         }
> 
> relevant code in QEMU that builds this AML is in nvdimm_build_fit()
> 
> > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0
> -->Kernel gets 0xc0 instead of 0xb8
> >
> >
> > It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> > ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and
> not
> > sure this is a 32/64bit issue or not. I am attaching the SSDT files with the
> above
> > dbg prints added. Could you please take a look and let me know what actually
> is
> > going on here...
> 
> diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl
> --- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
> +++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
> @@[...]
> @@ -28,7 +28,7 @@
>              Method (NCAL, 5, Serialized)
>              {
>                  Local6 = MEMA /* \MEMA */
> -                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
> +                OperationRegion (NPIO, SystemMemory, 0x09090000,
> 0x04)
> that's fine and matches your code
> 
>                  OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
>                  Field (NPIO, DWordAcc, NoLock, Preserve)
>                  {
> @@ -210,6 +210,6 @@
>          }
>      }
> 
> -    Name (MEMA, 0xBFBFD000)
> +    Name (MEMA, 0xFFFF0000)
> 
> However value here is suspicious. If I recall right it should
> point to DMS buffer allocated by firmware somewhere in the guest RAM.

But then it will be horribly wrong and will result in inconsistent behavior
as well, right?

Thanks,
Shameer
 
> 
>  }
> 
> > Much appreciated,
> > Shameer.
> >
> >

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Igor Mammedov]

On Mon, 25 Nov 2019 16:25:54 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 25 November 2019 15:46
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: Auger Eric <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org; peter.maydell@linaro.org;
> > shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com; Linuxarm <linuxarm@huawei.com>
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> > 
> > On Mon, 25 Nov 2019 13:20:02 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > Hi Eric/Igor,
> > >  
> > > > -----Original Message-----
> > > > From: Shameerali Kolothum Thodi
> > > > Sent: 22 October 2019 15:05
> > > > To: 'Auger Eric' <eric.auger@redhat.com>; qemu-devel@nongnu.org;
> > > > qemu-arm@nongnu.org; imammedo@redhat.com
> > > > Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; xuwei (O)
> > > > <xuwei5@huawei.com>; lersek@redhat.com; Linuxarm
> > > > <linuxarm@huawei.com>
> > > > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support  
> > 
> > not related to problem discussed in this patch but you probably
> > need to update docs/specs/acpi_nvdimm.txt to account for your changes  
> 
> Ok.
> 
> > > >  
> > >
> > > [..]
> > >  
> > > > > one question: I noticed that when a NVDIMM slot is hotplugged one get
> > > > > the following trace on guest:
> > > > >
> > > > > nfit ACPI0012:00: found a zero length table '0' parsing nfit
> > > > > pmem0: detected capacity change from 0 to 1073741824
> > > > >
> > > > > Have you experienced the 0 length trace?  
> > > >
> > > > I double checked and yes that trace is there. And I did a quick check with
> > > > x86 and it is not there.
> > > >
> > > > The reason looks like, ARM64 kernel receives an additional 8 bytes size  
> > when  
> > > > the kernel evaluates the "_FIT" object.
> > > >
> > > > For the same test scenario, Qemu reports a FIT buffer size of 0xb8 and
> > > >
> > > > X86 Guest kernel,
> > > > [    1.601077] acpi_nfit_init: data 0xffff8a273dc12b18 sz 0xb8
> > > >
> > > > ARM64 Guest,
> > > > [    0.933133] acpi_nfit_init: data 0xffff00003cbe6018 sz 0xc0
> > > >
> > > > I am not sure how that size gets changed for ARM which results in
> > > > the above mentioned 0 length trace. I need to debug this further.
> > > >
> > > > Please let me know if you have any pointers...  
> > >
> > > I spend some time debugging this further and it looks like the AML code
> > > behaves differently on x86 and ARM64.  
> > FIT table is built dynamically and you are the first to debug
> > such issue.
> > (apart from the author the NVDIMM code.  
> :)
> >  btw: why NVDIMM author is not on CC list???)  
> 
> Right. Missed that. CCd.
>  
> >   
> > > Booted guest with nvdimm mem, and used SSDT override with dbg prints
> > > added,
> > >
> > > -object memory-backend-ram,id=mem1,size=1G \
> > > -device nvdimm,id=dimm1,memdev=mem1 \
> > >
> > > On X86,
> > > -----------
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8  
> > Dirty Yes.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 000000C0"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 000005E0 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 000000BC"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 000000BC"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 000000B8"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 000000B8"
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size  
> > 0xb8 Dirty No.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 00000020 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000004"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 00000004"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 00000000"
> > > [AML]"NVDIMM-FIT: _FIT returned size 000000B8"
> > >
> > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff9855bb9a7518 sz 0xb8  -->  
> > Guest receives correct size(0xb8) here  
> > >
> > > On ARM64,
> > > ---------------
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8  
> > Dirty Yes.  
> > > [Qemu]VDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 00000000000005E0 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
> > > [AML]"NVDIMM-RFIT Rcvd buf size 00000000000000BC"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 00000000000000B8"
> > >
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size  
> > 0xb8 Dirty No.  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 0  
> > >
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"  --> All  
> > looks same as x86 up to here.  
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"  --->  
> > The size goes wrong. 8 bytes instead of 4!.
> >   
> > > [AML]"NVDIMM-RFIT Rcvd buf size 0000000000000008"
> > > [AML]"NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000008"  --> Again size goes  
> > wrong. 8 bytes instead of 4!.
> > 
> >                 Local1 = SizeOf (Local0)
> >                 printf("NVDIMM-RFIT Rcvd buf size %o", Local1)
> > 
> >                 Local1 -= 0x04
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here you get -4 so sizes you
> > see are consistent  
> 
> Hmm...IIUC, Method(RFIT, ) creates a buffer of size 0x4 as per 
> this,
> 
> "NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004"
> 
> And this is the buffer received by Method(_FIT, ), 
> 
>                     Local0 = RFIT (Local3)
>                     Local1 = SizeOf (Local0)
>                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
> 
> But "NVDIMM-FIT: Rcvd buf size 0000000000000008".
> 
> As you can see above, in the first iteration the buffer size received by _FIT is
> actually what RFIT created(0xB8).
> 
> > 
> >                 If ((Local1 == Zero))
> >                 {
> >                     Return (Buffer (Zero){})
> >                 }
> > 
> >                 CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
> >                 printf("NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o",
> > Local1)
> > 
> >   
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size  
> > 0xb8 Dirty No.  -->Another read is attempted  
> > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > func_ret_status 3  --> Error status returned
> > 
> > status 3 means that QEMU didn't like content of NRAM, and there is only
> > 1 place like this in nvdimm_dsm_func_read_fit()
> >     if (read_fit->offset > fit->len) {
> >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> >         goto exit;
> >     }
> > 
> > so I'd start looking from here and check that QEMU gets expected data
> > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > content of DSM buffer (from qemu side).  
> 
> I had printed the DSM buffer previously and it looked same, I will double check
> that.
>  
> >   
> > > [AML]"NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
> > > [AML]"NVDIMM-NCAL: Creating OBUF with 0000000000000020 bits"
> > > [AML]"NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
> > > [AML]"NVDIMM-FIT: Rcvd buf size 0000000000000000"  
> > RFIT returns 0-sized buffer in case of error  
> 
> Yes.
>                 If ((Zero != STAU))
>                 {
>                     Return (Buffer (Zero){})
>                 }
>  
> > > [AML]"NVDIMM-FIT: _FIT returned size 00000000000000C0"   --> Wrong  
> > size returned.
> > after that it goes
> > 
> >                     Local0 = Buffer (Zero){}
> >                     Local0 = RFIT (Local3)
> >                     Local1 = SizeOf (Local0)
> >                     printf("NVDIMM-FIT: Rcvd buf size %o", Local1)
> >                     If ((RSTA == 0x0100))
> >                     {
> >                         Local2 = Buffer (Zero){}
> >                         Local3 = Zero
> >                     }
> >                     Else
> >                     {
> >                         If ((Local1 == Zero))  
> >   --> here, probably on the second iteration of the loop taking Zero  
> >       size as EOF sign but without checking for any error (RSTA !=0)
> >       and returning partial buffer only  
> 
> I guess you meant the third iteration. Yes, it doesn't check for RSTA != 0
> case. And in the second iteration, since status == 0 and buffer len received is
> 8 bytes, 
>                         Concatenate (Local2, Local0, Local2)
> 
> The final buffer returned to Guest becomes 0xC0 (0xb8 + 0x8).
>  
> >                         {
> >                            printf("NVDIMM-FIT: _FIT returned size %o",
> > SizeOf(Local2))
> >                            Return (Local2)
> >                         }
> > 
> > relevant code in QEMU that builds this AML is in nvdimm_build_fit()
> >   
> > > [ KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57ce18 sz 0xc0
> > -->Kernel gets 0xc0 instead of 0xb8
> > >
> > >
> > > It looks like the aml, "CreateField (ODAT, Zero, Local1, OBUF)" goes wrong for
> > > ARM64 when the buffer is all zeroes. My knowledge on aml is very limited and  
> > not  
> > > sure this is a 32/64bit issue or not. I am attaching the SSDT files with the  
> > above  
> > > dbg prints added. Could you please take a look and let me know what actually  
> > is  
> > > going on here...  
> > 
> > diff -u SSDT-dbg-x86.dsl SSDT-dbg-arm64.dsl
> > --- SSDT-dbg-x86.dsl	2019-11-25 14:50:22.024983026 +0100
> > +++ SSDT-dbg-arm64.dsl	2019-11-25 14:50:06.740690645 +0100
> > @@[...]
> > @@ -28,7 +28,7 @@
> >              Method (NCAL, 5, Serialized)
> >              {
> >                  Local6 = MEMA /* \MEMA */
> > -                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
> > +                OperationRegion (NPIO, SystemMemory, 0x09090000,
> > 0x04)
> > that's fine and matches your code
> > 
> >                  OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
> >                  Field (NPIO, DWordAcc, NoLock, Preserve)
> >                  {
> > @@ -210,6 +210,6 @@
> >          }
> >      }
> > 
> > -    Name (MEMA, 0xBFBFD000)
> > +    Name (MEMA, 0xFFFF0000)
> > 
> > However value here is suspicious. If I recall right it should
> > point to DMS buffer allocated by firmware somewhere in the guest RAM.  
> 
> But then it will be horribly wrong and will result in inconsistent behavior
> as well, right?

I'd thinks so. I'm not sure what happens but RAM starts at 0x100000000
which is above the address you have in MEMA so you shouldn't have
sensible data there and access probably should raise some sort of error.

CCing Drew

> 
> Thanks,
> Shameer
>  
> > 
> >  }
> >   
> > > Much appreciated,
> > > Shameer.
> > >
> > >  
> 

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Andrew Jones]

On Tue, Nov 26, 2019 at 09:56:55AM +0100, Igor Mammedov wrote:
> On Mon, 25 Nov 2019 16:25:54 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > > -    Name (MEMA, 0xBFBFD000)
> > > +    Name (MEMA, 0xFFFF0000)
> > > 
> > > However value here is suspicious. If I recall right it should
> > > point to DMS buffer allocated by firmware somewhere in the guest RAM.  
> > 
> > But then it will be horribly wrong and will result in inconsistent behavior
> > as well, right?
> 
> I'd thinks so. I'm not sure what happens but RAM starts at 0x100000000
> which is above the address you have in MEMA so you shouldn't have
> sensible data there and access probably should raise some sort of error.
> 
> CCing Drew

RAM starts at 0x40000000 (the 1G boundary). So 0xffff0000 is OK, assuming
you want this just below the 3G boundary. With TCG, a data abort exception
will be injected for any access to an unmapped region. With KVM, unless
paging is enabled in the guest, then reads of unmapped regions will return
zero and writes will be ignored.

Thanks,
drew

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

[-- Attachment #1: Type: text/plain, Size: 8502 bytes --]



> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 26 November 2019 08:57
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; drjones@redhat.com;
> xiaoguangrong.eric@gmail.com; shannon.zhaosl@gmail.com;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>; Auger Eric
> <eric.auger@redhat.com>; qemu-arm@nongnu.org; xuwei (O)
> <xuwei5@huawei.com>; lersek@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[..]

> > > 0xb8 Dirty No.  -->Another read is attempted
> > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> > > func_ret_status 3  --> Error status returned
> > >
> > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > 1 place like this in nvdimm_dsm_func_read_fit()
> > >     if (read_fit->offset > fit->len) {
> > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > >         goto exit;
> > >     }
> > >
> > > so I'd start looking from here and check that QEMU gets expected data
> > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > content of DSM buffer (from qemu side).
> >
> > I had printed the DSM buffer previously and it looked same, I will double check
> > that.

Tried printing the buffer in both Qemu/AML code.

On Amr64,
-------------
(1st iteration with offset 0)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0 FIT size 0xb8 Dirty Yes.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0xC0
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM BUF[0x8] = 0x0
[QEMU]NVDIMM BUF[0x9] = 0x0
[QEMU]NVDIMM BUF[0xA] = 0x38
[QEMU]NVDIMM BUF[0xB] = 0x0
[QEMU]NVDIMM BUF[0xC] = 0x2
[QEMU]NVDIMM BUF[0xD] = 0x0
[QEMU]NVDIMM BUF[0xE] = 0x3
[QEMU]NVDIMM BUF[0xF] = 0x0
.........
[QEMU]NVDIMM BUF[0xBC] = 0x0
[QEMU]NVDIMM BUF[0xBD] = 0x0
[QEMU]NVDIMM BUF[0xBE] = 0x0
[QEMU]NVDIMM BUF[0xBF] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0xc0 func_ret_status 0 

"AML:NVDIMM-NCAL: Rcvd RLEN 00000000000000C0"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x00000000000000C0"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000008] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000009] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000A] = 0x0000000000000038"
"AML:NVDIMM-NCAL TBUF[000000000000000B] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000C] = 0x0000000000000002"
"AML:NVDIMM-NCAL TBUF[000000000000000D] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[000000000000000E] = 0x0000000000000003"
"AML:NVDIMM-NCAL TBUF[000000000000000F] = 0x0000000000000000"
...........
"AML:NVDIMM-NCAL TBUF[00000000000000BC] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BD] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BE] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[00000000000000BF] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 00000000000000BC"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 00000000000000BC"
"AML:NVDIMM-RFIT Rcvd buf size 00000000000000BC"
"AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size 00000000000000B8"
"AML:NVDIMM-FIT: Rcvd buf size 00000000000000B8"     -->All looks fine in first iteration.

(2nd iteration with offset 0xb8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 

"AML:NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 0000000000000004"   -->Requested size is 4
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008" --> Created size is 8 !.
"AML:NVDIMM-RFIT Rcvd buf size 0000000000000008"
"AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size 0000000000000004" 
"AML:NVDIMM-FIT: Rcvd buf size 0000000000000008"  -->Again wrong here.

(3rd iteration with offset 0xc0 -->0xb8 + 0x8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xc0 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x3
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 3 

"AML:NVDIMM-NCAL: Rcvd RLEN 0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000000] = 0x0000000000000008"
"AML:NVDIMM-NCAL TBUF[0000000000000001] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000002] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000003] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000004] = 0x0000000000000003"
"AML:NVDIMM-NCAL TBUF[0000000000000005] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000006] = 0x0000000000000000"
"AML:NVDIMM-NCAL TBUF[0000000000000007] = 0x0000000000000000"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 0000000000000008"
"AML:NVDIMM-RFIT: Zero Buff return, status 0000000000000003"   -->Error status, return 
"AML:NVDIMM-FIT: Rcvd buf size 0000000000000000"
"AML:NVDIMM-FIT: _FIT returned size 00000000000000C0"
[KERNEL] acpi_nfit_init: NVDIMM: data 0xffff0000fc57de18 sz 0xc0


On x86
(1st iteration with offset 0)
-Same as ARM64

(2nd iteration with offset 0xb8)
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: Read FIT: offset 0xb8 FIT size 0xb8 Dirty No.
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buff: 
[QEMU]NVDIMM BUF[0x0] = 0x8
[QEMU]NVDIMM BUF[0x1] = 0x0
[QEMU]NVDIMM BUF[0x2] = 0x0
[QEMU]NVDIMM BUF[0x3] = 0x0
[QEMU]NVDIMM BUF[0x4] = 0x0
[QEMU]NVDIMM BUF[0x5] = 0x0
[QEMU]NVDIMM BUF[0x6] = 0x0
[QEMU]NVDIMM BUF[0x7] = 0x0
[QEMU]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8 func_ret_status 0 
"AML:NVDIMM-NCAL: Rcvd RLEN 00000008"
"AML:NVDIMM-NCAL TBUF[00000000] = 0x00000008"
"AML:NVDIMM-NCAL TBUF[00000001] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000002] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000003] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000004] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000005] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000006] = 0x00000000"
"AML:NVDIMM-NCAL TBUF[00000007] = 0x00000000"
"AML:NVDIMM-NCAL: Creating OBUF with bytes 00000004"
"AML:NVDIMM-NCAL: Created  BUF(Local7) size 00000004" -->Correct size
"AML:NVDIMM-RFIT Rcvd buf size 00000004"
"AML:NVDIMM-FIT: Rcvd buf size 00000000"   --> Hence there is no data payload. 
"AML:NVDIMM-FIT: _FIT returned size 000000B8"  --> No 3rd iteration. Return correct size. 
[KERNEL] acpi_nfit_init: NVDIMM: data 0xffff8c6abd5b5518 sz 0xb8

Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64, 
2nd iteration case, the created buffer size in NCAL and RFIT methods have
additional 4 bytes!.

    CreateField (ODAT, Zero, Local1, OBUF)
    Concatenate (Buffer (Zero){}, OBUF, Local7)

Please let me know if you have any clue.

Thanks,
Shameer

.

[-- Attachment #2: SSDT-dbg2-arm64.dsl --]
[-- Type: application/octet-stream, Size: 7553 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Mon Nov 18 16:09:53 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002EF (751)
 *     Revision         0x01
 *     Checksum         0xA4
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemMemory, 0x09090000, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    TBUF,   32768
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }

                NTFI = Local6
                printf("AML:NVDIMM-NCAL: Rcvd RLEN %o", RLEN)
                Local2 = Zero
                While (RLEN > Local2)
                {
                    printf("AML:NVDIMM-NCAL TBUF[%o] = 0x%o", Local2, Derefof(TBUF[local2]))
                    Local2 += 1	
				}

                Local1 = (RLEN - 0x04)
                printf("AML:NVDIMM-NCAL: Creating OBUF with bytes %o", Local1)
                Local1 = (Local1 << 0x03)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                printf("AML:NVDIMM-NCAL: Created  BUF(Local7) size %o", SizeOf(Local7))
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = Buffer (Zero){}
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    printf("AML:NVDIMM-RFIT: Zero Buff return, status %o", RSTA)
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                printf("AML:NVDIMM-RFIT Rcvd buf size %o", Local1)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("AML:NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                           printf("AML:NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }
                      
                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xFFFF0000)
}


[-- Attachment #3: SSDT-dbg2-x86.dsl --]
[-- Type: application/octet-stream, Size: 7545 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Mon Nov 18 15:53:14 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002ED (749)
 *     Revision         0x01
 *     Checksum         0x3D
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    TBUF,   32768
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }

                NTFI = Local6
                printf("AML:NVDIMM-NCAL: Rcvd RLEN %o", RLEN)
                Local2 = Zero
                While (RLEN > Local2)
                {
                    printf("AML:NVDIMM-NCAL TBUF[%o] = 0x%o", Local2, Derefof(TBUF[local2]))
                    Local2 += 1	
				}

                Local1 = (RLEN - 0x04)
                printf("AML:NVDIMM-NCAL: Creating OBUF with bytes %o", Local1)
                Local1 = (Local1 << 0x03)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                printf("AML:NVDIMM-NCAL: Created  BUF(Local7) size %o", SizeOf(Local7))
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = Buffer (Zero){}
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    printf("AML:NVDIMM-RFIT: Zero Buff return, status %o", RSTA)
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                printf("AML:NVDIMM-RFIT Rcvd buf size %o", Local1)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                printf("AML:NVDIMM-RFIT Created NVDR.RFIT.BUFF size %o", Local1)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = Buffer (Zero){}
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    printf("AML:NVDIMM-FIT: Rcvd buf size %o", Local1)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                           printf("AML:NVDIMM-FIT: _FIT returned size %o", SizeOf(Local2))
                           Return (Local2)
                        }
                      
                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xBFBFD000)
}

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

Hi Igor/ xiaoguangrong,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 28 November 2019 12:36
> To: 'Igor Mammedov' <imammedo@redhat.com>;
> xiaoguangrong.eric@gmail.com
> Cc: peter.maydell@linaro.org; drjones@redhat.com;
> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm
> <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> 
> 
> > -----Original Message-----
> > From: Qemu-devel
> >
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 26 November 2019 08:57
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; drjones@redhat.com;
> > xiaoguangrong.eric@gmail.com; shannon.zhaosl@gmail.com;
> > qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>; Auger Eric
> > <eric.auger@redhat.com>; qemu-arm@nongnu.org; xuwei (O)
> > <xuwei5@huawei.com>; lersek@redhat.com
> > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> [..]
> 
> > > > 0xb8 Dirty No.  -->Another read is attempted
> > > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8
> > > > func_ret_status 3  --> Error status returned
> > > >
> > > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > > 1 place like this in nvdimm_dsm_func_read_fit()
> > > >     if (read_fit->offset > fit->len) {
> > > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > > >         goto exit;
> > > >     }
> > > >
> > > > so I'd start looking from here and check that QEMU gets expected data
> > > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > > content of DSM buffer (from qemu side).
> > >
> > > I had printed the DSM buffer previously and it looked same, I will double
> check
> > > that.
> 
> Tried printing the buffer in both Qemu/AML code.
> 
> On Amr64,

[...]
 
> Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> additional 4 bytes!.
> 
>     CreateField (ODAT, Zero, Local1, OBUF)
>     Concatenate (Buffer (Zero){}, OBUF, Local7)
> 
> Please let me know if you have any clue.
> 

I couldn't figure out yet, why this extra 4 bytes are added by aml code on ARM64
when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut without
any FIT data. ie, when the FIT buffer len (read_len) is zero.

But the below will fix this issue,

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index f91eea3802..cddf95f4c1 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -588,7 +588,7 @@ static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
     nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
                  read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");

-    if (read_fit->offset > fit->len) {
+    if (read_fit->offset >= fit->len) {
         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
         goto exit;
     }


This will return error code to aml in the second iteration when there is no further
FIT data to report. But, I am not sure why this check was omitted in the first place.

Please let me know if this is acceptable and then probably I can look into a v2 of this
series.

Thanks,
Shameer

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Igor Mammedov]

On Mon, 9 Dec 2019 17:39:15 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> Hi Igor/ xiaoguangrong,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 28 November 2019 12:36
> > To: 'Igor Mammedov' <imammedo@redhat.com>;
> > xiaoguangrong.eric@gmail.com
> > Cc: peter.maydell@linaro.org; drjones@redhat.com;
> > shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm
> > <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> > qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: Qemu-devel
> > >  
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn  
> > > u.org] On Behalf Of Igor Mammedov
> > > Sent: 26 November 2019 08:57
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: peter.maydell@linaro.org; drjones@redhat.com;
> > > xiaoguangrong.eric@gmail.com; shannon.zhaosl@gmail.com;
> > > qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>; Auger Eric
> > > <eric.auger@redhat.com>; qemu-arm@nongnu.org; xuwei (O)
> > > <xuwei5@huawei.com>; lersek@redhat.com
> > > Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support  
> > 
> > [..]
> >   
> > > > > 0xb8 Dirty No.  -->Another read is attempted  
> > > > > > [Qemu]NVDIMM:nvdimm_dsm_func_read_fit: read_fit_out buf size 0x8  
> > > > > func_ret_status 3  --> Error status returned
> > > > >
> > > > > status 3 means that QEMU didn't like content of NRAM, and there is only
> > > > > 1 place like this in nvdimm_dsm_func_read_fit()
> > > > >     if (read_fit->offset > fit->len) {
> > > > >         func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> > > > >         goto exit;
> > > > >     }
> > > > >
> > > > > so I'd start looking from here and check that QEMU gets expected data
> > > > > in nvdimm_dsm_write(). In other words I'd try to trace/compare
> > > > > content of DSM buffer (from qemu side).  
> > > >
> > > > I had printed the DSM buffer previously and it looked same, I will double  
> > check  
> > > > that.  
> > 
> > Tried printing the buffer in both Qemu/AML code.
> > 
> > On Amr64,  
> 
> [...]
>  
> > Attached the SSDT.dsl used for debugging. I am still not clear why on ARM64,
> > 2nd iteration case, the created buffer size in NCAL and RFIT methods have
> > additional 4 bytes!.
> > 
> >     CreateField (ODAT, Zero, Local1, OBUF)
> >     Concatenate (Buffer (Zero){}, OBUF, Local7)
> > 
> > Please let me know if you have any clue.
> >   
> 
> I couldn't figure out yet, why this extra 4 bytes are added by aml code on ARM64
> when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut without
> any FIT data. ie, when the FIT buffer len (read_len) is zero.
> 
> But the below will fix this issue,
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index f91eea3802..cddf95f4c1 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -588,7 +588,7 @@ static void nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
>      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
>                   read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> 
> -    if (read_fit->offset > fit->len) {
> +    if (read_fit->offset >= fit->len) {
>          func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
>          goto exit;
>      }
> 
> 
> This will return error code to aml in the second iteration when there is no further
> FIT data to report. But, I am not sure why this check was omitted in the first place.
> 
> Please let me know if this is acceptable and then probably I can look into a v2 of this
> series.
Sorry, I don't have capacity to debug this right now,
but I'd prefer if 'why' question was answered first.

Anyways, if something is unclear in how concrete AML code is build/works,
feel free to ask and I'll try to explain and guide you.

> Thanks,
> Shameer
> 
> 
> 

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

[-- Attachment #1: Type: text/plain, Size: 5206 bytes --]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 11 December 2019 07:57
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: xiaoguangrong.eric@gmail.com; peter.maydell@linaro.org;
> drjones@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> Linuxarm <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[...]

> > I couldn't figure out yet, why this extra 4 bytes are added by aml code on
> ARM64
> > when the nvdimm_dsm_func_read_fit() returns NvdimmFuncReadFITOut
> without
> > any FIT data. ie, when the FIT buffer len (read_len) is zero.
> >
> > But the below will fix this issue,
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index f91eea3802..cddf95f4c1 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -588,7 +588,7 @@ static void
> nvdimm_dsm_func_read_fit(NVDIMMState *state, NvdimmDsmIn *in,
> >      nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> >                   read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> >
> > -    if (read_fit->offset > fit->len) {
> > +    if (read_fit->offset >= fit->len) {
> >          func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> >          goto exit;
> >      }
> >
> >
> > This will return error code to aml in the second iteration when there is no
> further
> > FIT data to report. But, I am not sure why this check was omitted in the first
> place.
> >
> > Please let me know if this is acceptable and then probably I can look into a v2
> of this
> > series.
> Sorry, I don't have capacity to debug this right now,

No problem.

> but I'd prefer if 'why' question was answered first.

Right.

> Anyways, if something is unclear in how concrete AML code is build/works,
> feel free to ask and I'll try to explain and guide you.

Thanks for your help. I did spend some more time debugging this further.
I tried to introduce a totally new Buffer field object with different
sizes and printing the size after creation.

--- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
+++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
@@ -18,7 +18,7 @@
  *     Compiler ID      "BXPC"
  *     Compiler Version 0x00000001 (1)
  */
-DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
+DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
 {
     Scope (\_SB)
     {
@@ -48,6 +48,11 @@
                     RLEN,   32, 
                     ODAT,   32736
                 }
+                
+                Field (NRAM, DWordAcc, NoLock, Preserve)
+                {
+                    NBUF,   32768 
+                }
 
                 If ((Arg4 == Zero))
                 {
@@ -87,6 +92,12 @@
                     Local3 = DerefOf (Local2)
                     FARG = Local3
                 }
+               
+                Local2 = 0x2 
+                printf("AML:NVDIMM Creating TBUF with bytes %o", Local2)
+                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
+                Concatenate (Buffer (Zero){}, TBUF, Local3)
+                printf("AML:NVDIMM Size of TBUF(Local3) %o", SizeOf(Local3))
 
                 NTFI = Local6
                 Local1 = (RLEN - 0x04)

And run it by changing Local2 with different values, It looks on ARM64, 

For cases where, Local2 <8, the created buffer size is always 8 bytes

"AML:NVDIMM Creating TBUF with bytes 0000000000000002"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000008"

...
"AML:NVDIMM Creating TBUF with bytes 0000000000000005"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000008"

And once Local2 >=8, it gets the correct size,

"AML:NVDIMM Creating TBUF with bytes 0000000000000009"
"AML:NVDIMM Size of TBUF(Local3) 0000000000000009"


But on x86, the behavior is like, 

For cases where, Local2 <4, the created buffer size is always 4 bytes

"AML:NVDIMM Creating TBUF with bytes 00000002"
"AML:NVDIMM Size of TBUF(Local3) 00000004"
....
"AML:NVDIMM Creating TBUF with bytes 00000003"
"AML:NVDIMM Size of TBUF(Local3) 00000004"

And once Local2 >= 4, it is ok

"AML:NVDIMM Creating TBUF with bytes 00000005"
"AML:NVDIMM Size of TBUF(Local3) 00000005"
...
"AML:NVDIMM Creating TBUF with bytes 00000009"
"AML:NVDIMM Size of TBUF(Local3) 00000009"

This is the reason why it works on x86 and not on ARM64. Because, if you
remember on second iteration of the FIT buffer, the requested buffer size is 4 .

I tried changing the AccessType of the below NBUF field from DWordAcc to
ByteAcc/BufferAcc, but no luck.

+                Field (NRAM, DWordAcc, NoLock, Preserve)
+                {
+                    NBUF,   32768 
+                }

Not sure what we need to change for ARM64 to create buffer object of size 4
here. Please let me know if you have any pointers to debug this further.

(I am attaching both x86 and ARM64 SSDT dsl used for reference)

Thanks,
Shameer


> > Thanks,
> > Shameer
> >
> >
> >


[-- Attachment #2: SSDT-arm64-dbg.dsl --]
[-- Type: application/octet-stream, Size: 6942 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Thu Dec 12 15:28:21 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002EF (751)
 *     Revision         0x01
 *     Checksum         0xA4
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemMemory, 0x09090000, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }
                
                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    NBUF,   32768 
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }
               
                Local2 = 0xD 
                printf("AML:NVDIMM Creating TBUF with bytes %o", Local2)
                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
                Concatenate (Buffer (Zero){}, TBUF, Local3)
                printf("AML:NVDIMM Size of TBUF(Local3) %o", SizeOf(Local3))

                NTFI = Local6
                Local1 = (RLEN - 0x04)
                Local1 = (Local1 << 0x03)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                            Return (Local2)
                        }

                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xFFFF0000)
}


[-- Attachment #3: SSDT-x86-dbg.dsl --]
[-- Type: application/octet-stream, Size: 6934 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20180105 (64-bit version)
 * Copyright (c) 2000 - 2018 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of SSDT, Mon Nov 18 15:53:14 2019
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002ED (749)
 *     Revision         0x01
 *     Checksum         0x3D
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "NVDIMM"
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
{
    Scope (\_SB)
    {
        Device (NVDR)
        {
            Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID: Hardware ID
            Method (NCAL, 5, Serialized)
            {
                Local6 = MEMA /* \MEMA */
                OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
                OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
                Field (NPIO, DWordAcc, NoLock, Preserve)
                {
                    NTFI,   32
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    HDLE,   32, 
                    REVS,   32, 
                    FUNC,   32, 
                    FARG,   32672
                }

                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    RLEN,   32, 
                    ODAT,   32736
                }
                
                Field (NRAM, DWordAcc, NoLock, Preserve)
                {
                    NBUF,   32768 
                }

                If ((Arg4 == Zero))
                {
                    Local0 = ToUUID ("2f10e7a4-9e91-11e4-89d3-123b93f75cba")
                }
                ElseIf ((Arg4 == 0x00010000))
                {
                    Local0 = ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62")
                }
                Else
                {
                    Local0 = ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66")
                }

                If (((Local6 == Zero) | (Arg0 != Local0)))
                {
                    If ((Arg2 == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x00                                             // .
                        })
                    }

                    Return (Buffer (One)
                    {
                         0x01                                             // .
                    })
                }

                HDLE = Arg4
                REVS = Arg1
                FUNC = Arg2
                If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
                {
                    Local2 = Arg3 [Zero]
                    Local3 = DerefOf (Local2)
                    FARG = Local3
                }
               
                Local2 = 0x9 
                printf("AML:NVDIMM Creating TBUF with bytes %o", Local2)
                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
                Concatenate (Buffer (Zero){}, TBUF, Local3)
                printf("AML:NVDIMM Size of TBUF(Local3) %o", SizeOf(Local3))

                NTFI = Local6
                Local1 = (RLEN - 0x04)
                Local1 = (Local1 << 0x03)
                CreateField (ODAT, Zero, Local1, OBUF)
                Concatenate (Buffer (Zero){}, OBUF, Local7)
                Return (Local7)
            }

            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                Return (NCAL (Arg0, Arg1, Arg2, Arg3, Zero))
            }

            Name (RSTA, Zero)
            Method (RFIT, 1, Serialized)
            {
                Name (OFST, Zero)
                OFST = Arg0
                Local0 = NCAL (ToUUID ("648b9cf2-cda1-4312-8ad9-49c4af32bd62"), One, One, Package (0x01)
                        {
                            OFST
                        }, 0x00010000)
                CreateDWordField (Local0, Zero, STAU)
                RSTA = STAU /* \_SB_.NVDR.RFIT.STAU */
                If ((Zero != STAU))
                {
                    Return (Buffer (Zero){})
                }

                Local1 = SizeOf (Local0)
                Local1 -= 0x04
                If ((Local1 == Zero))
                {
                    Return (Buffer (Zero){})
                }

                CreateField (Local0, 0x20, (Local1 << 0x03), BUFF)
                Return (BUFF) /* \_SB_.NVDR.RFIT.BUFF */
            }

            Method (_FIT, 0, Serialized)  // _FIT: Firmware Interface Table
            {
                Local2 = Buffer (Zero){}
                Local3 = Zero
                While (One)
                {
                    Local0 = RFIT (Local3)
                    Local1 = SizeOf (Local0)
                    If ((RSTA == 0x0100))
                    {
                        Local2 = Buffer (Zero){}
                        Local3 = Zero
                    }
                    Else
                    {
                        If ((Local1 == Zero))
                        {
                            Return (Local2)
                        }

                        Local3 += Local1
                        Concatenate (Local2, Local0, Local2)
                    }
                }
            }

            Device (NV00)
            {
                Name (_ADR, One)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
                }
            }

            Device (NV01)
            {
                Name (_ADR, 0x02)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x02))
                }
            }

            Device (NV02)
            {
                Name (_ADR, 0x03)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x03))
                }
            }

            Device (NV03)
            {
                Name (_ADR, 0x04)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x04))
                }
            }

            Device (NV04)
            {
                Name (_ADR, 0x05)  // _ADR: Address
                Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                {
                    Return (NCAL (Arg0, Arg1, Arg2, Arg3, 0x05))
                }
            }
        }
    }

    Name (MEMA, 0xBFBFD000)
}

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 13 December 2019 12:52
> To: 'Igor Mammedov' <imammedo@redhat.com>
> Cc: xiaoguangrong.eric@gmail.com; peter.maydell@linaro.org;
> drjones@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> Linuxarm <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com
> Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> 

[...]

> 
> Thanks for your help. I did spend some more time debugging this further.
> I tried to introduce a totally new Buffer field object with different
> sizes and printing the size after creation.
> 
> --- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
> +++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
> @@ -18,7 +18,7 @@
>   *     Compiler ID      "BXPC"
>   *     Compiler Version 0x00000001 (1)
>   */
> -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
> +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
>  {
>      Scope (\_SB)
>      {
> @@ -48,6 +48,11 @@
>                      RLEN,   32,
>                      ODAT,   32736
>                  }
> +
> +                Field (NRAM, DWordAcc, NoLock, Preserve)
> +                {
> +                    NBUF,   32768
> +                }
> 
>                  If ((Arg4 == Zero))
>                  {
> @@ -87,6 +92,12 @@
>                      Local3 = DerefOf (Local2)
>                      FARG = Local3
>                  }
> +
> +                Local2 = 0x2
> +                printf("AML:NVDIMM Creating TBUF with bytes %o",
> Local2)
> +                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
> +                Concatenate (Buffer (Zero){}, TBUF, Local3)
> +                printf("AML:NVDIMM Size of TBUF(Local3) %o",
> SizeOf(Local3))
> 
>                  NTFI = Local6
>                  Local1 = (RLEN - 0x04)
> 
> And run it by changing Local2 with different values, It looks on ARM64,
> 
> For cases where, Local2 <8, the created buffer size is always 8 bytes
> 
> "AML:NVDIMM Creating TBUF with bytes 0000000000000002"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> 
> ...
> "AML:NVDIMM Creating TBUF with bytes 0000000000000005"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> 
> And once Local2 >=8, it gets the correct size,
> 
> "AML:NVDIMM Creating TBUF with bytes 0000000000000009"
> "AML:NVDIMM Size of TBUF(Local3) 0000000000000009"
> 
> 
> But on x86, the behavior is like,
> 
> For cases where, Local2 <4, the created buffer size is always 4 bytes
> 
> "AML:NVDIMM Creating TBUF with bytes 00000002"
> "AML:NVDIMM Size of TBUF(Local3) 00000004"
> ....
> "AML:NVDIMM Creating TBUF with bytes 00000003"
> "AML:NVDIMM Size of TBUF(Local3) 00000004"
> 
> And once Local2 >= 4, it is ok
> 
> "AML:NVDIMM Creating TBUF with bytes 00000005"
> "AML:NVDIMM Size of TBUF(Local3) 00000005"
> ...
> "AML:NVDIMM Creating TBUF with bytes 00000009"
> "AML:NVDIMM Size of TBUF(Local3) 00000009"
> 
> This is the reason why it works on x86 and not on ARM64. Because, if you
> remember on second iteration of the FIT buffer, the requested buffer size is 4 .
> 
> I tried changing the AccessType of the below NBUF field from DWordAcc to
> ByteAcc/BufferAcc, but no luck.
> 
> +                Field (NRAM, DWordAcc, NoLock, Preserve)
> +                {
> +                    NBUF,   32768
> +                }
> 
> Not sure what we need to change for ARM64 to create buffer object of size 4
> here. Please let me know if you have any pointers to debug this further.
> 
> (I am attaching both x86 and ARM64 SSDT dsl used for reference)

(+Jonathan)

Thanks to Jonathan for taking a fresh look at this issue and spotting this,
https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L109

And, from ACPI 6.3, table 19-419

"If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
will be treated as an Integer. Otherwise, it will be treated as a Buffer. The size
of an Integer is indicated by the Definition Block table header's Revision field.
A Revision field value less than 2 indicates that the size of an Integer is 32 bits.
A value greater than or equal to 2 signifies that the size of an Integer is 64 bits."

It looks like the main reason for the difference in behavior of the buffer object
size between x86 and ARM/virt, is because of the Revision number used in the
DSDT table. On x86 it is 1 and ARM/virt it is 2.

So most likely,

>     CreateField (ODAT, Zero, Local1, OBUF)
>     Concatenate (Buffer (Zero){}, OBUF, Local7)

defaults to the minimum integer byte length based on DSDT revision number.

I tried changing the DSDT rev number of x86 code to 2,
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 614e48ff38..2941edab8d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2257,7 +2257,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
     build_header(linker, table_data,
         (void *)(table_data->data + table_data->len - dsdt->buf->len),
-        "DSDT", dsdt->buf->len, 1, NULL, NULL);
+        "DSDT", dsdt->buf->len, 2, NULL, NULL);
     free_aml_allocator();
 }

And the same issue was reported by Guest Kernel,

[    1.636672] nfit ACPI0012:00: found a zero length table '0' parsing nfit


With a quick fix to the nvdimm aml code(below) everything seems to work
now. But again this may not be the right fix/approach for this.

Please take a look and let me know.

Thanks,
Shameer

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 64eacfad08..621f9ffd41 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
     aml_append(method, ifctx);
 
     aml_append(method, aml_store(aml_sizeof(buf), buf_size));
-    aml_append(method, aml_subtract(buf_size,
-                                    aml_int(4) /* the size of "STAU" */,
-                                    buf_size));
 
     /* if we read the end of fit. */
-    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
+                             aml_sizeof(aml_int(0)), NULL),
+                             aml_int(0)));
     aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
     aml_append(method, ifctx);
 
+    aml_append(method, aml_subtract(buf_size,
+                                    aml_int(4) /* the size of "STAU" */,
+                                    buf_size));
+
     aml_append(method, aml_create_field(buf,
                             aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
                             aml_shiftleft(buf_size, aml_int(3)), "BUFF"));

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Igor Mammedov]

On Mon, 6 Jan 2020 17:06:32 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 13 December 2019 12:52
> > To: 'Igor Mammedov' <imammedo@redhat.com>
> > Cc: xiaoguangrong.eric@gmail.com; peter.maydell@linaro.org;
> > drjones@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> > Linuxarm <linuxarm@huawei.com>; Auger Eric <eric.auger@redhat.com>;
> > qemu-arm@nongnu.org; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com
> > Subject: RE: [PATCH 0/5] ARM virt: Add NVDIMM support
> >   
> 
> [...]
> 
> > 
> > Thanks for your help. I did spend some more time debugging this further.
> > I tried to introduce a totally new Buffer field object with different
> > sizes and printing the size after creation.
> > 
> > --- SSDT.dsl	2019-12-12 15:28:21.976986949 +0000
> > +++ SSDT-arm64-dbg.dsl	2019-12-13 12:17:11.026806186 +0000
> > @@ -18,7 +18,7 @@
> >   *     Compiler ID      "BXPC"
> >   *     Compiler Version 0x00000001 (1)
> >   */
> > -DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001)
> > +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000002)
> >  {
> >      Scope (\_SB)
> >      {
> > @@ -48,6 +48,11 @@
> >                      RLEN,   32,
> >                      ODAT,   32736
> >                  }
> > +
> > +                Field (NRAM, DWordAcc, NoLock, Preserve)
> > +                {
> > +                    NBUF,   32768
> > +                }
> > 
> >                  If ((Arg4 == Zero))
> >                  {
> > @@ -87,6 +92,12 @@
> >                      Local3 = DerefOf (Local2)
> >                      FARG = Local3
> >                  }
> > +
> > +                Local2 = 0x2
> > +                printf("AML:NVDIMM Creating TBUF with bytes %o",
> > Local2)
> > +                CreateField (NBUF, Zero, (Local2 << 3), TBUF)
> > +                Concatenate (Buffer (Zero){}, TBUF, Local3)
> > +                printf("AML:NVDIMM Size of TBUF(Local3) %o",
> > SizeOf(Local3))
> > 
> >                  NTFI = Local6
> >                  Local1 = (RLEN - 0x04)
> > 
> > And run it by changing Local2 with different values, It looks on ARM64,
> > 
> > For cases where, Local2 <8, the created buffer size is always 8 bytes
> > 
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000002"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> > 
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000005"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000008"
> > 
> > And once Local2 >=8, it gets the correct size,
> > 
> > "AML:NVDIMM Creating TBUF with bytes 0000000000000009"
> > "AML:NVDIMM Size of TBUF(Local3) 0000000000000009"
> > 
> > 
> > But on x86, the behavior is like,
> > 
> > For cases where, Local2 <4, the created buffer size is always 4 bytes
> > 
> > "AML:NVDIMM Creating TBUF with bytes 00000002"
> > "AML:NVDIMM Size of TBUF(Local3) 00000004"
> > ....
> > "AML:NVDIMM Creating TBUF with bytes 00000003"
> > "AML:NVDIMM Size of TBUF(Local3) 00000004"
> > 
> > And once Local2 >= 4, it is ok
> > 
> > "AML:NVDIMM Creating TBUF with bytes 00000005"
> > "AML:NVDIMM Size of TBUF(Local3) 00000005"
> > ...
> > "AML:NVDIMM Creating TBUF with bytes 00000009"
> > "AML:NVDIMM Size of TBUF(Local3) 00000009"
> > 
> > This is the reason why it works on x86 and not on ARM64. Because, if you
> > remember on second iteration of the FIT buffer, the requested buffer size is 4 .
> > 
> > I tried changing the AccessType of the below NBUF field from DWordAcc to
> > ByteAcc/BufferAcc, but no luck.
> > 
> > +                Field (NRAM, DWordAcc, NoLock, Preserve)
> > +                {
> > +                    NBUF,   32768
> > +                }
> > 
> > Not sure what we need to change for ARM64 to create buffer object of size 4
> > here. Please let me know if you have any pointers to debug this further.
> > 
> > (I am attaching both x86 and ARM64 SSDT dsl used for reference)  
> 
> (+Jonathan)
> 
> Thanks to Jonathan for taking a fresh look at this issue and spotting this,
> https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L109
> 
> And, from ACPI 6.3, table 19-419
> 
> "If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
> will be treated as an Integer. Otherwise, it will be treated as a Buffer. The size
> of an Integer is indicated by the Definition Block table header's Revision field.
> A Revision field value less than 2 indicates that the size of an Integer is 32 bits.
> A value greater than or equal to 2 signifies that the size of an Integer is 64 bits."
> 
> It looks like the main reason for the difference in behavior of the buffer object
> size between x86 and ARM/virt, is because of the Revision number used in the
> DSDT table. On x86 it is 1 and ARM/virt it is 2.
> 
> So most likely,
> 
> >     CreateField (ODAT, Zero, Local1, OBUF)

You are right, that's where it goes wrong, since OBUF
is implicitly converted to integer if size is less than 64bits.

> >     Concatenate (Buffer (Zero){}, OBUF, Local7)

see more below

[...]

> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 64eacfad08..621f9ffd41 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
>      aml_append(method, ifctx);
>  
>      aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> -    aml_append(method, aml_subtract(buf_size,
> -                                    aml_int(4) /* the size of "STAU" */,
> -                                    buf_size));
>  
>      /* if we read the end of fit. */
> -    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
> +                             aml_sizeof(aml_int(0)), NULL),
> +                             aml_int(0)));
>      aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
>      aml_append(method, ifctx);
>  
> +    aml_append(method, aml_subtract(buf_size,
> +                                    aml_int(4) /* the size of "STAU" */,
> +                                    buf_size));
> +
>      aml_append(method, aml_create_field(buf,
>                              aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
>                              aml_shiftleft(buf_size, aml_int(3)), "BUFF"));

Instead of covering up error in NCAL, I'd prefer original issue fixed.

How about something like this pseudocode:

                NTFI = Local6                                                    
                Local1 = (RLEN - 0x04)                                           
-                Local1 = (Local1 << 0x03)                                        
-                CreateField (ODAT, Zero, Local1, OBUF)                           
-                Concatenate (Buffer (Zero) {}, OBUF, Local7)   

                If (Local1 < IntegerSize)
                {
                    Local7 = Buffer(0) // output buffer
                    Local8 = 0 // index for being copied byte
                    // build byte by byte output buffer
                    while (Local8 < Local1) {
                       Local9 = Buffer(1)
                       // copy 1 byte at Local8 offset from ODAT to temporary buffer Local9
                       Store(DeRef(Index(ODAT, Local8)), Index(Local9, 0))
                       Concatenate (Local7, Local9, Local7) 
                       Increment(Local8)
                    }
                    return Local7
                } else {
                    CreateField (ODAT, Zero, Local1, OBUF)
                    return OBUF
                }

RE: [PATCH 0/5] ARM virt: Add NVDIMM support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 09 January 2020 17:13
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; drjones@redhat.com;
> xiaoguangrong.eric@gmail.com; Auger Eric <eric.auger@redhat.com>;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>;
> shannon.zhaosl@gmail.com; qemu-arm@nongnu.org; xuwei (O)
> <xuwei5@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; lersek@redhat.com
> Subject: Re: [PATCH 0/5] ARM virt: Add NVDIMM support
> 
> On Mon, 6 Jan 2020 17:06:32 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Hi Igor,

[...]

> > (+Jonathan)
> >
> > Thanks to Jonathan for taking a fresh look at this issue and spotting this,
> >
> https://elixir.bootlin.com/linux/v5.5-rc5/source/drivers/acpi/acpica/utmisc.c#L
> 109
> >
> > And, from ACPI 6.3, table 19-419
> >
> > "If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it
> > will be treated as an Integer. Otherwise, it will be treated as a Buffer. The
> size
> > of an Integer is indicated by the Definition Block table header's Revision field.
> > A Revision field value less than 2 indicates that the size of an Integer is 32
> bits.
> > A value greater than or equal to 2 signifies that the size of an Integer is 64
> bits."
> >
> > It looks like the main reason for the difference in behavior of the buffer object
> > size between x86 and ARM/virt, is because of the Revision number used in
> the
> > DSDT table. On x86 it is 1 and ARM/virt it is 2.
> >
> > So most likely,
> >
> > >     CreateField (ODAT, Zero, Local1, OBUF)
> 
> You are right, that's where it goes wrong, since OBUF
> is implicitly converted to integer if size is less than 64bits.
> 
> > >     Concatenate (Buffer (Zero){}, OBUF, Local7)
> 
> see more below
> 
> [...]
> 
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 64eacfad08..621f9ffd41 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -1192,15 +1192,18 @@ static void nvdimm_build_fit(Aml *dev)
> >      aml_append(method, ifctx);
> >
> >      aml_append(method, aml_store(aml_sizeof(buf), buf_size));
> > -    aml_append(method, aml_subtract(buf_size,
> > -                                    aml_int(4) /* the size of
> "STAU" */,
> > -                                    buf_size));
> >
> >      /* if we read the end of fit. */
> > -    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> > +    ifctx = aml_if(aml_equal(aml_subtract(buf_size,
> > +                             aml_sizeof(aml_int(0)), NULL),
> > +                             aml_int(0)));
> >      aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> >      aml_append(method, ifctx);
> >
> > +    aml_append(method, aml_subtract(buf_size,
> > +                                    aml_int(4) /* the size of
> "STAU" */,
> > +                                    buf_size));
> > +
> >      aml_append(method, aml_create_field(buf,
> >                              aml_int(4 * BITS_PER_BYTE), /* offset
> at byte 4.*/
> >                              aml_shiftleft(buf_size, aml_int(3)),
> "BUFF"));
> 
> Instead of covering up error in NCAL, I'd prefer original issue fixed.
> How about something like this pseudocode:
> 
>                 NTFI = Local6
>                 Local1 = (RLEN - 0x04)
> -                Local1 = (Local1 << 0x03)
> -                CreateField (ODAT, Zero, Local1, OBUF)
> -                Concatenate (Buffer (Zero) {}, OBUF, Local7)
> 
>                 If (Local1 < IntegerSize)
>                 {
>                     Local7 = Buffer(0) // output buffer
>                     Local8 = 0 // index for being copied byte
>                     // build byte by byte output buffer
>                     while (Local8 < Local1) {
>                        Local9 = Buffer(1)
>                        // copy 1 byte at Local8 offset from ODAT to
> temporary buffer Local9
>                        Store(DeRef(Index(ODAT, Local8)), Index(Local9,
> 0))
>                        Concatenate (Local7, Local9, Local7)
>                        Increment(Local8)
>                     }
>                     return Local7
>                 } else {
>                     CreateField (ODAT, Zero, Local1, OBUF)
>                     return OBUF
>                 }
> 

Ok. This looks much better. I will test this and sent out a v2 soon addressing other
comments on this series as well.

Thanks,
Shameer

Re: [PATCH 0/5] ARM virt: Add NVDIMM support

[Igor Mammedov]

On Fri, 4 Oct 2019 16:52:57 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This series adds NVDIMM support to arm/virt platform.
> This has a dependency on [0] and make use of the GED
> device for NVDIMM hotplug events. The series reuses
> some of the patches posted by Eric in his earlier
> attempt here[1].
> 
> Patch 1/5 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2].
> 
> I have done basic sanity testing of NVDIMM deviecs with
> both ACPI and DT Guest boot. Further testing is always
> welcome.
> 
> Please let me know your feedback.
one more thing,

It's possible to run bios-tables-test for virt/arm now,
pls add corresponding test case
it might be easier to just extend test_acpi_virt_tcg_memhp() with nvdimms

> 
> Thanks,
> Shameer
> 
> [0] https://patchwork.kernel.org/cover/11150345/
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
> 
> Eric Auger (1):
>   hw/arm/boot: Expose the pmem nodes in the DT
> 
> Kwangwoo Lee (2):
>   nvdimm: Use configurable ACPI IO base and size
>   hw/arm/virt: Add nvdimm hot-plug infrastructure
> 
> Shameer Kolothum (2):
>   hw/arm: Align ACPI blob len to PAGE size
>   hw/arm/virt: Add nvdimm hotplug support
> 
>  docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
>  hw/acpi/generic_event_device.c         | 13 ++++++++
>  hw/acpi/nvdimm.c                       | 32 ++++++++++++------
>  hw/arm/Kconfig                         |  1 +
>  hw/arm/boot.c                          | 45 ++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c               | 20 ++++++++++++
>  hw/arm/virt.c                          | 42 ++++++++++++++++++++----
>  hw/i386/acpi-build.c                   |  6 ++++
>  hw/i386/acpi-build.h                   |  3 ++
>  hw/i386/pc_piix.c                      |  2 ++
>  hw/i386/pc_q35.c                       |  2 ++
>  hw/mem/Kconfig                         |  2 +-
>  include/hw/acpi/generic_event_device.h |  1 +
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/mem/nvdimm.h                |  3 ++
>  15 files changed, 157 insertions(+), 17 deletions(-)
>