[QEMU,v2,4/6] nvdimm: Implement ACPI NVDIMM Label Methods

Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W}, which
depricates corresponding _DSM Functions defined by PMEM _DSM Interface spec
[2].

In this implementation, we do 2 things
1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with ACPI
method dispatch, _DSM is one of the branches. This also paves the way for
adding other ACPI methods for NVDIMM.
2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
ASL form of SSDT changes can be found in next test/qtest/bios-table-test
commit message.

[1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
[2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
---
 hw/acpi/nvdimm.c        | 424 +++++++++++++++++++++++++++++++---------
 include/hw/mem/nvdimm.h |   6 +
 2 files changed, 338 insertions(+), 92 deletions(-)

On Mon, 30 May 2022 11:40:45 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W}, which
> depricates corresponding _DSM Functions defined by PMEM _DSM Interface spec
> [2].
> 
> In this implementation, we do 2 things
> 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with ACPI
> method dispatch, _DSM is one of the branches. This also paves the way for
> adding other ACPI methods for NVDIMM.
> 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> ASL form of SSDT changes can be found in next test/qtest/bios-table-test
> commit message.
> 
> [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
> ---
>  hw/acpi/nvdimm.c        | 424 +++++++++++++++++++++++++++++++---------

This patch is too large and doing to many things to be reviewable.
It needs to be split into smaller distinct chunks.
(however hold your horses and read on)

The patch it is too intrusive and my hunch is that it breaks
ABI and needs a bunch of compat knobs to work properly and
that I'd like to avoid unless there is not other way around
the problem.

I was skeptical about this approach during v1 review and
now I'm pretty much sure it's over-engineered and we can
just repack data we receive from existing label _DSM functions
to provide _LS{I,R,W} like it was suggested in v1.
It will be much simpler and affect only AML side without
complicating ABI and without any compat cruft and will work
with ping-pong migration without any issues.


>  include/hw/mem/nvdimm.h |   6 +
>  2 files changed, 338 insertions(+), 92 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59b42afcf1..50ee85866b 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState *state, GArray *table_offsets,
>  
>  #define NVDIMM_DSM_MEMORY_SIZE      4096
>  
> -struct NvdimmDsmIn {
> +struct NvdimmMthdIn {
>      uint32_t handle;
> +    uint32_t method;
> +    uint8_t  args[4088];
> +} QEMU_PACKED;
> +typedef struct NvdimmMthdIn NvdimmMthdIn;
> +struct NvdimmDsmIn {
>      uint32_t revision;
>      uint32_t function;
>      /* the remaining size in the page is used by arg3. */
>      union {
> -        uint8_t arg3[4084];
> +        uint8_t arg3[4080];
>      };
>  } QEMU_PACKED;
>  typedef struct NvdimmDsmIn NvdimmDsmIn;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmDsmOut {
>      /* the size of buffer filled by QEMU. */
> @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> -                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> +                  offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
> +                  NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncGetLabelDataOut {
>      /* the size of buffer filled by QEMU. */
> @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> -                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> +                  offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
> +                  NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncReadFITIn {
>      uint32_t offset; /* the offset into FIT buffer. */
>  } QEMU_PACKED;
>  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> -                  offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> +                  offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
> +                  NVDIMM_DSM_MEMORY_SIZE);
>  
>  struct NvdimmFuncReadFITOut {
>      /* the size of buffer filled by QEMU. */
> @@ -636,7 +644,8 @@ static uint32_t nvdimm_get_max_xfer_label_size(void)
>       * the max data ACPI can write one time which is transferred by
>       * 'Set Namespace Label Data' function.
>       */
> -    max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3) -
> +    max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn, args) -
> +                   offsetof(NvdimmDsmIn, arg3) -
>                     sizeof(NvdimmFuncSetLabelDataIn);
>  
>      return MIN(max_get_size, max_set_size);
> @@ -697,16 +706,15 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>  /*
>   * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
>   */
> -static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> -                                      hwaddr dsm_mem_addr)
> +static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> +                                    NvdimmFuncGetLabelDataIn *get_label_data,
> +                                    hwaddr dsm_mem_addr)
>  {
>      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> -    NvdimmFuncGetLabelDataIn *get_label_data;
>      NvdimmFuncGetLabelDataOut *get_label_data_out;
>      uint32_t status;
>      int size;
>  
> -    get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
>      get_label_data->offset = le32_to_cpu(get_label_data->offset);
>      get_label_data->length = le32_to_cpu(get_label_data->length);
>  
> @@ -737,15 +745,13 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  /*
>   * DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index 6).
>   */
> -static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> +static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> +                                      NvdimmFuncSetLabelDataIn *set_label_data,
>                                        hwaddr dsm_mem_addr)
>  {
>      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> -    NvdimmFuncSetLabelDataIn *set_label_data;
>      uint32_t status;
>  
> -    set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
> -
>      set_label_data->offset = le32_to_cpu(set_label_data->offset);
>      set_label_data->length = le32_to_cpu(set_label_data->length);
>  
> @@ -760,19 +766,21 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      }
>  
>      assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data) +
> -                    set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE);
> +           set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE -
> +           offsetof(NvdimmMthdIn, args));
>  
>      nvc->write_label_data(nvdimm, set_label_data->in_buf,
>                            set_label_data->length, set_label_data->offset);
>      nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
>  }
>  
> -static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> +static void nvdimm_dsm_device(uint32_t nv_handle, NvdimmDsmIn *dsm_in,
> +                                    hwaddr dsm_mem_addr)
>  {
> -    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in->handle);
> +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
>  
>      /* See the comments in nvdimm_dsm_root(). */
> -    if (!in->function) {
> +    if (!dsm_in->function) {
>          uint32_t supported_func = 0;
>  
>          if (nvdimm && nvdimm->label_size) {
> @@ -794,7 +802,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>      }
>  
>      /* Encode DSM function according to DSM Spec Rev1. */
> -    switch (in->function) {
> +    switch (dsm_in->function) {
>      case 4 /* Get Namespace Label Size */:
>          if (nvdimm->label_size) {
>              nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> @@ -803,13 +811,17 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          break;
>      case 5 /* Get Namespace Label Data */:
>          if (nvdimm->label_size) {
> -            nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
> +            nvdimm_dsm_get_label_data(nvdimm,
> +                                      (NvdimmFuncGetLabelDataIn *)dsm_in->arg3,
> +                                      dsm_mem_addr);
>              return;
>          }
>          break;
>      case 0x6 /* Set Namespace Label Data */:
>          if (nvdimm->label_size) {
> -            nvdimm_dsm_set_label_data(nvdimm, in, dsm_mem_addr);
> +            nvdimm_dsm_set_label_data(nvdimm,
> +                        (NvdimmFuncSetLabelDataIn *)dsm_in->arg3,
> +                        dsm_mem_addr);
>              return;
>          }
>          break;
> @@ -819,67 +831,128 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  }
>  
>  static uint64_t
> -nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> +nvdimm_method_read(void *opaque, hwaddr addr, unsigned size)
>  {
> -    nvdimm_debug("BUG: we never read _DSM IO Port.\n");
> +    nvdimm_debug("BUG: we never read NVDIMM Method IO Port.\n");
>      return 0;
>  }
>  
>  static void
> -nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +nvdimm_dsm_handle(void *opaque, NvdimmMthdIn *method_in, hwaddr dsm_mem_addr)
>  {
>      NVDIMMState *state = opaque;
> -    NvdimmDsmIn *in;
> -    hwaddr dsm_mem_addr = val;
> +    NvdimmDsmIn *dsm_in = (NvdimmDsmIn *)method_in->args;
>  
>      nvdimm_debug("dsm memory address 0x%" HWADDR_PRIx ".\n", dsm_mem_addr);
>  
> -    /*
> -     * The DSM memory is mapped to guest address space so an evil guest
> -     * can change its content while we are doing DSM emulation. Avoid
> -     * this by copying DSM memory to QEMU local memory.
> -     */
> -    in = g_new(NvdimmDsmIn, 1);
> -    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> -
> -    in->revision = le32_to_cpu(in->revision);
> -    in->function = le32_to_cpu(in->function);
> -    in->handle = le32_to_cpu(in->handle);
> -
> -    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n", in->revision,
> -                 in->handle, in->function);
> +    dsm_in->revision = le32_to_cpu(dsm_in->revision);
> +    dsm_in->function = le32_to_cpu(dsm_in->function);
>  
> +    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> +                 dsm_in->revision, method_in->handle, dsm_in->function);
>      /*
>       * Current NVDIMM _DSM Spec supports Rev1 and Rev2
>       * Intel® OptanePersistent Memory Module DSM Interface, Revision 2.0
>       */
> -    if (in->revision != 0x1 && in->revision != 0x2) {
> +    if (dsm_in->revision != 0x1 && dsm_in->revision != 0x2) {
>          nvdimm_debug("Revision 0x%x is not supported, expect 0x1 or 0x2.\n",
> -                     in->revision);
> +                     dsm_in->revision);
>          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
> -        goto exit;
> +        return;
>      }
>  
> -    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> -        nvdimm_dsm_handle_reserved_root_method(state, in, dsm_mem_addr);
> -        goto exit;
> +    if (method_in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> +        nvdimm_dsm_handle_reserved_root_method(state, dsm_in, dsm_mem_addr);
> +        return;
>      }
>  
>       /* Handle 0 is reserved for NVDIMM Root Device. */
> -    if (!in->handle) {
> -        nvdimm_dsm_root(in, dsm_mem_addr);
> -        goto exit;
> +    if (!method_in->handle) {
> +        nvdimm_dsm_root(dsm_in, dsm_mem_addr);
> +        return;
>      }
>  
> -    nvdimm_dsm_device(in, dsm_mem_addr);
> +    nvdimm_dsm_device(method_in->handle, dsm_in, dsm_mem_addr);
> +}
>  
> -exit:
> -    g_free(in);
> +static void nvdimm_lsi_handle(uint32_t nv_handle, hwaddr dsm_mem_addr)
> +{
> +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> +
> +    if (nvdimm->label_size) {
> +        nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> +    }
> +
> +    return;
> +}
> +
> +static void nvdimm_lsr_handle(uint32_t nv_handle,
> +                                    void *data,
> +                                    hwaddr dsm_mem_addr)
> +{
> +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> +    NvdimmFuncGetLabelDataIn *get_label_data = data;
> +
> +    if (nvdimm->label_size) {
> +        nvdimm_dsm_get_label_data(nvdimm, get_label_data, dsm_mem_addr);
> +    }
> +    return;
> +}
> +
> +static void nvdimm_lsw_handle(uint32_t nv_handle,
> +                                    void *data,
> +                                    hwaddr dsm_mem_addr)
> +{
> +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> +    NvdimmFuncSetLabelDataIn *set_label_data = data;
> +
> +    if (nvdimm->label_size) {
> +        nvdimm_dsm_set_label_data(nvdimm, set_label_data, dsm_mem_addr);
> +    }
> +    return;
> +}
> +
> +static void
> +nvdimm_method_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    NvdimmMthdIn *method_in;
> +    hwaddr dsm_mem_addr = val;
> +
> +    /*
> +     * The DSM memory is mapped to guest address space so an evil guest
> +     * can change its content while we are doing DSM emulation. Avoid
> +     * this by copying DSM memory to QEMU local memory.
> +     */
> +    method_in = g_new(NvdimmMthdIn, 1);
> +    cpu_physical_memory_read(dsm_mem_addr, method_in, sizeof(*method_in));
> +
> +    method_in->handle = le32_to_cpu(method_in->handle);
> +    method_in->method = le32_to_cpu(method_in->method);
> +
> +    switch (method_in->method) {
> +    case NVDIMM_METHOD_DSM:
> +        nvdimm_dsm_handle(opaque, method_in, dsm_mem_addr);
> +        break;
> +    case NVDIMM_METHOD_LSI:
> +        nvdimm_lsi_handle(method_in->handle, dsm_mem_addr);
> +        break;
> +    case NVDIMM_METHOD_LSR:
> +        nvdimm_lsr_handle(method_in->handle, method_in->args, dsm_mem_addr);
> +        break;
> +    case NVDIMM_METHOD_LSW:
> +        nvdimm_lsw_handle(method_in->handle, method_in->args, dsm_mem_addr);
> +        break;
> +    default:
> +        nvdimm_debug("%s: Unkown method 0x%x\n", __func__, method_in->method);
> +        break;
> +    }
> +
> +    g_free(method_in);
>  }
>  
> -static const MemoryRegionOps nvdimm_dsm_ops = {
> -    .read = nvdimm_dsm_read,
> -    .write = nvdimm_dsm_write,
> +static const MemoryRegionOps nvdimm_method_ops = {
> +    .read = nvdimm_method_read,
> +    .write = nvdimm_method_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid = {
>          .min_access_size = 4,
> @@ -899,12 +972,12 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
>                              FWCfgState *fw_cfg, Object *owner)
>  {
>      state->dsm_io = dsm_io;
> -    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
> +    memory_region_init_io(&state->io_mr, owner, &nvdimm_method_ops, state,
>                            "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));
> +    acpi_data_push(state->dsm_mem, sizeof(NvdimmMthdIn));
>      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem->data,
>                      state->dsm_mem->len);
>  
> @@ -918,13 +991,22 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
>  #define NVDIMM_DSM_IOPORT       "NPIO"
>  
>  #define NVDIMM_DSM_NOTIFY       "NTFI"
> +#define NVDIMM_DSM_METHOD       "MTHD"
>  #define NVDIMM_DSM_HANDLE       "HDLE"
>  #define NVDIMM_DSM_REVISION     "REVS"
>  #define NVDIMM_DSM_FUNCTION     "FUNC"
>  #define NVDIMM_DSM_ARG3         "FARG"
>  
> -#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> -#define NVDIMM_DSM_OUT_BUF      "ODAT"
> +#define NVDIMM_DSM_OFFSET       "OFST"
> +#define NVDIMM_DSM_TRANS_LEN    "TRSL"
> +#define NVDIMM_DSM_IN_BUFF      "IDAT"
> +
> +#define NVDIMM_DSM_OUT_BUF_SIZE     "RLEN"
> +#define NVDIMM_DSM_OUT_BUF          "ODAT"
> +#define NVDIMM_DSM_OUT_STATUS       "STUS"
> +#define NVDIMM_DSM_OUT_LSA_SIZE     "SIZE"
> +#define NVDIMM_DSM_OUT_MAX_TRANS    "MAXT"
> +
>  
>  #define NVDIMM_DSM_RFIT_STATUS  "RSTA"
>  
> @@ -938,7 +1020,6 @@ static void nvdimm_build_common_dsm(Aml *dev,
>      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>      Aml *whilectx, *offset;
>      uint8_t byte_list[1];
> -    AmlRegionSpace rs;
>  
>      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
>      uuid = aml_arg(0);
> @@ -949,37 +1030,15 @@ 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, 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)));
> -
> -    /*
> -     * DSM notifier:
> -     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and notify QEMU to
> -     *                    emulate the access.
> -     *
> -     * It is the IO port so that accessing them will cause VM-exit, the
> -     * control will be transferred to QEMU.
> -     */
> -    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
> -                      AML_PRESERVE);
> -    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> -               nvdimm_state->dsm_io.bit_width));
> -    aml_append(method, field);
> +               AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmMthdIn)));
>  
>      /*
>       * DSM input:
>       * NVDIMM_DSM_HANDLE: store device's handle, it's zero if the _DSM call
>       *                    happens on NVDIMM Root Device.
> +     * NVDIMM_DSM_METHOD: ACPI method indicator, to distinguish _DSM and
> +     *                    other ACPI methods.
>       * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
>       * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
>       * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a Package
> @@ -991,13 +1050,16 @@ static void nvdimm_build_common_dsm(Aml *dev,
>      field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
>                        AML_PRESERVE);
>      aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> -               sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
> +               sizeof(typeof_field(NvdimmMthdIn, handle)) * BITS_PER_BYTE));
> +    aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> +               sizeof(typeof_field(NvdimmMthdIn, method)) * BITS_PER_BYTE));
>      aml_append(field, aml_named_field(NVDIMM_DSM_REVISION,
>                 sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
>      aml_append(field, aml_named_field(NVDIMM_DSM_FUNCTION,
>                 sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
>      aml_append(field, aml_named_field(NVDIMM_DSM_ARG3,
> -         (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
> +         (sizeof(NvdimmMthdIn) - offsetof(NvdimmMthdIn, args) -
> +          offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
>      aml_append(method, field);
>  
>      /*
> @@ -1065,6 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
>       * it reserves 0 for root device and is the handle for NVDIMM devices.
>       * See the comments in nvdimm_slot_to_handle().
>       */
> +    aml_append(method, aml_store(aml_int(0), aml_name(NVDIMM_DSM_METHOD)));
>      aml_append(method, aml_store(handle, aml_name(NVDIMM_DSM_HANDLE)));
>      aml_append(method, aml_store(aml_arg(1), aml_name(NVDIMM_DSM_REVISION)));
>      aml_append(method, aml_store(function, aml_name(NVDIMM_DSM_FUNCTION)));
> @@ -1250,6 +1313,7 @@ static void nvdimm_build_fit(Aml *dev)
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> +    Aml *method, *pkg, *field;
>  
>      for (slot = 0; slot < ram_slots; slot++) {
>          uint32_t handle = nvdimm_slot_to_handle(slot);
> @@ -1266,6 +1330,155 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>           * table NFIT or _FIT.
>           */
>          aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle)));
> +        aml_append(nvdimm_dev, aml_operation_region(NVDIMM_DSM_MEMORY,
> +                   AML_SYSTEM_MEMORY, aml_name(NVDIMM_ACPI_MEM_ADDR),
> +                   sizeof(NvdimmMthdIn)));
> +
> +        /* ACPI 6.4: 6.5.10 NVDIMM Label Methods, _LS{I,R,W} */
> +
> +        /* Begin of _LSI Block */
> +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> +        /* _LSI Input field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> +                   sizeof(typeof_field(NvdimmMthdIn, handle)) * BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> +                   sizeof(typeof_field(NvdimmMthdIn, method)) * BITS_PER_BYTE));
> +        aml_append(method, field);
> +
> +        /* _LSI Output field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut, len)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> +                   func_ret_status)) * BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_LSA_SIZE,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut, label_size)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_MAX_TRANS,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut, max_xfer)) *
> +                   BITS_PER_BYTE));
> +        aml_append(method, field);
> +
> +        aml_append(method, aml_store(aml_int(handle),
> +                                      aml_name(NVDIMM_DSM_HANDLE)));
> +        aml_append(method, aml_store(aml_int(0x100),
> +                                      aml_name(NVDIMM_DSM_METHOD)));
> +        aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> +                                      aml_name(NVDIMM_DSM_NOTIFY)));
> +
> +        pkg = aml_package(3);
> +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_LSA_SIZE));
> +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_MAX_TRANS));
> +
> +        aml_append(method, aml_name_decl("RPKG", pkg));
> +
> +        aml_append(method, aml_return(aml_name("RPKG")));
> +        aml_append(nvdimm_dev, method); /* End of _LSI Block */
> +
> +
> +        /* Begin of _LSR Block */
> +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> +
> +        /* _LSR Input field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn, offset)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn, length)) *
> +                   BITS_PER_BYTE));
> +        aml_append(method, field);
> +
> +        /* _LSR Output field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut, len)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut,
> +                   func_ret_status)) * BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF,
> +                   (NVDIMM_DSM_MEMORY_SIZE -
> +                    offsetof(NvdimmFuncGetLabelDataOut, out_buf)) *
> +                    BITS_PER_BYTE));
> +        aml_append(method, field);
> +
> +        aml_append(method, aml_store(aml_int(handle),
> +                                      aml_name(NVDIMM_DSM_HANDLE)));
> +        aml_append(method, aml_store(aml_int(0x101),
> +                                      aml_name(NVDIMM_DSM_METHOD)));
> +        aml_append(method, aml_store(aml_arg(0), aml_name(NVDIMM_DSM_OFFSET)));
> +        aml_append(method, aml_store(aml_arg(1),
> +                                      aml_name(NVDIMM_DSM_TRANS_LEN)));
> +        aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> +                                      aml_name(NVDIMM_DSM_NOTIFY)));
> +
> +        aml_append(method, aml_store(aml_shiftleft(aml_arg(1), aml_int(3)),
> +                                         aml_local(1)));
> +        aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> +                   aml_int(0), aml_local(1), "OBUF"));
> +
> +        pkg = aml_package(2);
> +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> +        aml_append(pkg, aml_name("OBUF"));
> +        aml_append(method, aml_name_decl("RPKG", pkg));
> +
> +        aml_append(method, aml_return(aml_name("RPKG")));
> +        aml_append(nvdimm_dev, method); /* End of _LSR Block */
> +
> +        /* Begin of _LSW Block */
> +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> +        /* _LSW Input field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> +                   sizeof(typeof_field(NvdimmMthdIn, handle)) * BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> +                   sizeof(typeof_field(NvdimmMthdIn, method)) * BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn, offset)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn, length)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_IN_BUFF, 32640));
> +        aml_append(method, field);
> +
> +        /* _LSW Output field */
> +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
> +                          AML_PRESERVE);
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut, len)) *
> +                   BITS_PER_BYTE));
> +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut,
> +                   func_ret_status)) * BITS_PER_BYTE));
> +        aml_append(method, field);
> +
> +        aml_append(method, aml_store(aml_int(handle), aml_name(NVDIMM_DSM_HANDLE)));
> +        aml_append(method, aml_store(aml_int(0x102), aml_name(NVDIMM_DSM_METHOD)));
> +        aml_append(method, aml_store(aml_arg(0), aml_name(NVDIMM_DSM_OFFSET)));
> +        aml_append(method, aml_store(aml_arg(1), aml_name(NVDIMM_DSM_TRANS_LEN)));
> +        aml_append(method, aml_store(aml_arg(2), aml_name(NVDIMM_DSM_IN_BUFF)));
> +        aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> +                                      aml_name(NVDIMM_DSM_NOTIFY)));
> +
> +        aml_append(method, aml_return(aml_name(NVDIMM_DSM_OUT_STATUS)));
> +        aml_append(nvdimm_dev, method); /* End of _LSW Block */
>  
>          nvdimm_build_device_dsm(nvdimm_dev, handle);
>          aml_append(root_dev, nvdimm_dev);
> @@ -1278,7 +1491,8 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>                                uint32_t ram_slots, const char *oem_id)
>  {
>      int mem_addr_offset;
> -    Aml *ssdt, *sb_scope, *dev;
> +    Aml *ssdt, *sb_scope, *dev, *field;
> +    AmlRegionSpace rs;
>      AcpiTable table = { .sig = "SSDT", .rev = 1,
>                          .oem_id = oem_id, .oem_table_id = "NVDIMM" };
>  
> @@ -1286,6 +1500,9 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  
>      acpi_table_begin(&table, table_data);
>      ssdt = init_aml_allocator();
> +
> +    mem_addr_offset = build_append_named_dword(table_data,
> +                                               NVDIMM_ACPI_MEM_ADDR);
>      sb_scope = aml_scope("\\_SB");
>  
>      dev = aml_device("NVDR");
> @@ -1303,6 +1520,31 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>       */
>      aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>  
> +    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(dev, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> +               aml_int(nvdimm_state->dsm_io.address),
> +               nvdimm_state->dsm_io.bit_width >> 3));
> +
> +    /*
> +     * DSM notifier:
> +     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and notify QEMU to
> +     *                    emulate the access.
> +     *
> +     * It is the IO port so that accessing them will cause VM-exit, the
> +     * control will be transferred to QEMU.
> +     */
> +    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
> +                      AML_PRESERVE);
> +    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> +               nvdimm_state->dsm_io.bit_width));
> +    aml_append(dev, field);
> +
>      nvdimm_build_common_dsm(dev, nvdimm_state);
>  
>      /* 0 is reserved for root device. */
> @@ -1316,12 +1558,10 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  
>      /* copy AML table into ACPI tables blob and patch header there */
>      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> -    mem_addr_offset = build_append_named_dword(table_data,
> -                                               NVDIMM_ACPI_MEM_ADDR);
>  
>      bios_linker_loader_alloc(linker,
>                               NVDIMM_DSM_MEM_FILE, nvdimm_state->dsm_mem,
> -                             sizeof(NvdimmDsmIn), false /* high memory */);
> +                             sizeof(NvdimmMthdIn), false /* high memory */);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
>          NVDIMM_DSM_MEM_FILE, 0);
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index cf8f59be44..0206b6125b 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -37,6 +37,12 @@
>          }                                                     \
>      } while (0)
>  
> +/* NVDIMM ACPI Methods */
> +#define NVDIMM_METHOD_DSM   0
> +#define NVDIMM_METHOD_LSI   0x100
> +#define NVDIMM_METHOD_LSR   0x101
> +#define NVDIMM_METHOD_LSW   0x102
> +
>  /*
>   * The minimum label data size is required by NVDIMM Namespace
>   * specification, see the chapter 2 Namespaces:

On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> On Mon, 30 May 2022 11:40:45 +0800
> Robert Hoo <robert.hu@linux.intel.com> wrote:
> 
> > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > which
> > depricates corresponding _DSM Functions defined by PMEM _DSM
> > Interface spec
> > [2].
> > 
> > In this implementation, we do 2 things
> > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > ACPI
> > method dispatch, _DSM is one of the branches. This also paves the
> > way for
> > adding other ACPI methods for NVDIMM.
> > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > ASL form of SSDT changes can be found in next test/qtest/bios-
> > table-test
> > commit message.
> > 
> > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
> > ---
> >  hw/acpi/nvdimm.c        | 424 +++++++++++++++++++++++++++++++-----
> > ----
> 
> This patch is too large and doing to many things to be reviewable.
> It needs to be split into smaller distinct chunks.
> (however hold your horses and read on)
> 
> The patch it is too intrusive and my hunch is that it breaks
> ABI and needs a bunch of compat knobs to work properly and
> that I'd like to avoid unless there is not other way around
> the problem.

Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
and the compat knobs refers to related functions' input/output params?

My thoughts is that eventually, sooner or later, more ACPI methods will
be implemented per request, although now we can play the trick of
wrapper new methods over the pipe of old _DSM implementation.
Though this changes a little on existing struct NvdimmDsmIn {}, it
paves the way for the future; and actually the change is more an
extension or generalization, not fundamentally changes the framework.

In short, my point is the change/generalization/extension will be
inevitable, even if not present.
> 
> I was skeptical about this approach during v1 review and
> now I'm pretty much sure it's over-engineered and we can
> just repack data we receive from existing label _DSM functions
> to provide _LS{I,R,W} like it was suggested in v1.
> It will be much simpler and affect only AML side without
> complicating ABI and without any compat cruft and will work
> with ping-pong migration without any issues.

Ostensibly it may looks simpler, actually not, I think. The AML "common
pipe" NCAL() is already complex, it packs all _DSMs and NFIT() function
logics there, packing new stuff in/through it will be bug-prone.
Though this time we can avert touching it, as the new ACPI methods
deprecating old _DSM functionally is almost the same.
How about next time? are we going to always packing new methods logic
in NCAL()?
My point is that we should implement new methods as itself, of course,
as a general programming rule, we can/should abstract common routines,
but not packing them in one large function.
> 
> 
> >  include/hw/mem/nvdimm.h |   6 +
> >  2 files changed, 338 insertions(+), 92 deletions(-)
> > 
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 59b42afcf1..50ee85866b 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > *state, GArray *table_offsets,
> >  
> >  #define NVDIMM_DSM_MEMORY_SIZE      4096
> >  
> > -struct NvdimmDsmIn {
> > +struct NvdimmMthdIn {
> >      uint32_t handle;
> > +    uint32_t method;
> > +    uint8_t  args[4088];
> > +} QEMU_PACKED;
> > +typedef struct NvdimmMthdIn NvdimmMthdIn;
> > +struct NvdimmDsmIn {
> >      uint32_t revision;
> >      uint32_t function;
> >      /* the remaining size in the page is used by arg3. */
> >      union {
> > -        uint8_t arg3[4084];
> > +        uint8_t arg3[4080];
> >      };
> >  } QEMU_PACKED;
> >  typedef struct NvdimmDsmIn NvdimmDsmIn;
> > -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
> > +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
> >  
> >  struct NvdimmDsmOut {
> >      /* the size of buffer filled by QEMU. */
> > @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> >  } QEMU_PACKED;
> >  typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
> >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> > -                  offsetof(NvdimmDsmIn, arg3) >
> > NVDIMM_DSM_MEMORY_SIZE);
> > +                  offsetof(NvdimmDsmIn, arg3) +
> > offsetof(NvdimmMthdIn, args) >
> > +                  NVDIMM_DSM_MEMORY_SIZE);
> >  
> >  struct NvdimmFuncGetLabelDataOut {
> >      /* the size of buffer filled by QEMU. */
> > @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
> >  } QEMU_PACKED;
> >  typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> > -                  offsetof(NvdimmDsmIn, arg3) >
> > NVDIMM_DSM_MEMORY_SIZE);
> > +                  offsetof(NvdimmDsmIn, arg3) +
> > offsetof(NvdimmMthdIn, args) >
> > +                  NVDIMM_DSM_MEMORY_SIZE);
> >  
> >  struct NvdimmFuncReadFITIn {
> >      uint32_t offset; /* the offset into FIT buffer. */
> >  } QEMU_PACKED;
> >  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> > -                  offsetof(NvdimmDsmIn, arg3) >
> > NVDIMM_DSM_MEMORY_SIZE);
> > +                  offsetof(NvdimmDsmIn, arg3) +
> > offsetof(NvdimmMthdIn, args) >
> > +                  NVDIMM_DSM_MEMORY_SIZE);
> >  
> >  struct NvdimmFuncReadFITOut {
> >      /* the size of buffer filled by QEMU. */
> > @@ -636,7 +644,8 @@ static uint32_t
> > nvdimm_get_max_xfer_label_size(void)
> >       * the max data ACPI can write one time which is transferred
> > by
> >       * 'Set Namespace Label Data' function.
> >       */
> > -    max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3) -
> > +    max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn, args)
> > -
> > +                   offsetof(NvdimmDsmIn, arg3) -
> >                     sizeof(NvdimmFuncSetLabelDataIn);
> >  
> >      return MIN(max_get_size, max_set_size);
> > @@ -697,16 +706,15 @@ static uint32_t
> > nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
> >  /*
> >   * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
> >   */
> > -static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > NvdimmDsmIn *in,
> > -                                      hwaddr dsm_mem_addr)
> > +static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > +                                    NvdimmFuncGetLabelDataIn
> > *get_label_data,
> > +                                    hwaddr dsm_mem_addr)
> >  {
> >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > -    NvdimmFuncGetLabelDataIn *get_label_data;
> >      NvdimmFuncGetLabelDataOut *get_label_data_out;
> >      uint32_t status;
> >      int size;
> >  
> > -    get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> >      get_label_data->offset = le32_to_cpu(get_label_data->offset);
> >      get_label_data->length = le32_to_cpu(get_label_data->length);
> >  
> > @@ -737,15 +745,13 @@ static void
> > nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> >  /*
> >   * DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index 6).
> >   */
> > -static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > NvdimmDsmIn *in,
> > +static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > +                                      NvdimmFuncSetLabelDataIn
> > *set_label_data,
> >                                        hwaddr dsm_mem_addr)
> >  {
> >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > -    NvdimmFuncSetLabelDataIn *set_label_data;
> >      uint32_t status;
> >  
> > -    set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
> > -
> >      set_label_data->offset = le32_to_cpu(set_label_data->offset);
> >      set_label_data->length = le32_to_cpu(set_label_data->length);
> >  
> > @@ -760,19 +766,21 @@ static void
> > nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> >      }
> >  
> >      assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data) +
> > -                    set_label_data->length <=
> > NVDIMM_DSM_MEMORY_SIZE);
> > +           set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE -
> > +           offsetof(NvdimmMthdIn, args));
> >  
> >      nvc->write_label_data(nvdimm, set_label_data->in_buf,
> >                            set_label_data->length, set_label_data-
> > >offset);
> >      nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS,
> > dsm_mem_addr);
> >  }
> >  
> > -static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr
> > dsm_mem_addr)
> > +static void nvdimm_dsm_device(uint32_t nv_handle, NvdimmDsmIn
> > *dsm_in,
> > +                                    hwaddr dsm_mem_addr)
> >  {
> > -    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in-
> > >handle);
> > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> >  
> >      /* See the comments in nvdimm_dsm_root(). */
> > -    if (!in->function) {
> > +    if (!dsm_in->function) {
> >          uint32_t supported_func = 0;
> >  
> >          if (nvdimm && nvdimm->label_size) {
> > @@ -794,7 +802,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in,
> > hwaddr dsm_mem_addr)
> >      }
> >  
> >      /* Encode DSM function according to DSM Spec Rev1. */
> > -    switch (in->function) {
> > +    switch (dsm_in->function) {
> >      case 4 /* Get Namespace Label Size */:
> >          if (nvdimm->label_size) {
> >              nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > @@ -803,13 +811,17 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > *in, hwaddr dsm_mem_addr)
> >          break;
> >      case 5 /* Get Namespace Label Data */:
> >          if (nvdimm->label_size) {
> > -            nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
> > +            nvdimm_dsm_get_label_data(nvdimm,
> > +                                      (NvdimmFuncGetLabelDataIn
> > *)dsm_in->arg3,
> > +                                      dsm_mem_addr);
> >              return;
> >          }
> >          break;
> >      case 0x6 /* Set Namespace Label Data */:
> >          if (nvdimm->label_size) {
> > -            nvdimm_dsm_set_label_data(nvdimm, in, dsm_mem_addr);
> > +            nvdimm_dsm_set_label_data(nvdimm,
> > +                        (NvdimmFuncSetLabelDataIn *)dsm_in->arg3,
> > +                        dsm_mem_addr);
> >              return;
> >          }
> >          break;
> > @@ -819,67 +831,128 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > *in, hwaddr dsm_mem_addr)
> >  }
> >  
> >  static uint64_t
> > -nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> > +nvdimm_method_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> > -    nvdimm_debug("BUG: we never read _DSM IO Port.\n");
> > +    nvdimm_debug("BUG: we never read NVDIMM Method IO Port.\n");
> >      return 0;
> >  }
> >  
> >  static void
> > -nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned
> > size)
> > +nvdimm_dsm_handle(void *opaque, NvdimmMthdIn *method_in, hwaddr
> > dsm_mem_addr)
> >  {
> >      NVDIMMState *state = opaque;
> > -    NvdimmDsmIn *in;
> > -    hwaddr dsm_mem_addr = val;
> > +    NvdimmDsmIn *dsm_in = (NvdimmDsmIn *)method_in->args;
> >  
> >      nvdimm_debug("dsm memory address 0x%" HWADDR_PRIx ".\n",
> > dsm_mem_addr);
> >  
> > -    /*
> > -     * The DSM memory is mapped to guest address space so an evil
> > guest
> > -     * can change its content while we are doing DSM emulation.
> > Avoid
> > -     * this by copying DSM memory to QEMU local memory.
> > -     */
> > -    in = g_new(NvdimmDsmIn, 1);
> > -    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> > -
> > -    in->revision = le32_to_cpu(in->revision);
> > -    in->function = le32_to_cpu(in->function);
> > -    in->handle = le32_to_cpu(in->handle);
> > -
> > -    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > in->revision,
> > -                 in->handle, in->function);
> > +    dsm_in->revision = le32_to_cpu(dsm_in->revision);
> > +    dsm_in->function = le32_to_cpu(dsm_in->function);
> >  
> > +    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > +                 dsm_in->revision, method_in->handle, dsm_in-
> > >function);
> >      /*
> >       * Current NVDIMM _DSM Spec supports Rev1 and Rev2
> >       * Intel® OptanePersistent Memory Module DSM Interface,
> > Revision 2.0
> >       */
> > -    if (in->revision != 0x1 && in->revision != 0x2) {
> > +    if (dsm_in->revision != 0x1 && dsm_in->revision != 0x2) {
> >          nvdimm_debug("Revision 0x%x is not supported, expect 0x1
> > or 0x2.\n",
> > -                     in->revision);
> > +                     dsm_in->revision);
> >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > dsm_mem_addr);
> > -        goto exit;
> > +        return;
> >      }
> >  
> > -    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > -        nvdimm_dsm_handle_reserved_root_method(state, in,
> > dsm_mem_addr);
> > -        goto exit;
> > +    if (method_in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > +        nvdimm_dsm_handle_reserved_root_method(state, dsm_in,
> > dsm_mem_addr);
> > +        return;
> >      }
> >  
> >       /* Handle 0 is reserved for NVDIMM Root Device. */
> > -    if (!in->handle) {
> > -        nvdimm_dsm_root(in, dsm_mem_addr);
> > -        goto exit;
> > +    if (!method_in->handle) {
> > +        nvdimm_dsm_root(dsm_in, dsm_mem_addr);
> > +        return;
> >      }
> >  
> > -    nvdimm_dsm_device(in, dsm_mem_addr);
> > +    nvdimm_dsm_device(method_in->handle, dsm_in, dsm_mem_addr);
> > +}
> >  
> > -exit:
> > -    g_free(in);
> > +static void nvdimm_lsi_handle(uint32_t nv_handle, hwaddr
> > dsm_mem_addr)
> > +{
> > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > +
> > +    if (nvdimm->label_size) {
> > +        nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > +    }
> > +
> > +    return;
> > +}
> > +
> > +static void nvdimm_lsr_handle(uint32_t nv_handle,
> > +                                    void *data,
> > +                                    hwaddr dsm_mem_addr)
> > +{
> > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > +    NvdimmFuncGetLabelDataIn *get_label_data = data;
> > +
> > +    if (nvdimm->label_size) {
> > +        nvdimm_dsm_get_label_data(nvdimm, get_label_data,
> > dsm_mem_addr);
> > +    }
> > +    return;
> > +}
> > +
> > +static void nvdimm_lsw_handle(uint32_t nv_handle,
> > +                                    void *data,
> > +                                    hwaddr dsm_mem_addr)
> > +{
> > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > +    NvdimmFuncSetLabelDataIn *set_label_data = data;
> > +
> > +    if (nvdimm->label_size) {
> > +        nvdimm_dsm_set_label_data(nvdimm, set_label_data,
> > dsm_mem_addr);
> > +    }
> > +    return;
> > +}
> > +
> > +static void
> > +nvdimm_method_write(void *opaque, hwaddr addr, uint64_t val,
> > unsigned size)
> > +{
> > +    NvdimmMthdIn *method_in;
> > +    hwaddr dsm_mem_addr = val;
> > +
> > +    /*
> > +     * The DSM memory is mapped to guest address space so an evil
> > guest
> > +     * can change its content while we are doing DSM emulation.
> > Avoid
> > +     * this by copying DSM memory to QEMU local memory.
> > +     */
> > +    method_in = g_new(NvdimmMthdIn, 1);
> > +    cpu_physical_memory_read(dsm_mem_addr, method_in,
> > sizeof(*method_in));
> > +
> > +    method_in->handle = le32_to_cpu(method_in->handle);
> > +    method_in->method = le32_to_cpu(method_in->method);
> > +
> > +    switch (method_in->method) {
> > +    case NVDIMM_METHOD_DSM:
> > +        nvdimm_dsm_handle(opaque, method_in, dsm_mem_addr);
> > +        break;
> > +    case NVDIMM_METHOD_LSI:
> > +        nvdimm_lsi_handle(method_in->handle, dsm_mem_addr);
> > +        break;
> > +    case NVDIMM_METHOD_LSR:
> > +        nvdimm_lsr_handle(method_in->handle, method_in->args,
> > dsm_mem_addr);
> > +        break;
> > +    case NVDIMM_METHOD_LSW:
> > +        nvdimm_lsw_handle(method_in->handle, method_in->args,
> > dsm_mem_addr);
> > +        break;
> > +    default:
> > +        nvdimm_debug("%s: Unkown method 0x%x\n", __func__,
> > method_in->method);
> > +        break;
> > +    }
> > +
> > +    g_free(method_in);
> >  }
> >  
> > -static const MemoryRegionOps nvdimm_dsm_ops = {
> > -    .read = nvdimm_dsm_read,
> > -    .write = nvdimm_dsm_write,
> > +static const MemoryRegionOps nvdimm_method_ops = {
> > +    .read = nvdimm_method_read,
> > +    .write = nvdimm_method_write,
> >      .endianness = DEVICE_LITTLE_ENDIAN,
> >      .valid = {
> >          .min_access_size = 4,
> > @@ -899,12 +972,12 @@ void nvdimm_init_acpi_state(NVDIMMState
> > *state, MemoryRegion *io,
> >                              FWCfgState *fw_cfg, Object *owner)
> >  {
> >      state->dsm_io = dsm_io;
> > -    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops,
> > state,
> > +    memory_region_init_io(&state->io_mr, owner,
> > &nvdimm_method_ops, state,
> >                            "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));
> > +    acpi_data_push(state->dsm_mem, sizeof(NvdimmMthdIn));
> >      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem-
> > >data,
> >                      state->dsm_mem->len);
> >  
> > @@ -918,13 +991,22 @@ void nvdimm_init_acpi_state(NVDIMMState
> > *state, MemoryRegion *io,
> >  #define NVDIMM_DSM_IOPORT       "NPIO"
> >  
> >  #define NVDIMM_DSM_NOTIFY       "NTFI"
> > +#define NVDIMM_DSM_METHOD       "MTHD"
> >  #define NVDIMM_DSM_HANDLE       "HDLE"
> >  #define NVDIMM_DSM_REVISION     "REVS"
> >  #define NVDIMM_DSM_FUNCTION     "FUNC"
> >  #define NVDIMM_DSM_ARG3         "FARG"
> >  
> > -#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> > -#define NVDIMM_DSM_OUT_BUF      "ODAT"
> > +#define NVDIMM_DSM_OFFSET       "OFST"
> > +#define NVDIMM_DSM_TRANS_LEN    "TRSL"
> > +#define NVDIMM_DSM_IN_BUFF      "IDAT"
> > +
> > +#define NVDIMM_DSM_OUT_BUF_SIZE     "RLEN"
> > +#define NVDIMM_DSM_OUT_BUF          "ODAT"
> > +#define NVDIMM_DSM_OUT_STATUS       "STUS"
> > +#define NVDIMM_DSM_OUT_LSA_SIZE     "SIZE"
> > +#define NVDIMM_DSM_OUT_MAX_TRANS    "MAXT"
> > +
> >  
> >  #define NVDIMM_DSM_RFIT_STATUS  "RSTA"
> >  
> > @@ -938,7 +1020,6 @@ static void nvdimm_build_common_dsm(Aml *dev,
> >      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > *dsm_out_buf_size;
> >      Aml *whilectx, *offset;
> >      uint8_t byte_list[1];
> > -    AmlRegionSpace rs;
> >  
> >      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
> >      uuid = aml_arg(0);
> > @@ -949,37 +1030,15 @@ 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, 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)));
> > -
> > -    /*
> > -     * DSM notifier:
> > -     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > notify QEMU to
> > -     *                    emulate the access.
> > -     *
> > -     * It is the IO port so that accessing them will cause VM-
> > exit, the
> > -     * control will be transferred to QEMU.
> > -     */
> > -    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > AML_NOLOCK,
> > -                      AML_PRESERVE);
> > -    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > -               nvdimm_state->dsm_io.bit_width));
> > -    aml_append(method, field);
> > +               AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmMthdIn)));
> >  
> >      /*
> >       * DSM input:
> >       * NVDIMM_DSM_HANDLE: store device's handle, it's zero if the
> > _DSM call
> >       *                    happens on NVDIMM Root Device.
> > +     * NVDIMM_DSM_METHOD: ACPI method indicator, to distinguish
> > _DSM and
> > +     *                    other ACPI methods.
> >       * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
> >       * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
> >       * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a
> > Package
> > @@ -991,13 +1050,16 @@ static void nvdimm_build_common_dsm(Aml
> > *dev,
> >      field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> >                        AML_PRESERVE);
> >      aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > -               sizeof(typeof_field(NvdimmDsmIn, handle)) *
> > BITS_PER_BYTE));
> > +               sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > BITS_PER_BYTE));
> > +    aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > +               sizeof(typeof_field(NvdimmMthdIn, method)) *
> > BITS_PER_BYTE));
> >      aml_append(field, aml_named_field(NVDIMM_DSM_REVISION,
> >                 sizeof(typeof_field(NvdimmDsmIn, revision)) *
> > BITS_PER_BYTE));
> >      aml_append(field, aml_named_field(NVDIMM_DSM_FUNCTION,
> >                 sizeof(typeof_field(NvdimmDsmIn, function)) *
> > BITS_PER_BYTE));
> >      aml_append(field, aml_named_field(NVDIMM_DSM_ARG3,
> > -         (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) *
> > BITS_PER_BYTE));
> > +         (sizeof(NvdimmMthdIn) - offsetof(NvdimmMthdIn, args) -
> > +          offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
> >      aml_append(method, field);
> >  
> >      /*
> > @@ -1065,6 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
> >       * it reserves 0 for root device and is the handle for NVDIMM
> > devices.
> >       * See the comments in nvdimm_slot_to_handle().
> >       */
> > +    aml_append(method, aml_store(aml_int(0),
> > aml_name(NVDIMM_DSM_METHOD)));
> >      aml_append(method, aml_store(handle,
> > aml_name(NVDIMM_DSM_HANDLE)));
> >      aml_append(method, aml_store(aml_arg(1),
> > aml_name(NVDIMM_DSM_REVISION)));
> >      aml_append(method, aml_store(function,
> > aml_name(NVDIMM_DSM_FUNCTION)));
> > @@ -1250,6 +1313,7 @@ static void nvdimm_build_fit(Aml *dev)
> >  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t
> > ram_slots)
> >  {
> >      uint32_t slot;
> > +    Aml *method, *pkg, *field;
> >  
> >      for (slot = 0; slot < ram_slots; slot++) {
> >          uint32_t handle = nvdimm_slot_to_handle(slot);
> > @@ -1266,6 +1330,155 @@ static void nvdimm_build_nvdimm_devices(Aml
> > *root_dev, uint32_t ram_slots)
> >           * table NFIT or _FIT.
> >           */
> >          aml_append(nvdimm_dev, aml_name_decl("_ADR",
> > aml_int(handle)));
> > +        aml_append(nvdimm_dev,
> > aml_operation_region(NVDIMM_DSM_MEMORY,
> > +                   AML_SYSTEM_MEMORY,
> > aml_name(NVDIMM_ACPI_MEM_ADDR),
> > +                   sizeof(NvdimmMthdIn)));
> > +
> > +        /* ACPI 6.4: 6.5.10 NVDIMM Label Methods, _LS{I,R,W} */
> > +
> > +        /* Begin of _LSI Block */
> > +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> > +        /* _LSI Input field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > BITS_PER_BYTE));
> > +        aml_append(method, field);
> > +
> > +        /* _LSI Output field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > len)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > +                   func_ret_status)) * BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_LSA_SIZE,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > label_size)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field,
> > aml_named_field(NVDIMM_DSM_OUT_MAX_TRANS,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > max_xfer)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(method, field);
> > +
> > +        aml_append(method, aml_store(aml_int(handle),
> > +                                      aml_name(NVDIMM_DSM_HANDLE))
> > );
> > +        aml_append(method, aml_store(aml_int(0x100),
> > +                                      aml_name(NVDIMM_DSM_METHOD))
> > );
> > +        aml_append(method,
> > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > );
> > +
> > +        pkg = aml_package(3);
> > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_LSA_SIZE));
> > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_MAX_TRANS));
> > +
> > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > +
> > +        aml_append(method, aml_return(aml_name("RPKG")));
> > +        aml_append(nvdimm_dev, method); /* End of _LSI Block */
> > +
> > +
> > +        /* Begin of _LSR Block */
> > +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> > +
> > +        /* _LSR Input field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > offset)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > length)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(method, field);
> > +
> > +        /* _LSR Output field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut,
> > len)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut,
> > +                   func_ret_status)) * BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF,
> > +                   (NVDIMM_DSM_MEMORY_SIZE -
> > +                    offsetof(NvdimmFuncGetLabelDataOut, out_buf))
> > *
> > +                    BITS_PER_BYTE));
> > +        aml_append(method, field);
> > +
> > +        aml_append(method, aml_store(aml_int(handle),
> > +                                      aml_name(NVDIMM_DSM_HANDLE))
> > );
> > +        aml_append(method, aml_store(aml_int(0x101),
> > +                                      aml_name(NVDIMM_DSM_METHOD))
> > );
> > +        aml_append(method, aml_store(aml_arg(0),
> > aml_name(NVDIMM_DSM_OFFSET)));
> > +        aml_append(method, aml_store(aml_arg(1),
> > +                                      aml_name(NVDIMM_DSM_TRANS_LE
> > N)));
> > +        aml_append(method,
> > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > );
> > +
> > +        aml_append(method, aml_store(aml_shiftleft(aml_arg(1),
> > aml_int(3)),
> > +                                         aml_local(1)));
> > +        aml_append(method,
> > aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> > +                   aml_int(0), aml_local(1), "OBUF"));
> > +
> > +        pkg = aml_package(2);
> > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > +        aml_append(pkg, aml_name("OBUF"));
> > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > +
> > +        aml_append(method, aml_return(aml_name("RPKG")));
> > +        aml_append(nvdimm_dev, method); /* End of _LSR Block */
> > +
> > +        /* Begin of _LSW Block */
> > +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> > +        /* _LSW Input field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > offset)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > length)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_IN_BUFF,
> > 32640));
> > +        aml_append(method, field);
> > +
> > +        /* _LSW Output field */
> > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                          AML_PRESERVE);
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut,
> > len)) *
> > +                   BITS_PER_BYTE));
> > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut,
> > +                   func_ret_status)) * BITS_PER_BYTE));
> > +        aml_append(method, field);
> > +
> > +        aml_append(method, aml_store(aml_int(handle),
> > aml_name(NVDIMM_DSM_HANDLE)));
> > +        aml_append(method, aml_store(aml_int(0x102),
> > aml_name(NVDIMM_DSM_METHOD)));
> > +        aml_append(method, aml_store(aml_arg(0),
> > aml_name(NVDIMM_DSM_OFFSET)));
> > +        aml_append(method, aml_store(aml_arg(1),
> > aml_name(NVDIMM_DSM_TRANS_LEN)));
> > +        aml_append(method, aml_store(aml_arg(2),
> > aml_name(NVDIMM_DSM_IN_BUFF)));
> > +        aml_append(method,
> > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > );
> > +
> > +        aml_append(method,
> > aml_return(aml_name(NVDIMM_DSM_OUT_STATUS)));
> > +        aml_append(nvdimm_dev, method); /* End of _LSW Block */
> >  
> >          nvdimm_build_device_dsm(nvdimm_dev, handle);
> >          aml_append(root_dev, nvdimm_dev);
> > @@ -1278,7 +1491,8 @@ static void nvdimm_build_ssdt(GArray
> > *table_offsets, GArray *table_data,
> >                                uint32_t ram_slots, const char
> > *oem_id)
> >  {
> >      int mem_addr_offset;
> > -    Aml *ssdt, *sb_scope, *dev;
> > +    Aml *ssdt, *sb_scope, *dev, *field;
> > +    AmlRegionSpace rs;
> >      AcpiTable table = { .sig = "SSDT", .rev = 1,
> >                          .oem_id = oem_id, .oem_table_id = "NVDIMM"
> > };
> >  
> > @@ -1286,6 +1500,9 @@ static void nvdimm_build_ssdt(GArray
> > *table_offsets, GArray *table_data,
> >  
> >      acpi_table_begin(&table, table_data);
> >      ssdt = init_aml_allocator();
> > +
> > +    mem_addr_offset = build_append_named_dword(table_data,
> > +                                               NVDIMM_ACPI_MEM_ADD
> > R);
> >      sb_scope = aml_scope("\\_SB");
> >  
> >      dev = aml_device("NVDR");
> > @@ -1303,6 +1520,31 @@ static void nvdimm_build_ssdt(GArray
> > *table_offsets, GArray *table_data,
> >       */
> >      aml_append(dev, aml_name_decl("_HID",
> > aml_string("ACPI0012")));
> >  
> > +    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(dev, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> > +               aml_int(nvdimm_state->dsm_io.address),
> > +               nvdimm_state->dsm_io.bit_width >> 3));
> > +
> > +    /*
> > +     * DSM notifier:
> > +     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > notify QEMU to
> > +     *                    emulate the access.
> > +     *
> > +     * It is the IO port so that accessing them will cause VM-
> > exit, the
> > +     * control will be transferred to QEMU.
> > +     */
> > +    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > AML_NOLOCK,
> > +                      AML_PRESERVE);
> > +    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > +               nvdimm_state->dsm_io.bit_width));
> > +    aml_append(dev, field);
> > +
> >      nvdimm_build_common_dsm(dev, nvdimm_state);
> >  
> >      /* 0 is reserved for root device. */
> > @@ -1316,12 +1558,10 @@ static void nvdimm_build_ssdt(GArray
> > *table_offsets, GArray *table_data,
> >  
> >      /* copy AML table into ACPI tables blob and patch header there
> > */
> >      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf-
> > >len);
> > -    mem_addr_offset = build_append_named_dword(table_data,
> > -                                               NVDIMM_ACPI_MEM_ADD
> > R);
> >  
> >      bios_linker_loader_alloc(linker,
> >                               NVDIMM_DSM_MEM_FILE, nvdimm_state-
> > >dsm_mem,
> > -                             sizeof(NvdimmDsmIn), false /* high
> > memory */);
> > +                             sizeof(NvdimmMthdIn), false /* high
> > memory */);
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
> >          NVDIMM_DSM_MEM_FILE, 0);
> > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> > index cf8f59be44..0206b6125b 100644
> > --- a/include/hw/mem/nvdimm.h
> > +++ b/include/hw/mem/nvdimm.h
> > @@ -37,6 +37,12 @@
> >          }                                                     \
> >      } while (0)
> >  
> > +/* NVDIMM ACPI Methods */
> > +#define NVDIMM_METHOD_DSM   0
> > +#define NVDIMM_METHOD_LSI   0x100
> > +#define NVDIMM_METHOD_LSR   0x101
> > +#define NVDIMM_METHOD_LSW   0x102
> > +
> >  /*
> >   * The minimum label data size is required by NVDIMM Namespace
> >   * specification, see the chapter 2 Namespaces:
> 
>

Ping...
On Fri, 2022-07-01 at 17:23 +0800, Robert Hoo wrote:
> On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> > On Mon, 30 May 2022 11:40:45 +0800
> > Robert Hoo <robert.hu@linux.intel.com> wrote:
> > 
> > > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > > which
> > > depricates corresponding _DSM Functions defined by PMEM _DSM
> > > Interface spec
> > > [2].
> > > 
> > > In this implementation, we do 2 things
> > > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > > ACPI
> > > method dispatch, _DSM is one of the branches. This also paves the
> > > way for
> > > adding other ACPI methods for NVDIMM.
> > > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > > ASL form of SSDT changes can be found in next test/qtest/bios-
> > > table-test
> > > commit message.
> > > 
> > > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated
> > > Functions
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
> > > ---
> > >  hw/acpi/nvdimm.c        | 424 +++++++++++++++++++++++++++++++---
> > > --
> > > ----
> > 
> > This patch is too large and doing to many things to be reviewable.
> > It needs to be split into smaller distinct chunks.
> > (however hold your horses and read on)
> > 
> > The patch it is too intrusive and my hunch is that it breaks
> > ABI and needs a bunch of compat knobs to work properly and
> > that I'd like to avoid unless there is not other way around
> > the problem.
> 
> Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> and the compat knobs refers to related functions' input/output
> params?
> 
> My thoughts is that eventually, sooner or later, more ACPI methods
> will
> be implemented per request, although now we can play the trick of
> wrapper new methods over the pipe of old _DSM implementation.
> Though this changes a little on existing struct NvdimmDsmIn {}, it
> paves the way for the future; and actually the change is more an
> extension or generalization, not fundamentally changes the framework.
> 
> In short, my point is the change/generalization/extension will be
> inevitable, even if not present.
> > 
> > I was skeptical about this approach during v1 review and
> > now I'm pretty much sure it's over-engineered and we can
> > just repack data we receive from existing label _DSM functions
> > to provide _LS{I,R,W} like it was suggested in v1.
> > It will be much simpler and affect only AML side without
> > complicating ABI and without any compat cruft and will work
> > with ping-pong migration without any issues.
> 
> Ostensibly it may looks simpler, actually not, I think. The AML
> "common
> pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> function
> logics there, packing new stuff in/through it will be bug-prone.
> Though this time we can avert touching it, as the new ACPI methods
> deprecating old _DSM functionally is almost the same.
> How about next time? are we going to always packing new methods logic
> in NCAL()?
> My point is that we should implement new methods as itself, of
> course,
> as a general programming rule, we can/should abstract common
> routines,
> but not packing them in one large function.
> > 
> > 
> > >  include/hw/mem/nvdimm.h |   6 +
> > >  2 files changed, 338 insertions(+), 92 deletions(-)
> > > 
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index 59b42afcf1..50ee85866b 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > > *state, GArray *table_offsets,
> > >  
> > >  #define NVDIMM_DSM_MEMORY_SIZE      4096
> > >  
> > > -struct NvdimmDsmIn {
> > > +struct NvdimmMthdIn {
> > >      uint32_t handle;
> > > +    uint32_t method;
> > > +    uint8_t  args[4088];
> > > +} QEMU_PACKED;
> > > +typedef struct NvdimmMthdIn NvdimmMthdIn;
> > > +struct NvdimmDsmIn {
> > >      uint32_t revision;
> > >      uint32_t function;
> > >      /* the remaining size in the page is used by arg3. */
> > >      union {
> > > -        uint8_t arg3[4084];
> > > +        uint8_t arg3[4080];
> > >      };
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmDsmIn NvdimmDsmIn;
> > > -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) !=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) !=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmDsmOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncGetLabelDataIn
> > > NvdimmFuncGetLabelDataIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncGetLabelDataOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncSetLabelDataIn
> > > NvdimmFuncSetLabelDataIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncReadFITIn {
> > >      uint32_t offset; /* the offset into FIT buffer. */
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncReadFITOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -636,7 +644,8 @@ static uint32_t
> > > nvdimm_get_max_xfer_label_size(void)
> > >       * the max data ACPI can write one time which is transferred
> > > by
> > >       * 'Set Namespace Label Data' function.
> > >       */
> > > -    max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3)
> > > -
> > > +    max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn,
> > > args)
> > > -
> > > +                   offsetof(NvdimmDsmIn, arg3) -
> > >                     sizeof(NvdimmFuncSetLabelDataIn);
> > >  
> > >      return MIN(max_get_size, max_set_size);
> > > @@ -697,16 +706,15 @@ static uint32_t
> > > nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
> > >  /*
> > >   * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index
> > > 5).
> > >   */
> > > -static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > > NvdimmDsmIn *in,
> > > -                                      hwaddr dsm_mem_addr)
> > > +static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > > +                                    NvdimmFuncGetLabelDataIn
> > > *get_label_data,
> > > +                                    hwaddr dsm_mem_addr)
> > >  {
> > >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > > -    NvdimmFuncGetLabelDataIn *get_label_data;
> > >      NvdimmFuncGetLabelDataOut *get_label_data_out;
> > >      uint32_t status;
> > >      int size;
> > >  
> > > -    get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> > >      get_label_data->offset = le32_to_cpu(get_label_data-
> > > >offset);
> > >      get_label_data->length = le32_to_cpu(get_label_data-
> > > >length);
> > >  
> > > @@ -737,15 +745,13 @@ static void
> > > nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> > >  /*
> > >   * DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index
> > > 6).
> > >   */
> > > -static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > > NvdimmDsmIn *in,
> > > +static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > > +                                      NvdimmFuncSetLabelDataIn
> > > *set_label_data,
> > >                                        hwaddr dsm_mem_addr)
> > >  {
> > >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > > -    NvdimmFuncSetLabelDataIn *set_label_data;
> > >      uint32_t status;
> > >  
> > > -    set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
> > > -
> > >      set_label_data->offset = le32_to_cpu(set_label_data-
> > > >offset);
> > >      set_label_data->length = le32_to_cpu(set_label_data-
> > > >length);
> > >  
> > > @@ -760,19 +766,21 @@ static void
> > > nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> > >      }
> > >  
> > >      assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data)
> > > +
> > > -                    set_label_data->length <=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +           set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE -
> > > +           offsetof(NvdimmMthdIn, args));
> > >  
> > >      nvc->write_label_data(nvdimm, set_label_data->in_buf,
> > >                            set_label_data->length,
> > > set_label_data-
> > > > offset);
> > > 
> > >      nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS,
> > > dsm_mem_addr);
> > >  }
> > >  
> > > -static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr
> > > dsm_mem_addr)
> > > +static void nvdimm_dsm_device(uint32_t nv_handle, NvdimmDsmIn
> > > *dsm_in,
> > > +                                    hwaddr dsm_mem_addr)
> > >  {
> > > -    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in-
> > > > handle);
> > > 
> > > +    NVDIMMDevice *nvdimm =
> > > nvdimm_get_device_by_handle(nv_handle);
> > >  
> > >      /* See the comments in nvdimm_dsm_root(). */
> > > -    if (!in->function) {
> > > +    if (!dsm_in->function) {
> > >          uint32_t supported_func = 0;
> > >  
> > >          if (nvdimm && nvdimm->label_size) {
> > > @@ -794,7 +802,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > > *in,
> > > hwaddr dsm_mem_addr)
> > >      }
> > >  
> > >      /* Encode DSM function according to DSM Spec Rev1. */
> > > -    switch (in->function) {
> > > +    switch (dsm_in->function) {
> > >      case 4 /* Get Namespace Label Size */:
> > >          if (nvdimm->label_size) {
> > >              nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > > @@ -803,13 +811,17 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > > *in, hwaddr dsm_mem_addr)
> > >          break;
> > >      case 5 /* Get Namespace Label Data */:
> > >          if (nvdimm->label_size) {
> > > -            nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
> > > +            nvdimm_dsm_get_label_data(nvdimm,
> > > +                                      (NvdimmFuncGetLabelDataIn
> > > *)dsm_in->arg3,
> > > +                                      dsm_mem_addr);
> > >              return;
> > >          }
> > >          break;
> > >      case 0x6 /* Set Namespace Label Data */:
> > >          if (nvdimm->label_size) {
> > > -            nvdimm_dsm_set_label_data(nvdimm, in, dsm_mem_addr);
> > > +            nvdimm_dsm_set_label_data(nvdimm,
> > > +                        (NvdimmFuncSetLabelDataIn *)dsm_in-
> > > >arg3,
> > > +                        dsm_mem_addr);
> > >              return;
> > >          }
> > >          break;
> > > @@ -819,67 +831,128 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > > *in, hwaddr dsm_mem_addr)
> > >  }
> > >  
> > >  static uint64_t
> > > -nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> > > +nvdimm_method_read(void *opaque, hwaddr addr, unsigned size)
> > >  {
> > > -    nvdimm_debug("BUG: we never read _DSM IO Port.\n");
> > > +    nvdimm_debug("BUG: we never read NVDIMM Method IO Port.\n");
> > >      return 0;
> > >  }
> > >  
> > >  static void
> > > -nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val,
> > > unsigned
> > > size)
> > > +nvdimm_dsm_handle(void *opaque, NvdimmMthdIn *method_in, hwaddr
> > > dsm_mem_addr)
> > >  {
> > >      NVDIMMState *state = opaque;
> > > -    NvdimmDsmIn *in;
> > > -    hwaddr dsm_mem_addr = val;
> > > +    NvdimmDsmIn *dsm_in = (NvdimmDsmIn *)method_in->args;
> > >  
> > >      nvdimm_debug("dsm memory address 0x%" HWADDR_PRIx ".\n",
> > > dsm_mem_addr);
> > >  
> > > -    /*
> > > -     * The DSM memory is mapped to guest address space so an
> > > evil
> > > guest
> > > -     * can change its content while we are doing DSM emulation.
> > > Avoid
> > > -     * this by copying DSM memory to QEMU local memory.
> > > -     */
> > > -    in = g_new(NvdimmDsmIn, 1);
> > > -    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> > > -
> > > -    in->revision = le32_to_cpu(in->revision);
> > > -    in->function = le32_to_cpu(in->function);
> > > -    in->handle = le32_to_cpu(in->handle);
> > > -
> > > -    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > > in->revision,
> > > -                 in->handle, in->function);
> > > +    dsm_in->revision = le32_to_cpu(dsm_in->revision);
> > > +    dsm_in->function = le32_to_cpu(dsm_in->function);
> > >  
> > > +    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > > +                 dsm_in->revision, method_in->handle, dsm_in-
> > > > function);
> > > 
> > >      /*
> > >       * Current NVDIMM _DSM Spec supports Rev1 and Rev2
> > >       * Intel® OptanePersistent Memory Module DSM Interface,
> > > Revision 2.0
> > >       */
> > > -    if (in->revision != 0x1 && in->revision != 0x2) {
> > > +    if (dsm_in->revision != 0x1 && dsm_in->revision != 0x2) {
> > >          nvdimm_debug("Revision 0x%x is not supported, expect 0x1
> > > or 0x2.\n",
> > > -                     in->revision);
> > > +                     dsm_in->revision);
> > >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > > dsm_mem_addr);
> > > -        goto exit;
> > > +        return;
> > >      }
> > >  
> > > -    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > > -        nvdimm_dsm_handle_reserved_root_method(state, in,
> > > dsm_mem_addr);
> > > -        goto exit;
> > > +    if (method_in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > > +        nvdimm_dsm_handle_reserved_root_method(state, dsm_in,
> > > dsm_mem_addr);
> > > +        return;
> > >      }
> > >  
> > >       /* Handle 0 is reserved for NVDIMM Root Device. */
> > > -    if (!in->handle) {
> > > -        nvdimm_dsm_root(in, dsm_mem_addr);
> > > -        goto exit;
> > > +    if (!method_in->handle) {
> > > +        nvdimm_dsm_root(dsm_in, dsm_mem_addr);
> > > +        return;
> > >      }
> > >  
> > > -    nvdimm_dsm_device(in, dsm_mem_addr);
> > > +    nvdimm_dsm_device(method_in->handle, dsm_in, dsm_mem_addr);
> > > +}
> > >  
> > > -exit:
> > > -    g_free(in);
> > > +static void nvdimm_lsi_handle(uint32_t nv_handle, hwaddr
> > > dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm =
> > > nvdimm_get_device_by_handle(nv_handle);
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > > +    }
> > > +
> > > +    return;
> > > +}
> > > +
> > > +static void nvdimm_lsr_handle(uint32_t nv_handle,
> > > +                                    void *data,
> > > +                                    hwaddr dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm =
> > > nvdimm_get_device_by_handle(nv_handle);
> > > +    NvdimmFuncGetLabelDataIn *get_label_data = data;
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_get_label_data(nvdimm, get_label_data,
> > > dsm_mem_addr);
> > > +    }
> > > +    return;
> > > +}
> > > +
> > > +static void nvdimm_lsw_handle(uint32_t nv_handle,
> > > +                                    void *data,
> > > +                                    hwaddr dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm =
> > > nvdimm_get_device_by_handle(nv_handle);
> > > +    NvdimmFuncSetLabelDataIn *set_label_data = data;
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_set_label_data(nvdimm, set_label_data,
> > > dsm_mem_addr);
> > > +    }
> > > +    return;
> > > +}
> > > +
> > > +static void
> > > +nvdimm_method_write(void *opaque, hwaddr addr, uint64_t val,
> > > unsigned size)
> > > +{
> > > +    NvdimmMthdIn *method_in;
> > > +    hwaddr dsm_mem_addr = val;
> > > +
> > > +    /*
> > > +     * The DSM memory is mapped to guest address space so an
> > > evil
> > > guest
> > > +     * can change its content while we are doing DSM emulation.
> > > Avoid
> > > +     * this by copying DSM memory to QEMU local memory.
> > > +     */
> > > +    method_in = g_new(NvdimmMthdIn, 1);
> > > +    cpu_physical_memory_read(dsm_mem_addr, method_in,
> > > sizeof(*method_in));
> > > +
> > > +    method_in->handle = le32_to_cpu(method_in->handle);
> > > +    method_in->method = le32_to_cpu(method_in->method);
> > > +
> > > +    switch (method_in->method) {
> > > +    case NVDIMM_METHOD_DSM:
> > > +        nvdimm_dsm_handle(opaque, method_in, dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSI:
> > > +        nvdimm_lsi_handle(method_in->handle, dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSR:
> > > +        nvdimm_lsr_handle(method_in->handle, method_in->args,
> > > dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSW:
> > > +        nvdimm_lsw_handle(method_in->handle, method_in->args,
> > > dsm_mem_addr);
> > > +        break;
> > > +    default:
> > > +        nvdimm_debug("%s: Unkown method 0x%x\n", __func__,
> > > method_in->method);
> > > +        break;
> > > +    }
> > > +
> > > +    g_free(method_in);
> > >  }
> > >  
> > > -static const MemoryRegionOps nvdimm_dsm_ops = {
> > > -    .read = nvdimm_dsm_read,
> > > -    .write = nvdimm_dsm_write,
> > > +static const MemoryRegionOps nvdimm_method_ops = {
> > > +    .read = nvdimm_method_read,
> > > +    .write = nvdimm_method_write,
> > >      .endianness = DEVICE_LITTLE_ENDIAN,
> > >      .valid = {
> > >          .min_access_size = 4,
> > > @@ -899,12 +972,12 @@ void nvdimm_init_acpi_state(NVDIMMState
> > > *state, MemoryRegion *io,
> > >                              FWCfgState *fw_cfg, Object *owner)
> > >  {
> > >      state->dsm_io = dsm_io;
> > > -    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops,
> > > state,
> > > +    memory_region_init_io(&state->io_mr, owner,
> > > &nvdimm_method_ops, state,
> > >                            "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));
> > > +    acpi_data_push(state->dsm_mem, sizeof(NvdimmMthdIn));
> > >      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem-
> > > > data,
> > > 
> > >                      state->dsm_mem->len);
> > >  
> > > @@ -918,13 +991,22 @@ void nvdimm_init_acpi_state(NVDIMMState
> > > *state, MemoryRegion *io,
> > >  #define NVDIMM_DSM_IOPORT       "NPIO"
> > >  
> > >  #define NVDIMM_DSM_NOTIFY       "NTFI"
> > > +#define NVDIMM_DSM_METHOD       "MTHD"
> > >  #define NVDIMM_DSM_HANDLE       "HDLE"
> > >  #define NVDIMM_DSM_REVISION     "REVS"
> > >  #define NVDIMM_DSM_FUNCTION     "FUNC"
> > >  #define NVDIMM_DSM_ARG3         "FARG"
> > >  
> > > -#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> > > -#define NVDIMM_DSM_OUT_BUF      "ODAT"
> > > +#define NVDIMM_DSM_OFFSET       "OFST"
> > > +#define NVDIMM_DSM_TRANS_LEN    "TRSL"
> > > +#define NVDIMM_DSM_IN_BUFF      "IDAT"
> > > +
> > > +#define NVDIMM_DSM_OUT_BUF_SIZE     "RLEN"
> > > +#define NVDIMM_DSM_OUT_BUF          "ODAT"
> > > +#define NVDIMM_DSM_OUT_STATUS       "STUS"
> > > +#define NVDIMM_DSM_OUT_LSA_SIZE     "SIZE"
> > > +#define NVDIMM_DSM_OUT_MAX_TRANS    "MAXT"
> > > +
> > >  
> > >  #define NVDIMM_DSM_RFIT_STATUS  "RSTA"
> > >  
> > > @@ -938,7 +1020,6 @@ static void nvdimm_build_common_dsm(Aml
> > > *dev,
> > >      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > > *dsm_out_buf_size;
> > >      Aml *whilectx, *offset;
> > >      uint8_t byte_list[1];
> > > -    AmlRegionSpace rs;
> > >  
> > >      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
> > >      uuid = aml_arg(0);
> > > @@ -949,37 +1030,15 @@ 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,
> > > 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)));
> > > -
> > > -    /*
> > > -     * DSM notifier:
> > > -     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > > notify QEMU to
> > > -     *                    emulate the access.
> > > -     *
> > > -     * It is the IO port so that accessing them will cause VM-
> > > exit, the
> > > -     * control will be transferred to QEMU.
> > > -     */
> > > -    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > -                      AML_PRESERVE);
> > > -    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > > -               nvdimm_state->dsm_io.bit_width));
> > > -    aml_append(method, field);
> > > +               AML_SYSTEM_MEMORY, dsm_mem,
> > > sizeof(NvdimmMthdIn)));
> > >  
> > >      /*
> > >       * DSM input:
> > >       * NVDIMM_DSM_HANDLE: store device's handle, it's zero if
> > > the
> > > _DSM call
> > >       *                    happens on NVDIMM Root Device.
> > > +     * NVDIMM_DSM_METHOD: ACPI method indicator, to distinguish
> > > _DSM and
> > > +     *                    other ACPI methods.
> > >       * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
> > >       * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
> > >       * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a
> > > Package
> > > @@ -991,13 +1050,16 @@ static void nvdimm_build_common_dsm(Aml
> > > *dev,
> > >      field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > >                        AML_PRESERVE);
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > -               sizeof(typeof_field(NvdimmDsmIn, handle)) *
> > > BITS_PER_BYTE));
> > > +               sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +    aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +               sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_REVISION,
> > >                 sizeof(typeof_field(NvdimmDsmIn, revision)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_FUNCTION,
> > >                 sizeof(typeof_field(NvdimmDsmIn, function)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_ARG3,
> > > -         (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) *
> > > BITS_PER_BYTE));
> > > +         (sizeof(NvdimmMthdIn) - offsetof(NvdimmMthdIn, args) -
> > > +          offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
> > >      aml_append(method, field);
> > >  
> > >      /*
> > > @@ -1065,6 +1127,7 @@ static void nvdimm_build_common_dsm(Aml
> > > *dev,
> > >       * it reserves 0 for root device and is the handle for
> > > NVDIMM
> > > devices.
> > >       * See the comments in nvdimm_slot_to_handle().
> > >       */
> > > +    aml_append(method, aml_store(aml_int(0),
> > > aml_name(NVDIMM_DSM_METHOD)));
> > >      aml_append(method, aml_store(handle,
> > > aml_name(NVDIMM_DSM_HANDLE)));
> > >      aml_append(method, aml_store(aml_arg(1),
> > > aml_name(NVDIMM_DSM_REVISION)));
> > >      aml_append(method, aml_store(function,
> > > aml_name(NVDIMM_DSM_FUNCTION)));
> > > @@ -1250,6 +1313,7 @@ static void nvdimm_build_fit(Aml *dev)
> > >  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t
> > > ram_slots)
> > >  {
> > >      uint32_t slot;
> > > +    Aml *method, *pkg, *field;
> > >  
> > >      for (slot = 0; slot < ram_slots; slot++) {
> > >          uint32_t handle = nvdimm_slot_to_handle(slot);
> > > @@ -1266,6 +1330,155 @@ static void
> > > nvdimm_build_nvdimm_devices(Aml
> > > *root_dev, uint32_t ram_slots)
> > >           * table NFIT or _FIT.
> > >           */
> > >          aml_append(nvdimm_dev, aml_name_decl("_ADR",
> > > aml_int(handle)));
> > > +        aml_append(nvdimm_dev,
> > > aml_operation_region(NVDIMM_DSM_MEMORY,
> > > +                   AML_SYSTEM_MEMORY,
> > > aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                   sizeof(NvdimmMthdIn)));
> > > +
> > > +        /* ACPI 6.4: 6.5.10 NVDIMM Label Methods, _LS{I,R,W} */
> > > +
> > > +        /* Begin of _LSI Block */
> > > +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> > > +        /* _LSI Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSI Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut
> > > ,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut
> > > ,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_LSA_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut
> > > ,
> > > label_size)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_MAX_TRANS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut
> > > ,
> > > max_xfer)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > +                                      aml_name(NVDIMM_DSM_HANDLE
> > > ))
> > > );
> > > +        aml_append(method, aml_store(aml_int(0x100),
> > > +                                      aml_name(NVDIMM_DSM_METHOD
> > > ))
> > > );
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY
> > > ))
> > > );
> > > +
> > > +        pkg = aml_package(3);
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_LSA_SIZE));
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_MAX_TRANS));
> > > +
> > > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > > +
> > > +        aml_append(method, aml_return(aml_name("RPKG")));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSI Block */
> > > +
> > > +
> > > +        /* Begin of _LSR Block */
> > > +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> > > +
> > > +        /* _LSR Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > > offset)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > > length)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSR Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut
> > > ,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut
> > > ,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF,
> > > +                   (NVDIMM_DSM_MEMORY_SIZE -
> > > +                    offsetof(NvdimmFuncGetLabelDataOut,
> > > out_buf))
> > > *
> > > +                    BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > +                                      aml_name(NVDIMM_DSM_HANDLE
> > > ))
> > > );
> > > +        aml_append(method, aml_store(aml_int(0x101),
> > > +                                      aml_name(NVDIMM_DSM_METHOD
> > > ))
> > > );
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name(NVDIMM_DSM_OFFSET)));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > +                                      aml_name(NVDIMM_DSM_TRANS_
> > > LE
> > > N)));
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY
> > > ))
> > > );
> > > +
> > > +        aml_append(method, aml_store(aml_shiftleft(aml_arg(1),
> > > aml_int(3)),
> > > +                                         aml_local(1)));
> > > +        aml_append(method,
> > > aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> > > +                   aml_int(0), aml_local(1), "OBUF"));
> > > +
> > > +        pkg = aml_package(2);
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > > +        aml_append(pkg, aml_name("OBUF"));
> > > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > > +
> > > +        aml_append(method, aml_return(aml_name("RPKG")));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSR Block */
> > > +
> > > +        /* Begin of _LSW Block */
> > > +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> > > +        /* _LSW Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > > offset)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > > length)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_IN_BUFF,
> > > 32640));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSW Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut
> > > ,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut
> > > ,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > aml_name(NVDIMM_DSM_HANDLE)));
> > > +        aml_append(method, aml_store(aml_int(0x102),
> > > aml_name(NVDIMM_DSM_METHOD)));
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name(NVDIMM_DSM_OFFSET)));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > aml_name(NVDIMM_DSM_TRANS_LEN)));
> > > +        aml_append(method, aml_store(aml_arg(2),
> > > aml_name(NVDIMM_DSM_IN_BUFF)));
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY
> > > ))
> > > );
> > > +
> > > +        aml_append(method,
> > > aml_return(aml_name(NVDIMM_DSM_OUT_STATUS)));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSW Block */
> > >  
> > >          nvdimm_build_device_dsm(nvdimm_dev, handle);
> > >          aml_append(root_dev, nvdimm_dev);
> > > @@ -1278,7 +1491,8 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >                                uint32_t ram_slots, const char
> > > *oem_id)
> > >  {
> > >      int mem_addr_offset;
> > > -    Aml *ssdt, *sb_scope, *dev;
> > > +    Aml *ssdt, *sb_scope, *dev, *field;
> > > +    AmlRegionSpace rs;
> > >      AcpiTable table = { .sig = "SSDT", .rev = 1,
> > >                          .oem_id = oem_id, .oem_table_id =
> > > "NVDIMM"
> > > };
> > >  
> > > @@ -1286,6 +1500,9 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >  
> > >      acpi_table_begin(&table, table_data);
> > >      ssdt = init_aml_allocator();
> > > +
> > > +    mem_addr_offset = build_append_named_dword(table_data,
> > > +                                               NVDIMM_ACPI_MEM_A
> > > DD
> > > R);
> > >      sb_scope = aml_scope("\\_SB");
> > >  
> > >      dev = aml_device("NVDR");
> > > @@ -1303,6 +1520,31 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >       */
> > >      aml_append(dev, aml_name_decl("_HID",
> > > aml_string("ACPI0012")));
> > >  
> > > +    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(dev, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> > > +               aml_int(nvdimm_state->dsm_io.address),
> > > +               nvdimm_state->dsm_io.bit_width >> 3));
> > > +
> > > +    /*
> > > +     * DSM notifier:
> > > +     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > > notify QEMU to
> > > +     *                    emulate the access.
> > > +     *
> > > +     * It is the IO port so that accessing them will cause VM-
> > > exit, the
> > > +     * control will be transferred to QEMU.
> > > +     */
> > > +    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                      AML_PRESERVE);
> > > +    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > > +               nvdimm_state->dsm_io.bit_width));
> > > +    aml_append(dev, field);
> > > +
> > >      nvdimm_build_common_dsm(dev, nvdimm_state);
> > >  
> > >      /* 0 is reserved for root device. */
> > > @@ -1316,12 +1558,10 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >  
> > >      /* copy AML table into ACPI tables blob and patch header
> > > there
> > > */
> > >      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf-
> > > > len);
> > > 
> > > -    mem_addr_offset = build_append_named_dword(table_data,
> > > -                                               NVDIMM_ACPI_MEM_A
> > > DD
> > > R);
> > >  
> > >      bios_linker_loader_alloc(linker,
> > >                               NVDIMM_DSM_MEM_FILE, nvdimm_state-
> > > > dsm_mem,
> > > 
> > > -                             sizeof(NvdimmDsmIn), false /* high
> > > memory */);
> > > +                             sizeof(NvdimmMthdIn), false /* high
> > > memory */);
> > >      bios_linker_loader_add_pointer(linker,
> > >          ACPI_BUILD_TABLE_FILE, mem_addr_offset,
> > > sizeof(uint32_t),
> > >          NVDIMM_DSM_MEM_FILE, 0);
> > > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> > > index cf8f59be44..0206b6125b 100644
> > > --- a/include/hw/mem/nvdimm.h
> > > +++ b/include/hw/mem/nvdimm.h
> > > @@ -37,6 +37,12 @@
> > >          }                                                     \
> > >      } while (0)
> > >  
> > > +/* NVDIMM ACPI Methods */
> > > +#define NVDIMM_METHOD_DSM   0
> > > +#define NVDIMM_METHOD_LSI   0x100
> > > +#define NVDIMM_METHOD_LSR   0x101
> > > +#define NVDIMM_METHOD_LSW   0x102
> > > +
> > >  /*
> > >   * The minimum label data size is required by NVDIMM Namespace
> > >   * specification, see the chapter 2 Namespaces:
> > 
> >

On Tue, Jul 19, 2022 at 10:46:38AM +0800, Robert Hoo wrote:
> Ping...

Igor could you respond? It's been 3 weeks ...

On Fri, 01 Jul 2022 17:23:04 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> > On Mon, 30 May 2022 11:40:45 +0800
> > Robert Hoo <robert.hu@linux.intel.com> wrote:
> >   
> > > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > > which
> > > depricates corresponding _DSM Functions defined by PMEM _DSM
> > > Interface spec
> > > [2].
> > > 
> > > In this implementation, we do 2 things
> > > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > > ACPI
> > > method dispatch, _DSM is one of the branches. This also paves the
> > > way for
> > > adding other ACPI methods for NVDIMM.
> > > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > > ASL form of SSDT changes can be found in next test/qtest/bios-
> > > table-test
> > > commit message.
> > > 
> > > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
> > > ---
> > >  hw/acpi/nvdimm.c        | 424 +++++++++++++++++++++++++++++++-----
> > > ----  
> > 
> > This patch is too large and doing to many things to be reviewable.
> > It needs to be split into smaller distinct chunks.
> > (however hold your horses and read on)
> > 
> > The patch it is too intrusive and my hunch is that it breaks
> > ABI and needs a bunch of compat knobs to work properly and
> > that I'd like to avoid unless there is not other way around
> > the problem.  
> 
> Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> and the compat knobs refers to related functions' input/output params?

ABI are structures that guest and QEMU pass through information
between each other. And knobs in this case would be compat variable[s]
to keep old behavior in place for old machine types.

> My thoughts is that eventually, sooner or later, more ACPI methods will
> be implemented per request, although now we can play the trick of
> wrapper new methods over the pipe of old _DSM implementation.
> Though this changes a little on existing struct NvdimmDsmIn {}, it
> paves the way for the future; and actually the change is more an
> extension or generalization, not fundamentally changes the framework.
> 
> In short, my point is the change/generalization/extension will be
> inevitable, even if not present.

Expanding ABI (interface between host&guest) has 2 drawbacks
 * it exposes more attack surface of VMM to hostile guest
   and rises chances that vulnerability would slip through
   review/testing
 * migration wise, QEMU has to support any ABI for years
   and not only latest an greatest interface but also old
   ones to keep guest started on older QEMU working across
   migration, so any ABI change should be considered very
   carefully before being implemented otherwise it all
   quickly snowballs in unsupportable mess of compat
   variables smeared across host/guest.
   Reducing exposed ABI and constant need to expand it
   was a reason why we have moved ACPI code from firmware
   into QEMU, so we could describe hardware without costs
   associated with of maintaining ABI.

There might be need to extend ABI eventually, but not in this case.

> > I was skeptical about this approach during v1 review and
> > now I'm pretty much sure it's over-engineered and we can
> > just repack data we receive from existing label _DSM functions
> > to provide _LS{I,R,W} like it was suggested in v1.
> > It will be much simpler and affect only AML side without
> > complicating ABI and without any compat cruft and will work
> > with ping-pong migration without any issues.  
> 
> Ostensibly it may looks simpler, actually not, I think. The AML "common
> pipe" NCAL() is already complex, it packs all _DSMs and NFIT() function
> logics there, packing new stuff in/through it will be bug-prone.
> Though this time we can avert touching it, as the new ACPI methods
> deprecating old _DSM functionally is almost the same.
> How about next time? are we going to always packing new methods logic
> in NCAL()?
> My point is that we should implement new methods as itself, of course,
> as a general programming rule, we can/should abstract common routines,
> but not packing them in one large function.
> > 
> >   
> > >  include/hw/mem/nvdimm.h |   6 +
> > >  2 files changed, 338 insertions(+), 92 deletions(-)
> > > 
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index 59b42afcf1..50ee85866b 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > > *state, GArray *table_offsets,
> > >  
> > >  #define NVDIMM_DSM_MEMORY_SIZE      4096
> > >  
> > > -struct NvdimmDsmIn {
> > > +struct NvdimmMthdIn {
> > >      uint32_t handle;
> > > +    uint32_t method;
> > > +    uint8_t  args[4088];
> > > +} QEMU_PACKED;
> > > +typedef struct NvdimmMthdIn NvdimmMthdIn;
> > > +struct NvdimmDsmIn {
> > >      uint32_t revision;
> > >      uint32_t function;
> > >      /* the remaining size in the page is used by arg3. */
> > >      union {
> > > -        uint8_t arg3[4084];
> > > +        uint8_t arg3[4080];
> > >      };
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmDsmIn NvdimmDsmIn;
> > > -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
> > > +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmDsmOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncGetLabelDataOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncReadFITIn {
> > >      uint32_t offset; /* the offset into FIT buffer. */
> > >  } QEMU_PACKED;
> > >  typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> > >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> > > -                  offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +                  offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn, args) >
> > > +                  NVDIMM_DSM_MEMORY_SIZE);
> > >  
> > >  struct NvdimmFuncReadFITOut {
> > >      /* the size of buffer filled by QEMU. */
> > > @@ -636,7 +644,8 @@ static uint32_t
> > > nvdimm_get_max_xfer_label_size(void)
> > >       * the max data ACPI can write one time which is transferred
> > > by
> > >       * 'Set Namespace Label Data' function.
> > >       */
> > > -    max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3) -
> > > +    max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn, args)
> > > -
> > > +                   offsetof(NvdimmDsmIn, arg3) -
> > >                     sizeof(NvdimmFuncSetLabelDataIn);
> > >  
> > >      return MIN(max_get_size, max_set_size);
> > > @@ -697,16 +706,15 @@ static uint32_t
> > > nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
> > >  /*
> > >   * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
> > >   */
> > > -static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > > NvdimmDsmIn *in,
> > > -                                      hwaddr dsm_mem_addr)
> > > +static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
> > > +                                    NvdimmFuncGetLabelDataIn
> > > *get_label_data,
> > > +                                    hwaddr dsm_mem_addr)
> > >  {
> > >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > > -    NvdimmFuncGetLabelDataIn *get_label_data;
> > >      NvdimmFuncGetLabelDataOut *get_label_data_out;
> > >      uint32_t status;
> > >      int size;
> > >  
> > > -    get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> > >      get_label_data->offset = le32_to_cpu(get_label_data->offset);
> > >      get_label_data->length = le32_to_cpu(get_label_data->length);
> > >  
> > > @@ -737,15 +745,13 @@ static void
> > > nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> > >  /*
> > >   * DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index 6).
> > >   */
> > > -static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > > NvdimmDsmIn *in,
> > > +static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
> > > +                                      NvdimmFuncSetLabelDataIn
> > > *set_label_data,
> > >                                        hwaddr dsm_mem_addr)
> > >  {
> > >      NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
> > > -    NvdimmFuncSetLabelDataIn *set_label_data;
> > >      uint32_t status;
> > >  
> > > -    set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
> > > -
> > >      set_label_data->offset = le32_to_cpu(set_label_data->offset);
> > >      set_label_data->length = le32_to_cpu(set_label_data->length);
> > >  
> > > @@ -760,19 +766,21 @@ static void
> > > nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> > >      }
> > >  
> > >      assert(offsetof(NvdimmDsmIn, arg3) + sizeof(*set_label_data) +
> > > -                    set_label_data->length <=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +           set_label_data->length <= NVDIMM_DSM_MEMORY_SIZE -
> > > +           offsetof(NvdimmMthdIn, args));
> > >  
> > >      nvc->write_label_data(nvdimm, set_label_data->in_buf,
> > >                            set_label_data->length, set_label_data-  
> > > >offset);  
> > >      nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS,
> > > dsm_mem_addr);
> > >  }
> > >  
> > > -static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr
> > > dsm_mem_addr)
> > > +static void nvdimm_dsm_device(uint32_t nv_handle, NvdimmDsmIn
> > > *dsm_in,
> > > +                                    hwaddr dsm_mem_addr)
> > >  {
> > > -    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in-  
> > > >handle);  
> > > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > >  
> > >      /* See the comments in nvdimm_dsm_root(). */
> > > -    if (!in->function) {
> > > +    if (!dsm_in->function) {
> > >          uint32_t supported_func = 0;
> > >  
> > >          if (nvdimm && nvdimm->label_size) {
> > > @@ -794,7 +802,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in,
> > > hwaddr dsm_mem_addr)
> > >      }
> > >  
> > >      /* Encode DSM function according to DSM Spec Rev1. */
> > > -    switch (in->function) {
> > > +    switch (dsm_in->function) {
> > >      case 4 /* Get Namespace Label Size */:
> > >          if (nvdimm->label_size) {
> > >              nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > > @@ -803,13 +811,17 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > > *in, hwaddr dsm_mem_addr)
> > >          break;
> > >      case 5 /* Get Namespace Label Data */:
> > >          if (nvdimm->label_size) {
> > > -            nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
> > > +            nvdimm_dsm_get_label_data(nvdimm,
> > > +                                      (NvdimmFuncGetLabelDataIn
> > > *)dsm_in->arg3,
> > > +                                      dsm_mem_addr);
> > >              return;
> > >          }
> > >          break;
> > >      case 0x6 /* Set Namespace Label Data */:
> > >          if (nvdimm->label_size) {
> > > -            nvdimm_dsm_set_label_data(nvdimm, in, dsm_mem_addr);
> > > +            nvdimm_dsm_set_label_data(nvdimm,
> > > +                        (NvdimmFuncSetLabelDataIn *)dsm_in->arg3,
> > > +                        dsm_mem_addr);
> > >              return;
> > >          }
> > >          break;
> > > @@ -819,67 +831,128 @@ static void nvdimm_dsm_device(NvdimmDsmIn
> > > *in, hwaddr dsm_mem_addr)
> > >  }
> > >  
> > >  static uint64_t
> > > -nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> > > +nvdimm_method_read(void *opaque, hwaddr addr, unsigned size)
> > >  {
> > > -    nvdimm_debug("BUG: we never read _DSM IO Port.\n");
> > > +    nvdimm_debug("BUG: we never read NVDIMM Method IO Port.\n");
> > >      return 0;
> > >  }
> > >  
> > >  static void
> > > -nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned
> > > size)
> > > +nvdimm_dsm_handle(void *opaque, NvdimmMthdIn *method_in, hwaddr
> > > dsm_mem_addr)
> > >  {
> > >      NVDIMMState *state = opaque;
> > > -    NvdimmDsmIn *in;
> > > -    hwaddr dsm_mem_addr = val;
> > > +    NvdimmDsmIn *dsm_in = (NvdimmDsmIn *)method_in->args;
> > >  
> > >      nvdimm_debug("dsm memory address 0x%" HWADDR_PRIx ".\n",
> > > dsm_mem_addr);
> > >  
> > > -    /*
> > > -     * The DSM memory is mapped to guest address space so an evil
> > > guest
> > > -     * can change its content while we are doing DSM emulation.
> > > Avoid
> > > -     * this by copying DSM memory to QEMU local memory.
> > > -     */
> > > -    in = g_new(NvdimmDsmIn, 1);
> > > -    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> > > -
> > > -    in->revision = le32_to_cpu(in->revision);
> > > -    in->function = le32_to_cpu(in->function);
> > > -    in->handle = le32_to_cpu(in->handle);
> > > -
> > > -    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > > in->revision,
> > > -                 in->handle, in->function);
> > > +    dsm_in->revision = le32_to_cpu(dsm_in->revision);
> > > +    dsm_in->function = le32_to_cpu(dsm_in->function);
> > >  
> > > +    nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > > +                 dsm_in->revision, method_in->handle, dsm_in-  
> > > >function);  
> > >      /*
> > >       * Current NVDIMM _DSM Spec supports Rev1 and Rev2
> > >       * Intel® OptanePersistent Memory Module DSM Interface,
> > > Revision 2.0
> > >       */
> > > -    if (in->revision != 0x1 && in->revision != 0x2) {
> > > +    if (dsm_in->revision != 0x1 && dsm_in->revision != 0x2) {
> > >          nvdimm_debug("Revision 0x%x is not supported, expect 0x1
> > > or 0x2.\n",
> > > -                     in->revision);
> > > +                     dsm_in->revision);
> > >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > > dsm_mem_addr);
> > > -        goto exit;
> > > +        return;
> > >      }
> > >  
> > > -    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > > -        nvdimm_dsm_handle_reserved_root_method(state, in,
> > > dsm_mem_addr);
> > > -        goto exit;
> > > +    if (method_in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> > > +        nvdimm_dsm_handle_reserved_root_method(state, dsm_in,
> > > dsm_mem_addr);
> > > +        return;
> > >      }
> > >  
> > >       /* Handle 0 is reserved for NVDIMM Root Device. */
> > > -    if (!in->handle) {
> > > -        nvdimm_dsm_root(in, dsm_mem_addr);
> > > -        goto exit;
> > > +    if (!method_in->handle) {
> > > +        nvdimm_dsm_root(dsm_in, dsm_mem_addr);
> > > +        return;
> > >      }
> > >  
> > > -    nvdimm_dsm_device(in, dsm_mem_addr);
> > > +    nvdimm_dsm_device(method_in->handle, dsm_in, dsm_mem_addr);
> > > +}
> > >  
> > > -exit:
> > > -    g_free(in);
> > > +static void nvdimm_lsi_handle(uint32_t nv_handle, hwaddr
> > > dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> > > +    }
> > > +
> > > +    return;
> > > +}
> > > +
> > > +static void nvdimm_lsr_handle(uint32_t nv_handle,
> > > +                                    void *data,
> > > +                                    hwaddr dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > > +    NvdimmFuncGetLabelDataIn *get_label_data = data;
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_get_label_data(nvdimm, get_label_data,
> > > dsm_mem_addr);
> > > +    }
> > > +    return;
> > > +}
> > > +
> > > +static void nvdimm_lsw_handle(uint32_t nv_handle,
> > > +                                    void *data,
> > > +                                    hwaddr dsm_mem_addr)
> > > +{
> > > +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(nv_handle);
> > > +    NvdimmFuncSetLabelDataIn *set_label_data = data;
> > > +
> > > +    if (nvdimm->label_size) {
> > > +        nvdimm_dsm_set_label_data(nvdimm, set_label_data,
> > > dsm_mem_addr);
> > > +    }
> > > +    return;
> > > +}
> > > +
> > > +static void
> > > +nvdimm_method_write(void *opaque, hwaddr addr, uint64_t val,
> > > unsigned size)
> > > +{
> > > +    NvdimmMthdIn *method_in;
> > > +    hwaddr dsm_mem_addr = val;
> > > +
> > > +    /*
> > > +     * The DSM memory is mapped to guest address space so an evil
> > > guest
> > > +     * can change its content while we are doing DSM emulation.
> > > Avoid
> > > +     * this by copying DSM memory to QEMU local memory.
> > > +     */
> > > +    method_in = g_new(NvdimmMthdIn, 1);
> > > +    cpu_physical_memory_read(dsm_mem_addr, method_in,
> > > sizeof(*method_in));
> > > +
> > > +    method_in->handle = le32_to_cpu(method_in->handle);
> > > +    method_in->method = le32_to_cpu(method_in->method);
> > > +
> > > +    switch (method_in->method) {
> > > +    case NVDIMM_METHOD_DSM:
> > > +        nvdimm_dsm_handle(opaque, method_in, dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSI:
> > > +        nvdimm_lsi_handle(method_in->handle, dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSR:
> > > +        nvdimm_lsr_handle(method_in->handle, method_in->args,
> > > dsm_mem_addr);
> > > +        break;
> > > +    case NVDIMM_METHOD_LSW:
> > > +        nvdimm_lsw_handle(method_in->handle, method_in->args,
> > > dsm_mem_addr);
> > > +        break;
> > > +    default:
> > > +        nvdimm_debug("%s: Unkown method 0x%x\n", __func__,
> > > method_in->method);
> > > +        break;
> > > +    }
> > > +
> > > +    g_free(method_in);
> > >  }
> > >  
> > > -static const MemoryRegionOps nvdimm_dsm_ops = {
> > > -    .read = nvdimm_dsm_read,
> > > -    .write = nvdimm_dsm_write,
> > > +static const MemoryRegionOps nvdimm_method_ops = {
> > > +    .read = nvdimm_method_read,
> > > +    .write = nvdimm_method_write,
> > >      .endianness = DEVICE_LITTLE_ENDIAN,
> > >      .valid = {
> > >          .min_access_size = 4,
> > > @@ -899,12 +972,12 @@ void nvdimm_init_acpi_state(NVDIMMState
> > > *state, MemoryRegion *io,
> > >                              FWCfgState *fw_cfg, Object *owner)
> > >  {
> > >      state->dsm_io = dsm_io;
> > > -    memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops,
> > > state,
> > > +    memory_region_init_io(&state->io_mr, owner,
> > > &nvdimm_method_ops, state,
> > >                            "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));
> > > +    acpi_data_push(state->dsm_mem, sizeof(NvdimmMthdIn));
> > >      fw_cfg_add_file(fw_cfg, NVDIMM_DSM_MEM_FILE, state->dsm_mem-  
> > > >data,  
> > >                      state->dsm_mem->len);
> > >  
> > > @@ -918,13 +991,22 @@ void nvdimm_init_acpi_state(NVDIMMState
> > > *state, MemoryRegion *io,
> > >  #define NVDIMM_DSM_IOPORT       "NPIO"
> > >  
> > >  #define NVDIMM_DSM_NOTIFY       "NTFI"
> > > +#define NVDIMM_DSM_METHOD       "MTHD"
> > >  #define NVDIMM_DSM_HANDLE       "HDLE"
> > >  #define NVDIMM_DSM_REVISION     "REVS"
> > >  #define NVDIMM_DSM_FUNCTION     "FUNC"
> > >  #define NVDIMM_DSM_ARG3         "FARG"
> > >  
> > > -#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
> > > -#define NVDIMM_DSM_OUT_BUF      "ODAT"
> > > +#define NVDIMM_DSM_OFFSET       "OFST"
> > > +#define NVDIMM_DSM_TRANS_LEN    "TRSL"
> > > +#define NVDIMM_DSM_IN_BUFF      "IDAT"
> > > +
> > > +#define NVDIMM_DSM_OUT_BUF_SIZE     "RLEN"
> > > +#define NVDIMM_DSM_OUT_BUF          "ODAT"
> > > +#define NVDIMM_DSM_OUT_STATUS       "STUS"
> > > +#define NVDIMM_DSM_OUT_LSA_SIZE     "SIZE"
> > > +#define NVDIMM_DSM_OUT_MAX_TRANS    "MAXT"
> > > +
> > >  
> > >  #define NVDIMM_DSM_RFIT_STATUS  "RSTA"
> > >  
> > > @@ -938,7 +1020,6 @@ static void nvdimm_build_common_dsm(Aml *dev,
> > >      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > > *dsm_out_buf_size;
> > >      Aml *whilectx, *offset;
> > >      uint8_t byte_list[1];
> > > -    AmlRegionSpace rs;
> > >  
> > >      method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
> > >      uuid = aml_arg(0);
> > > @@ -949,37 +1030,15 @@ 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, 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)));
> > > -
> > > -    /*
> > > -     * DSM notifier:
> > > -     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > > notify QEMU to
> > > -     *                    emulate the access.
> > > -     *
> > > -     * It is the IO port so that accessing them will cause VM-
> > > exit, the
> > > -     * control will be transferred to QEMU.
> > > -     */
> > > -    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > -                      AML_PRESERVE);
> > > -    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > > -               nvdimm_state->dsm_io.bit_width));
> > > -    aml_append(method, field);
> > > +               AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmMthdIn)));
> > >  
> > >      /*
> > >       * DSM input:
> > >       * NVDIMM_DSM_HANDLE: store device's handle, it's zero if the
> > > _DSM call
> > >       *                    happens on NVDIMM Root Device.
> > > +     * NVDIMM_DSM_METHOD: ACPI method indicator, to distinguish
> > > _DSM and
> > > +     *                    other ACPI methods.
> > >       * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
> > >       * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
> > >       * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a
> > > Package
> > > @@ -991,13 +1050,16 @@ static void nvdimm_build_common_dsm(Aml
> > > *dev,
> > >      field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > >                        AML_PRESERVE);
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > -               sizeof(typeof_field(NvdimmDsmIn, handle)) *
> > > BITS_PER_BYTE));
> > > +               sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +    aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +               sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_REVISION,
> > >                 sizeof(typeof_field(NvdimmDsmIn, revision)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_FUNCTION,
> > >                 sizeof(typeof_field(NvdimmDsmIn, function)) *
> > > BITS_PER_BYTE));
> > >      aml_append(field, aml_named_field(NVDIMM_DSM_ARG3,
> > > -         (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) *
> > > BITS_PER_BYTE));
> > > +         (sizeof(NvdimmMthdIn) - offsetof(NvdimmMthdIn, args) -
> > > +          offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
> > >      aml_append(method, field);
> > >  
> > >      /*
> > > @@ -1065,6 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
> > >       * it reserves 0 for root device and is the handle for NVDIMM
> > > devices.
> > >       * See the comments in nvdimm_slot_to_handle().
> > >       */
> > > +    aml_append(method, aml_store(aml_int(0),
> > > aml_name(NVDIMM_DSM_METHOD)));
> > >      aml_append(method, aml_store(handle,
> > > aml_name(NVDIMM_DSM_HANDLE)));
> > >      aml_append(method, aml_store(aml_arg(1),
> > > aml_name(NVDIMM_DSM_REVISION)));
> > >      aml_append(method, aml_store(function,
> > > aml_name(NVDIMM_DSM_FUNCTION)));
> > > @@ -1250,6 +1313,7 @@ static void nvdimm_build_fit(Aml *dev)
> > >  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t
> > > ram_slots)
> > >  {
> > >      uint32_t slot;
> > > +    Aml *method, *pkg, *field;
> > >  
> > >      for (slot = 0; slot < ram_slots; slot++) {
> > >          uint32_t handle = nvdimm_slot_to_handle(slot);
> > > @@ -1266,6 +1330,155 @@ static void nvdimm_build_nvdimm_devices(Aml
> > > *root_dev, uint32_t ram_slots)
> > >           * table NFIT or _FIT.
> > >           */
> > >          aml_append(nvdimm_dev, aml_name_decl("_ADR",
> > > aml_int(handle)));
> > > +        aml_append(nvdimm_dev,
> > > aml_operation_region(NVDIMM_DSM_MEMORY,
> > > +                   AML_SYSTEM_MEMORY,
> > > aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                   sizeof(NvdimmMthdIn)));
> > > +
> > > +        /* ACPI 6.4: 6.5.10 NVDIMM Label Methods, _LS{I,R,W} */
> > > +
> > > +        /* Begin of _LSI Block */
> > > +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> > > +        /* _LSI Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSI Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_LSA_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > > label_size)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field,
> > > aml_named_field(NVDIMM_DSM_OUT_MAX_TRANS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelSizeOut,
> > > max_xfer)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > +                                      aml_name(NVDIMM_DSM_HANDLE))
> > > );
> > > +        aml_append(method, aml_store(aml_int(0x100),
> > > +                                      aml_name(NVDIMM_DSM_METHOD))
> > > );
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > > );
> > > +
> > > +        pkg = aml_package(3);
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_LSA_SIZE));
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_MAX_TRANS));
> > > +
> > > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > > +
> > > +        aml_append(method, aml_return(aml_name("RPKG")));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSI Block */
> > > +
> > > +
> > > +        /* Begin of _LSR Block */
> > > +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> > > +
> > > +        /* _LSR Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > > offset)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataIn,
> > > length)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSR Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmFuncGetLabelDataOut,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF,
> > > +                   (NVDIMM_DSM_MEMORY_SIZE -
> > > +                    offsetof(NvdimmFuncGetLabelDataOut, out_buf))
> > > *
> > > +                    BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > +                                      aml_name(NVDIMM_DSM_HANDLE))
> > > );
> > > +        aml_append(method, aml_store(aml_int(0x101),
> > > +                                      aml_name(NVDIMM_DSM_METHOD))
> > > );
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name(NVDIMM_DSM_OFFSET)));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > +                                      aml_name(NVDIMM_DSM_TRANS_LE
> > > N)));
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > > );
> > > +
> > > +        aml_append(method, aml_store(aml_shiftleft(aml_arg(1),
> > > aml_int(3)),
> > > +                                         aml_local(1)));
> > > +        aml_append(method,
> > > aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> > > +                   aml_int(0), aml_local(1), "OBUF"));
> > > +
> > > +        pkg = aml_package(2);
> > > +        aml_append(pkg, aml_name(NVDIMM_DSM_OUT_STATUS));
> > > +        aml_append(pkg, aml_name("OBUF"));
> > > +        aml_append(method, aml_name_decl("RPKG", pkg));
> > > +
> > > +        aml_append(method, aml_return(aml_name("RPKG")));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSR Block */
> > > +
> > > +        /* Begin of _LSW Block */
> > > +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> > > +        /* _LSW Input field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, handle)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_METHOD,
> > > +                   sizeof(typeof_field(NvdimmMthdIn, method)) *
> > > BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OFFSET,
> > > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > > offset)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_TRANS_LEN,
> > > +                   sizeof(typeof_field(NvdimmFuncSetLabelDataIn,
> > > length)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_IN_BUFF,
> > > 32640));
> > > +        aml_append(method, field);
> > > +
> > > +        /* _LSW Output field */
> > > +        field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                          AML_PRESERVE);
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
> > > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut,
> > > len)) *
> > > +                   BITS_PER_BYTE));
> > > +        aml_append(field, aml_named_field(NVDIMM_DSM_OUT_STATUS,
> > > +                   sizeof(typeof_field(NvdimmDsmFuncNoPayloadOut,
> > > +                   func_ret_status)) * BITS_PER_BYTE));
> > > +        aml_append(method, field);
> > > +
> > > +        aml_append(method, aml_store(aml_int(handle),
> > > aml_name(NVDIMM_DSM_HANDLE)));
> > > +        aml_append(method, aml_store(aml_int(0x102),
> > > aml_name(NVDIMM_DSM_METHOD)));
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name(NVDIMM_DSM_OFFSET)));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > aml_name(NVDIMM_DSM_TRANS_LEN)));
> > > +        aml_append(method, aml_store(aml_arg(2),
> > > aml_name(NVDIMM_DSM_IN_BUFF)));
> > > +        aml_append(method,
> > > aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR),
> > > +                                      aml_name(NVDIMM_DSM_NOTIFY))
> > > );
> > > +
> > > +        aml_append(method,
> > > aml_return(aml_name(NVDIMM_DSM_OUT_STATUS)));
> > > +        aml_append(nvdimm_dev, method); /* End of _LSW Block */
> > >  
> > >          nvdimm_build_device_dsm(nvdimm_dev, handle);
> > >          aml_append(root_dev, nvdimm_dev);
> > > @@ -1278,7 +1491,8 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >                                uint32_t ram_slots, const char
> > > *oem_id)
> > >  {
> > >      int mem_addr_offset;
> > > -    Aml *ssdt, *sb_scope, *dev;
> > > +    Aml *ssdt, *sb_scope, *dev, *field;
> > > +    AmlRegionSpace rs;
> > >      AcpiTable table = { .sig = "SSDT", .rev = 1,
> > >                          .oem_id = oem_id, .oem_table_id = "NVDIMM"
> > > };
> > >  
> > > @@ -1286,6 +1500,9 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >  
> > >      acpi_table_begin(&table, table_data);
> > >      ssdt = init_aml_allocator();
> > > +
> > > +    mem_addr_offset = build_append_named_dword(table_data,
> > > +                                               NVDIMM_ACPI_MEM_ADD
> > > R);
> > >      sb_scope = aml_scope("\\_SB");
> > >  
> > >      dev = aml_device("NVDR");
> > > @@ -1303,6 +1520,31 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >       */
> > >      aml_append(dev, aml_name_decl("_HID",
> > > aml_string("ACPI0012")));
> > >  
> > > +    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(dev, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
> > > +               aml_int(nvdimm_state->dsm_io.address),
> > > +               nvdimm_state->dsm_io.bit_width >> 3));
> > > +
> > > +    /*
> > > +     * DSM notifier:
> > > +     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and
> > > notify QEMU to
> > > +     *                    emulate the access.
> > > +     *
> > > +     * It is the IO port so that accessing them will cause VM-
> > > exit, the
> > > +     * control will be transferred to QEMU.
> > > +     */
> > > +    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC,
> > > AML_NOLOCK,
> > > +                      AML_PRESERVE);
> > > +    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
> > > +               nvdimm_state->dsm_io.bit_width));
> > > +    aml_append(dev, field);
> > > +
> > >      nvdimm_build_common_dsm(dev, nvdimm_state);
> > >  
> > >      /* 0 is reserved for root device. */
> > > @@ -1316,12 +1558,10 @@ static void nvdimm_build_ssdt(GArray
> > > *table_offsets, GArray *table_data,
> > >  
> > >      /* copy AML table into ACPI tables blob and patch header there
> > > */
> > >      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf-  
> > > >len);  
> > > -    mem_addr_offset = build_append_named_dword(table_data,
> > > -                                               NVDIMM_ACPI_MEM_ADD
> > > R);
> > >  
> > >      bios_linker_loader_alloc(linker,
> > >                               NVDIMM_DSM_MEM_FILE, nvdimm_state-  
> > > >dsm_mem,  
> > > -                             sizeof(NvdimmDsmIn), false /* high
> > > memory */);
> > > +                             sizeof(NvdimmMthdIn), false /* high
> > > memory */);
> > >      bios_linker_loader_add_pointer(linker,
> > >          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
> > >          NVDIMM_DSM_MEM_FILE, 0);
> > > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> > > index cf8f59be44..0206b6125b 100644
> > > --- a/include/hw/mem/nvdimm.h
> > > +++ b/include/hw/mem/nvdimm.h
> > > @@ -37,6 +37,12 @@
> > >          }                                                     \
> > >      } while (0)
> > >  
> > > +/* NVDIMM ACPI Methods */
> > > +#define NVDIMM_METHOD_DSM   0
> > > +#define NVDIMM_METHOD_LSI   0x100
> > > +#define NVDIMM_METHOD_LSR   0x101
> > > +#define NVDIMM_METHOD_LSW   0x102
> > > +
> > >  /*
> > >   * The minimum label data size is required by NVDIMM Namespace
> > >   * specification, see the chapter 2 Namespaces:  
> > 
> >   
>

On Thu, 2022-07-21 at 10:58 +0200, Igor Mammedov wrote:
[...]
Thanks Igor for review.
> > > The patch it is too intrusive and my hunch is that it breaks
> > > ABI and needs a bunch of compat knobs to work properly and
> > > that I'd like to avoid unless there is not other way around
> > > the problem.  
> > 
> > Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> > and the compat knobs refers to related functions' input/output
> > params?
> 
> ABI are structures that guest and QEMU pass through information
> between each other. And knobs in this case would be compat
> variable[s]
> to keep old behavior in place for old machine types.

My humble opinion:
The changes of the compat variable(s) here don't break the ABI, the ABI
between guest and host/qemu is the ACPI spec which we don't change and
fully conform to it; actually we're implementing it.
e.g. with these patches, old guest can boot up with no difference nor
changes.
> 
> > My thoughts is that eventually, sooner or later, more ACPI methods
> > will
> > be implemented per request, although now we can play the trick of
> > wrapper new methods over the pipe of old _DSM implementation.
> > Though this changes a little on existing struct NvdimmDsmIn {}, it
> > paves the way for the future; and actually the change is more an
> > extension or generalization, not fundamentally changes the
> > framework.
> > 
> > In short, my point is the change/generalization/extension will be
> > inevitable, even if not present.
> 
> Expanding ABI (interface between host&guest) has 2 drawbacks
>  * it exposes more attack surface of VMM to hostile guest
>    and rises chances that vulnerability would slip through
>    review/testing

This patch doesn't increase attach surface, I think.

>  * migration wise, QEMU has to support any ABI for years
>    and not only latest an greatest interface but also old
>    ones to keep guest started on older QEMU working across
>    migration, so any ABI change should be considered very
>    carefully before being implemented otherwise it all
>    quickly snowballs in unsupportable mess of compat
>    variables smeared across host/guest.
>    Reducing exposed ABI and constant need to expand it
>    was a reason why we have moved ACPI code from firmware
>    into QEMU, so we could describe hardware without costs
>    associated with of maintaining ABI.

Yeah, migration is the only broken thing. With this patch, guest ACPI
table changes, live guest migrate between new and old qemus will have
problem. But I think this is not the only example of such kind of
problem. How about other similar cases?

In fact, the point of our contention is around this 
https://www.qemu.org/docs/master/specs/acpi_nvdimm.html, whether or not
change the implementation protocol by this patch. The protocol was for
_DSM only. Unless we're not going to support any ACPI methods, it
should be updated, and the _LS{I,R,W} are ACPI methods, we can play the
trick in this special case, but definitely not next time.

I suggest to do it now, nevertheless, you maintainers make the final
decision.

> 
> There might be need to extend ABI eventually, but not in this case.
> 
> > > I was skeptical about this approach during v1 review and
> > > now I'm pretty much sure it's over-engineered and we can
> > > just repack data we receive from existing label _DSM functions
> > > to provide _LS{I,R,W} like it was suggested in v1.
> > > It will be much simpler and affect only AML side without
> > > complicating ABI and without any compat cruft and will work
> > > with ping-pong migration without any issues.  
> > 
> > Ostensibly it may looks simpler, actually not, I think. The AML
> > "common
> > pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> > function
> > logics there, packing new stuff in/through it will be bug-prone.
> > Though this time we can avert touching it, as the new ACPI methods
> > deprecating old _DSM functionally is almost the same.
> > How about next time? are we going to always packing new methods
> > logic
> > in NCAL()?
> > My point is that we should implement new methods as itself, of
> > course,
> > as a general programming rule, we can/should abstract common
> > routines,
> > but not packing them in one large function.
> > > 
> > >   
[...]

On Wed, 27 Jul 2022 13:22:34 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Thu, 2022-07-21 at 10:58 +0200, Igor Mammedov wrote:
> [...]
> Thanks Igor for review.
> > > > The patch it is too intrusive and my hunch is that it breaks
> > > > ABI and needs a bunch of compat knobs to work properly and
> > > > that I'd like to avoid unless there is not other way around
> > > > the problem.    
> > > 
> > > Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> > > and the compat knobs refers to related functions' input/output
> > > params?  
> > 
> > ABI are structures that guest and QEMU pass through information
> > between each other. And knobs in this case would be compat
> > variable[s]
> > to keep old behavior in place for old machine types.  
> 
> My humble opinion:
> The changes of the compat variable(s) here don't break the ABI, the ABI
> between guest and host/qemu is the ACPI spec which we don't change and
> fully conform to it; actually we're implementing it.
> e.g. with these patches, old guest can boot up with no difference nor
> changes.

it's not about booting but about migration.
boot on old QEMU and then migrate to one with your patches,
then make guest use _DSM again. You will see that migrated
guest still uses _old_ ACPI tables/AML and ABI in new QEMU
_must_ be compatible with that.

As for the patch, it's too big, and looking at it I wasn't
able to convince myself that it's correct.

 
> >   
> > > My thoughts is that eventually, sooner or later, more ACPI methods
> > > will
> > > be implemented per request, although now we can play the trick of
> > > wrapper new methods over the pipe of old _DSM implementation.
> > > Though this changes a little on existing struct NvdimmDsmIn {}, it
> > > paves the way for the future; and actually the change is more an
> > > extension or generalization, not fundamentally changes the
> > > framework.
> > > 
> > > In short, my point is the change/generalization/extension will be
> > > inevitable, even if not present.  
> > 
> > Expanding ABI (interface between host&guest) has 2 drawbacks
> >  * it exposes more attack surface of VMM to hostile guest
> >    and rises chances that vulnerability would slip through
> >    review/testing  
> 
> This patch doesn't increase attach surface, I think.
> 
> >  * migration wise, QEMU has to support any ABI for years
> >    and not only latest an greatest interface but also old
> >    ones to keep guest started on older QEMU working across
> >    migration, so any ABI change should be considered very
> >    carefully before being implemented otherwise it all
> >    quickly snowballs in unsupportable mess of compat
> >    variables smeared across host/guest.
> >    Reducing exposed ABI and constant need to expand it
> >    was a reason why we have moved ACPI code from firmware
> >    into QEMU, so we could describe hardware without costs
> >    associated with of maintaining ABI.  
> 
> Yeah, migration is the only broken thing. With this patch, guest ACPI
> table changes, live guest migrate between new and old qemus will have
> problem. But I think this is not the only example of such kind of
> problem. How about other similar cases?

Upstream policy for version-ed machine types (pc-*/q35-*/...),
forward migration _must_ work.
If you consider your device should e supported/usable downstream,
you also need take in account backward migration as well.


> In fact, the point of our contention is around this 
> https://www.qemu.org/docs/master/specs/acpi_nvdimm.html, whether or not
> change the implementation protocol by this patch. The protocol was for
> _DSM only. Unless we're not going to support any ACPI methods, it
> should be updated, and the _LS{I,R,W} are ACPI methods, we can play the
> trick in this special case, but definitely not next time.
> 
> I suggest to do it now, nevertheless, you maintainers make the final
> decision.

Not for this case (i.e. make patches minimal, touching only AML side
and reusing data that QEMU already provides via MMIO).

If ABI needs extending in future, that should be discussed separately
when there is actual need for it. 

> > 
> > There might be need to extend ABI eventually, but not in this case.
> >   
> > > > I was skeptical about this approach during v1 review and
> > > > now I'm pretty much sure it's over-engineered and we can
> > > > just repack data we receive from existing label _DSM functions
> > > > to provide _LS{I,R,W} like it was suggested in v1.
> > > > It will be much simpler and affect only AML side without
> > > > complicating ABI and without any compat cruft and will work
> > > > with ping-pong migration without any issues.    
> > > 
> > > Ostensibly it may looks simpler, actually not, I think. The AML
> > > "common
> > > pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> > > function
> > > logics there, packing new stuff in/through it will be bug-prone.
> > > Though this time we can avert touching it, as the new ACPI methods
> > > deprecating old _DSM functionally is almost the same.
> > > How about next time? are we going to always packing new methods
> > > logic
> > > in NCAL()?
> > > My point is that we should implement new methods as itself, of
> > > course,
> > > as a general programming rule, we can/should abstract common
> > > routines,
> > > but not packing them in one large function.  
> > > > 
> > > >     
> [...]
>