[v4,2/3] nvdimm acpi: introduce _FIT

Commit Message

Xiao Guangrong Nov. 3, 2016, 3:51 a.m. UTC

_FIT is required for hotplug support, guest will inquire the updated
device info from it if a hotplug event is received

As FIT buffer is not completely mapped into guest address space, so a
new function, Read FIT whose UUID is UUID
648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
is concatenated before _FIT return

Refer to docs/specs/acpi-nvdimm.txt for detailed design

Signed-off-by: Xiao Guangrong <[e-mail]>
---
 docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
 hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 271 insertions(+), 17 deletions(-)

Comments

Stefan Hajnoczi Nov. 3, 2016, 9:53 a.m. UTC | #1

On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:
> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */

Not worth changing but please make each logical change a separate patch
in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
constant renaming.  It's easier to review, bisect, and backport when
structured as separate patches.

Xiao Guangrong Nov. 3, 2016, 10:08 a.m. UTC | #2

On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:
>> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>>  }
>>
>> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
>> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
>> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
>
> Not worth changing but please make each logical change a separate patch
> in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
> constant renaming.  It's easier to review, bisect, and backport when
> structured as separate patches.
>

Yes, indeed. Thanks for your suggestion, will pay more attention. :P

Igor Mammedov Nov. 3, 2016, 11:58 a.m. UTC | #3

On Thu,  3 Nov 2016 11:51:29 +0800
Xiao Guangrong <[e-mail]> wrote:

> _FIT is required for hotplug support, guest will inquire the updated
> device info from it if a hotplug event is received
s/_FIT/_FIT method/

the same applies to subj. line

> 
> As FIT buffer is not completely mapped into guest address space, so a
> new function, Read FIT whose UUID is UUID
> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> is concatenated before _FIT return

Commit message hard to read/parse, it might be better if I'd use simple
short sentences.


> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> 
> Signed-off-by: Xiao Guangrong <[e-mail]>
> ---
>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 271 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> index 0fdd251..364e832 100644
> --- a/docs/specs/acpi_nvdimm.txt
> +++ b/docs/specs/acpi_nvdimm.txt
> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
>     NVDIMM Firmware Interface Table (NFIT).
>  
> -QEMU NVDIMM Implemention
> -========================
> +QEMU NVDIMM Implementation
> +==========================
>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>  for NVDIMM ACPI.
>  
> @@ -82,6 +82,16 @@ Memory:
>     ACPI writes _DSM Input Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>                  Root device.
> +
> +                The handle is completely QEMU internal thing, the values in
> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
[1, 0xFFFF] indicate nvdimm device
and move 0 to reserved section

> +                root device named NVDR), other values are reserved by other
s/by/for/

> +                purpose.
s/purpose/purposes/

> +                Current reserved handle:
s/Current reserved handle/Reserved handles/

> +                0x10000 is reserved for QEMU internal DSM function called on
> +                the root device.
description is too obscure, I wonder if it could be more specific


>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> @@ -127,6 +137,49 @@ _DSM process diagram:
>   | result from the page     |      |              |
>   +--------------------------+      +--------------+
>  
> - _FIT implementation
> - -------------------
> - TODO (will fill it when nvdimm hotplug is introduced)
> +QEMU internal use only _DSM function
> +------------------------------------
> +There is the function introduced by QEMU and only used by QEMU internal.
drop it

> +1) Read FIT
> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> +   function (private QEMU function)
not necessary, drop it. Maybe extend UUID description in
"Input parameters:" section


> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
s/Read_FIT function/_DSM method/

> +   QEMU in 1 page sized increments which are then concatenated and returned
> +   as _FIT method result.
> +
> +   Input parameters:
> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> +   Arg1 – Revision ID (set to 1)
> +   Arg2 - Function Index, 0x1
> +   Arg3 - A package containing a buffer whose layout is as follows:
> +
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> +   |          |        |        | read from                                 |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   Output:
> +   +----------+--------+--------+-------------------------------------------+
> +   |  Field   | Length | Offset |                 Description               |
> +   +----------+--------+--------+-------------------------------------------+
> +   |          |        |        | return status codes                       |
> +   |          |        |        | 0x100 - error caused by NFIT update while |
> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
> +   +----------+--------+--------+-------------------------------------------+
> +   | length   |   4    |   4    | The fit size                              |           
> +   +----------+-----------------+-------------------------------------------+
> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> +   |          |        |        | filed above                               |
> +   +----------+--------+--------+-------------------------------------------+
> +
> +   The FIT offset is maintained by the OSPM itself, current offset plus
> +   the length returned by the function is the next offset we should read.
there shouldn't be 'we' or anything personal in spec

> +   When all the FIT data has been read out, zero length is returned.
> +
> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> +   again).

PS:
 fix typos and fix spelling/grammatical errors you are adding in above text.


> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fee077..593ac0d 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>  
> +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) > 4096);
> +
> +struct NvdimmFuncReadFITOut {
> +    /* the size of buffer filled by QEMU. */
> +    uint32_t len;
> +    uint32_t func_ret_status; /* return status code. */
> +    uint32_t length; /* the length of fit. */
redundant as len field above already has it all.

just drop this and describe properly 'len' in spec section
i.e. len: length of entire returned data (including the header)

> +    uint8_t fit[0]; /* the FIT data. */
> +} QEMU_PACKED;
> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
> +
>  static void
>  nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
>  {
> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>  }
>  
> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
> +#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
> +
> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000
> +
> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> +                                     hwaddr dsm_mem_addr)
> +{
> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
> +    NvdimmFuncReadFITIn *read_fit;
> +    NvdimmFuncReadFITOut *read_fit_out;
> +    GArray *fit;
> +    uint32_t read_len = 0, func_ret_status, offset;
> +    int size;
> +
> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> +    offset = le32_to_cpu(read_fit->offset);
> +
> +    fit = fit_buf->fit;
> +
> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
> +                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> +
> +    /* It is the first time to read FIT. */
> +    if (!offset) {
> +        fit_buf->dirty = false;
> +    } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
> +        goto exit;
> +    }
> +
> +    if (offset > fit->len) {
> +        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
> +        goto exit;
> +    }
> +
> +    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
> +    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));
> +
> +exit:
> +    size = sizeof(NvdimmFuncReadFITOut) + read_len;
> +    read_fit_out = g_malloc(size);
> +
> +    read_fit_out->len = cpu_to_le32(size);
> +    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
> +    read_fit_out->length = cpu_to_le32(read_len);
> +    memcpy(read_fit_out->fit, fit->data + offset, read_len);
> +
> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
> +
> +    g_free(read_fit_out);
> +}
> +
> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> +                                     hwaddr dsm_mem_addr)
function name doesn't make any sense to me

> +{
> +    switch (in->function) {
> +    case 0x0:
> +        nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
> +        return;
> +    case 0x1 /* Read FIT */:
> +        nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
> +        return;
> +    }
> +
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
> +}
> +
>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>  {
>      /*
> @@ -563,7 +651,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
>  
>      nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
>  
> -    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
> +    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      label_size_out.label_size = cpu_to_le32(label_size);
>      label_size_out.max_xfer = cpu_to_le32(mxfer);
>  
> @@ -574,7 +662,7 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
>  static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>                                             uint32_t offset, uint32_t length)
>  {
> -    uint32_t ret = 3 /* Invalid Input Parameters */;
> +    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;
>  
>      if (offset + length < offset) {
>          nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
> @@ -594,7 +682,7 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
>          return ret;
>      }
>  
> -    return 0 /* Success */;
> +    return NVDIMM_DSM_RET_STATUS_SUCCESS;
>  }
>  
>  /*
> @@ -618,7 +706,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
>                                          get_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -628,7 +716,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      get_label_data_out = g_malloc(size);
>  
>      get_label_data_out->len = cpu_to_le32(size);
> -    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
> +    get_label_data_out->func_ret_status =
> +                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
>      nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
>                           get_label_data->length, get_label_data->offset);
>  
> @@ -656,7 +745,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
>                                          set_label_data->length);
> -    if (status != 0 /* Success */) {
> +    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
>          nvdimm_dsm_no_payload(status, dsm_mem_addr);
>          return;
>      }
> @@ -666,7 +755,7 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      nvc->write_label_data(nvdimm, set_label_data->in_buf,
>                            set_label_data->length, set_label_data->offset);
> -    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
>  }
>  
>  static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> @@ -717,7 +806,7 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>          break;
>      }
>  
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>  }
>  
>  static uint64_t
> @@ -730,6 +819,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
>  static void
>  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>  {
> +    AcpiNVDIMMState *state = opaque;
>      NvdimmDsmIn *in;
>      hwaddr dsm_mem_addr = val;
>  
> @@ -753,7 +843,12 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
>          nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
>                       in->revision, 0x1);
> -        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
> +        goto exit;
> +    }
> +
> +    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
> +        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
>          goto exit;
>      }
>  
> @@ -809,9 +904,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>  #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
>  #define NVDIMM_DSM_OUT_BUF      "ODAT"
>  
> +#define NVDIMM_DSM_RFIT_STATUS  "RSTA"
> +
> +#define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
> +
>  static void nvdimm_build_common_dsm(Aml *dev)
>  {
> -    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
> +    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
>      Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
>      Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
>      uint8_t byte_list[1];
> @@ -900,9 +999,15 @@ static void nvdimm_build_common_dsm(Aml *dev)
>                 /* UUID for NVDIMM Root Device */, expected_uuid));
>      aml_append(method, ifctx);
>      elsectx = aml_else();
> -    aml_append(elsectx, aml_store(
> +    ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
> +    aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
> +               /* UUID for QEMU internal use */), expected_uuid));
> +    aml_append(elsectx, ifctx);
> +    elsectx2 = aml_else();
> +    aml_append(elsectx2, aml_store(
>                 aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
>                 /* UUID for NVDIMM Devices */, expected_uuid));
> +    aml_append(elsectx, elsectx2);
>      aml_append(method, elsectx);
>  
>      uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
> @@ -919,7 +1024,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
>      aml_append(unsupport, ifctx);
>  
>      /* No function is supported yet. */
> -    byte_list[0] = 1 /* Not Supported */;
> +    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
>      aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
>      aml_append(method, unsupport);
>  
> @@ -982,6 +1087,101 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>      aml_append(dev, method);
>  }
>  
> +static void nvdimm_build_fit_method(Aml *dev)
> +{
> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
> +    Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
> +
> +    buf = aml_local(0);
> +    buf_size = aml_local(1);
> +    fit = aml_local(2);
> +
> +    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));
> +
> +    /* build helper function, RFIT. */
> +    method = aml_method("RFIT", 1, AML_SERIALIZED);
> +    aml_append(method, aml_name_decl("OFST", aml_int(0)));
> +
> +    /* prepare input package. */
> +    pkg = aml_package(1);
> +    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
> +    aml_append(pkg, aml_name("OFST"));
> +
> +    /* call Read_FIT function. */
> +    call_result = aml_call5(NVDIMM_COMMON_DSM,
> +                            aml_touuid(NVDIMM_QEMU_RSVD_UUID),
> +                            aml_int(1) /* Revision 1 */,
> +                            aml_int(0x1) /* Read FIT */,
> +                            pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
> +    aml_append(method, aml_store(call_result, buf));
> +
> +    /* handle _DSM result. */
> +    aml_append(method, aml_create_dword_field(buf,
> +               aml_int(0) /* offset at byte 0 */, "STAU"));
> +
> +    aml_append(method, aml_store(aml_name("STAU"),
> +                                 aml_name(NVDIMM_DSM_RFIT_STATUS)));
> +
> +     /* if something is wrong during _DSM. */
> +    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
> +    ifctx = aml_if(aml_lnot(ifcond));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +
> +    aml_append(method, aml_create_dword_field(buf,
> +               aml_int(4) /* offset at byte 4 */, "LENG"));
> +    aml_append(method, aml_store(aml_name("LENG"), buf_size));
> +    /* if we read the end of fit. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
> +    aml_append(method, ifctx);
> +
> +    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
> +                                 buf_size));
> +    aml_append(method, aml_create_field(buf,
> +                            aml_int(8 * BITS_PER_BYTE), /* offset at byte 4.*/
> +                            buf_size, "BUFF"));
> +    aml_append(method, aml_return(aml_name("BUFF")));
> +    aml_append(dev, method);
> +
> +    /* build _FIT. */
> +    method = aml_method("_FIT", 0, AML_SERIALIZED);
> +    offset = aml_local(3);
> +
> +    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
> +    aml_append(method, aml_store(aml_int(0), offset));
> +
> +    whilectx = aml_while(aml_int(1));
> +    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
> +    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
> +
> +    /*
> +     * if fit buffer was changed during RFIT, read from the beginning
> +     * again.
> +     */
> +    ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
> +                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
> +    aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
> +    aml_append(ifctx, aml_store(aml_int(0), offset));
> +    aml_append(whilectx, ifctx);
> +
> +    elsectx = aml_else();
> +
> +    /* finish fit read if no data is read out. */
> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
> +    aml_append(ifctx, aml_return(fit));
> +    aml_append(elsectx, ifctx);
> +
> +    /* update the offset. */
> +    aml_append(elsectx, aml_add(offset, buf_size, offset));
> +    /* append the data we read out to the fit buffer. */
> +    aml_append(elsectx, aml_concatenate(fit, buf, fit));
> +    aml_append(whilectx, elsectx);
> +    aml_append(method, whilectx);
> +
> +    aml_append(dev, method);
> +}
> +
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> @@ -1040,6 +1240,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
>  
>      /* 0 is reserved for root device. */
>      nvdimm_build_device_dsm(dev, 0);
> +    nvdimm_build_fit_method(dev);
>  
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
>

Xiao Guangrong Nov. 3, 2016, 12:21 p.m. UTC | #4

On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:29 +0800
> Xiao Guangrong <[e-mail]> wrote:
>
>> _FIT is required for hotplug support, guest will inquire the updated
>> device info from it if a hotplug event is received
> s/_FIT/_FIT method/
>
> the same applies to subj. line

Okay.

>
>>
>> As FIT buffer is not completely mapped into guest address space, so a
>> new function, Read FIT whose UUID is UUID
>> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
>> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
>> is concatenated before _FIT return
>
> Commit message hard to read/parse, it might be better if I'd use simple
> short sentences.

Okay, will change it to:

As FIT buffer is not completely mapped into guest address space, Read_FIT
method is introduced to read NFIT structures blob from QEMU, The buffer
is concatenated before _FIT return

>
>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
>>
>> Signed-off-by: Xiao Guangrong <[e-mail]>
>> ---
>>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
>>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 271 insertions(+), 17 deletions(-)
>>
>> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
>> index 0fdd251..364e832 100644
>> --- a/docs/specs/acpi_nvdimm.txt
>> +++ b/docs/specs/acpi_nvdimm.txt
>> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
>>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
>>     NVDIMM Firmware Interface Table (NFIT).
>>
>> -QEMU NVDIMM Implemention
>> -========================
>> +QEMU NVDIMM Implementation
>> +==========================
>>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
>>  for NVDIMM ACPI.
>>
>> @@ -82,6 +82,16 @@ Memory:
>>     ACPI writes _DSM Input Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
>>                  Root device.
>> +
>> +                The handle is completely QEMU internal thing, the values in
>> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
> [1, 0xFFFF] indicate nvdimm device
> and move 0 to reserved section

Okay.

>
>> +                root device named NVDR), other values are reserved by other
> s/by/for/

okay.

>
>> +                purpose.
> s/purpose/purposes/

okay.

>
>> +                Current reserved handle:
> s/Current reserved handle/Reserved handles/
>
>> +                0x10000 is reserved for QEMU internal DSM function called on
>> +                the root device.
> description is too obscure, I wonder if it could be more specific

I would like to make these reserved values more generic in order to
support more QEMU reserved _DSM methods in the further. So, i planed:
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
methods. Handle 0x10000 indicates the method is for the root device.
0x10001 ~ 0x1FFFF indicates the method is for the nvdimm devices.

As currently we do not have reserved method on nvdimm device, so i
only document 0x1000 for the root device.

>
>
>>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
>>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
>>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
>> @@ -127,6 +137,49 @@ _DSM process diagram:
>>   | result from the page     |      |              |
>>   +--------------------------+      +--------------+
>>
>> - _FIT implementation
>> - -------------------
>> - TODO (will fill it when nvdimm hotplug is introduced)
>> +QEMU internal use only _DSM function
>> +------------------------------------
>> +There is the function introduced by QEMU and only used by QEMU internal.
> drop it
>
>> +1) Read FIT
>> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
>> +   function (private QEMU function)
> not necessary, drop it. Maybe extend UUID description in
> "Input parameters:" section
>

okay.

>
>> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
> s/Read_FIT function/_DSM method/

okay.

>
>> +   QEMU in 1 page sized increments which are then concatenated and returned
>> +   as _FIT method result.
>> +
>> +   Input parameters:
>> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
>> +   Arg1 – Revision ID (set to 1)
>> +   Arg2 - Function Index, 0x1
>> +   Arg3 - A package containing a buffer whose layout is as follows:
>> +
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |  Field   | Length | Offset |                 Description               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
>> +   |          |        |        | read from                                 |
>> +   +----------+--------+--------+-------------------------------------------+
>> +
>> +   Output:
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |  Field   | Length | Offset |                 Description               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   |          |        |        | return status codes                       |
>> +   |          |        |        | 0x100 - error caused by NFIT update while |
>> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>> +   +----------+--------+--------+-------------------------------------------+
>> +   | length   |   4    |   4    | The fit size                              |
>> +   +----------+-----------------+-------------------------------------------+
>> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
>> +   |          |        |        | filed above                               |
>> +   +----------+--------+--------+-------------------------------------------+
>> +
>> +   The FIT offset is maintained by the OSPM itself, current offset plus
>> +   the length returned by the function is the next offset we should read.
> there shouldn't be 'we' or anything personal in spec

Okay, change it to OSPM. :)

>
>> +   When all the FIT data has been read out, zero length is returned.
>> +
>> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
>> +   again).
>
> PS:
>  fix typos and fix spelling/grammatical errors you are adding in above text.

Sorry for the poor English, will check it carefully.

>
>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 9fee077..593ac0d 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>>
>> +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) > 4096);
>> +
>> +struct NvdimmFuncReadFITOut {
>> +    /* the size of buffer filled by QEMU. */
>> +    uint32_t len;
>> +    uint32_t func_ret_status; /* return status code. */
>> +    uint32_t length; /* the length of fit. */
> redundant as len field above already has it all.
>
> just drop this and describe properly 'len' in spec section
> i.e. len: length of entire returned data (including the header)

Okay, i will change the spec like this:

    QEMU Writes Output Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)

And drop the length field in Read_Fit return buffer, doc
the fit buffer like this:

    +----------+--------+--------+-------------------------------------------+
    |  Field   | Length | Offset |                 Description               |
    +----------+--------+--------+-------------------------------------------+
    |          |        |        | return status codes                       |
    |          |        |        | 0x100 - error caused by NFIT update while |
    | status   |   4    |   0    | read by _FIT wasn't completed, other      |
    |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
    +----------+--------+--------+-------------------------------------------+
    | fit data | Varies |   8    | FIT data, The remaining size in the       |
    |          |        |        | returned buffer is used by FIT            |
    +----------+--------+--------+-------------------------------------------+



>> +}
>> +
>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>> +                                     hwaddr dsm_mem_addr)
> function name doesn't make any sense to me

As i explained above, handle 0x10000 indicates the reserved _DSM method is
called on the root device...

It makes sense now? :)

Igor Mammedov Nov. 3, 2016, 12:30 p.m. UTC | #5

On Thu, 3 Nov 2016 18:08:04 +0800
Xiao Guangrong <[e-mail]> wrote:

> On 11/03/2016 05:53 PM, Stefan Hajnoczi wrote:
> > On Thu, Nov 03, 2016 at 11:51:29AM +0800, Xiao Guangrong wrote:  
> >> @@ -504,6 +521,77 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
> >>      cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
> >>  }
> >>
> >> +#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
> >> +#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
> >> +#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */  
> >
> > Not worth changing but please make each logical change a separate patch
> > in the future.  This patch is cluttered with NVDIMM_DSM_RET_STATUS_
> > constant renaming.  It's easier to review, bisect, and backport when
> > structured as separate patches.
> >  
> 
> Yes, indeed. Thanks for your suggestion, will pay more attention. :P
just do renaming first as separate patch
and then hotplug patches on top

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Igor Mammedov Nov. 3, 2016, 1 p.m. UTC | #6

On Thu, 3 Nov 2016 20:21:05 +0800
Xiao Guangrong <[e-mail]> wrote:

> On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> > On Thu,  3 Nov 2016 11:51:29 +0800
> > Xiao Guangrong <[e-mail]> wrote:
> >  
> >> _FIT is required for hotplug support, guest will inquire the updated
> >> device info from it if a hotplug event is received  
> > s/_FIT/_FIT method/
> >
> > the same applies to subj. line  
> 
> Okay.
> 
> >  
> >>
> >> As FIT buffer is not completely mapped into guest address space, so a
> >> new function, Read FIT whose UUID is UUID
> >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> >> is concatenated before _FIT return  
> >
> > Commit message hard to read/parse, it might be better if I'd use simple
> > short sentences.  
> 
> Okay, will change it to:
> 
> As FIT buffer is not completely mapped into guest address space, Read_FIT
> method is introduced to read NFIT structures blob from QEMU, The buffer
> is concatenated before _FIT return
> 
> >
> >  
> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >>
> >> Signed-off-by: Xiao Guangrong <[e-mail]>
> >> ---
> >>  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
> >>  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 271 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> >> index 0fdd251..364e832 100644
> >> --- a/docs/specs/acpi_nvdimm.txt
> >> +++ b/docs/specs/acpi_nvdimm.txt
> >> @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
> >>     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
> >>     NVDIMM Firmware Interface Table (NFIT).
> >>
> >> -QEMU NVDIMM Implemention
> >> -========================
> >> +QEMU NVDIMM Implementation
> >> +==========================
> >>  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
> >>  for NVDIMM ACPI.
> >>
> >> @@ -82,6 +82,16 @@ Memory:
> >>     ACPI writes _DSM Input Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
> >>                  Root device.
> >> +
> >> +                The handle is completely QEMU internal thing, the values in
> >> +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm  
> > [1, 0xFFFF] indicate nvdimm device
> > and move 0 to reserved section  
> 
> Okay.
> 
> >  
> >> +                root device named NVDR), other values are reserved by other  
> > s/by/for/  
> 
> okay.
> 
> >  
> >> +                purpose.  
> > s/purpose/purposes/  
> 
> okay.
> 
> >  
> >> +                Current reserved handle:  
> > s/Current reserved handle/Reserved handles/
> >  
> >> +                0x10000 is reserved for QEMU internal DSM function called on
> >> +                the root device.  
> > description is too obscure, I wonder if it could be more specific  
> 
> I would like to make these reserved values more generic in order to
> support more QEMU reserved _DSM methods in the further. So, i planed:
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
> methods. Handle 0x10000 indicates the method is for the root device.
> 0x10001 ~ 0x1FFFF indicates the method is for the nvdimm devices.
> 
> As currently we do not have reserved method on nvdimm device, so i
> only document 0x1000 for the root device.
> 
> >
> >  
> >>     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
> >>     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
> >>     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> >> @@ -127,6 +137,49 @@ _DSM process diagram:
> >>   | result from the page     |      |              |
> >>   +--------------------------+      +--------------+
> >>
> >> - _FIT implementation
> >> - -------------------
> >> - TODO (will fill it when nvdimm hotplug is introduced)
> >> +QEMU internal use only _DSM function
> >> +------------------------------------
> >> +There is the function introduced by QEMU and only used by QEMU internal.  
> > drop it
> >  
> >> +1) Read FIT
> >> +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> >> +   function (private QEMU function)  
> > not necessary, drop it. Maybe extend UUID description in
> > "Input parameters:" section
> >  
> 
> okay.
> 
> >  
> >> +   _FIT method uses Read_FIT function to fetch NFIT structures blob from  
> > s/Read_FIT function/_DSM method/  
> 
> okay.
> 
> >  
> >> +   QEMU in 1 page sized increments which are then concatenated and returned
> >> +   as _FIT method result.
> >> +
> >> +   Input parameters:
> >> +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> >> +   Arg1 – Revision ID (set to 1)
> >> +   Arg2 - Function Index, 0x1
> >> +   Arg3 - A package containing a buffer whose layout is as follows:
> >> +
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |  Field   | Length | Offset |                 Description               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> >> +   |          |        |        | read from                                 |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +
> >> +   Output:
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |  Field   | Length | Offset |                 Description               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   |          |        |        | return status codes                       |
> >> +   |          |        |        | 0x100 - error caused by NFIT update while |
> >> +   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
> >> +   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +   | length   |   4    |   4    | The fit size                              |
> >> +   +----------+-----------------+-------------------------------------------+
> >> +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> >> +   |          |        |        | filed above                               |
> >> +   +----------+--------+--------+-------------------------------------------+
> >> +
> >> +   The FIT offset is maintained by the OSPM itself, current offset plus
> >> +   the length returned by the function is the next offset we should read.  
> > there shouldn't be 'we' or anything personal in spec  
> 
> Okay, change it to OSPM. :)
> 
> >  
> >> +   When all the FIT data has been read out, zero length is returned.
> >> +
> >> +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> >> +   again).  
> >
> > PS:
> >  fix typos and fix spelling/grammatical errors you are adding in above text.  
> 
> Sorry for the poor English, will check it carefully.
> 
> >
> >  
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 9fee077..593ac0d 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> >>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> >>                    offsetof(NvdimmDsmIn, arg3) > 4096);
> >>
> >> +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) > 4096);
> >> +
> >> +struct NvdimmFuncReadFITOut {
> >> +    /* the size of buffer filled by QEMU. */
> >> +    uint32_t len;
> >> +    uint32_t func_ret_status; /* return status code. */
> >> +    uint32_t length; /* the length of fit. */  
> > redundant as len field above already has it all.
> >
> > just drop this and describe properly 'len' in spec section
> > i.e. len: length of entire returned data (including the header)  
> 
> Okay, i will change the spec like this:
> 
>     QEMU Writes Output Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)
> 
> And drop the length field in Read_Fit return buffer, doc
> the fit buffer like this:
> 
>     +----------+--------+--------+-------------------------------------------+
>     |  Field   | Length | Offset |                 Description               |
>     +----------+--------+--------+-------------------------------------------+
you need to add length here, otherwise this table is not correct


>     |          |        |        | return status codes                       |
>     |          |        |        | 0x100 - error caused by NFIT update while |
>     | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>     +----------+--------+--------+-------------------------------------------+
>     | fit data | Varies |   8    | FIT data, The remaining size in the       |
>     |          |        |        | returned buffer is used by FIT            |
>     +----------+--------+--------+-------------------------------------------+
> 
> 
> 
> >> +}
> >> +
> >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> >> +                                     hwaddr dsm_mem_addr)  
> > function name doesn't make any sense to me  
> 
> As i explained above, handle 0x10000 indicates the reserved _DSM method is
> called on the root device...
> 
> It makes sense now? :)
function name should reflect what it does,
i.e use verb and I see only nouns here.

Xiao Guangrong Nov. 3, 2016, 1:02 p.m. UTC | #7

On 11/03/2016 09:00 PM, Igor Mammedov wrote:




>>> just drop this and describe properly 'len' in spec section
>>> i.e. len: length of entire returned data (including the header)
>>
>> Okay, i will change the spec like this:
>>
>>     QEMU Writes Output Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, length of entire returned data (including the header)
>>
>> And drop the length field in Read_Fit return buffer, doc
>> the fit buffer like this:
>>
>>     +----------+--------+--------+-------------------------------------------+
>>     |  Field   | Length | Offset |                 Description               |
>>     +----------+--------+--------+-------------------------------------------+
> you need to add length here, otherwise this table is not correct

Ah, so i am confused.

struct NvdimmFuncReadFITOut definition is based on the layout of
Read_FI output. You suggested to drop the length filed in NvdimmFuncReadFITOut
but keep it in the layout, it is not consistent.

I missed something?

>
>
>>     |          |        |        | return status codes                       |
>>     |          |        |        | 0x100 - error caused by NFIT update while |
>>     | status   |   4    |   0    | read by _FIT wasn't completed, other      |
>>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
>>     +----------+--------+--------+-------------------------------------------+
>>     | fit data | Varies |   8    | FIT data, The remaining size in the       |
>>     |          |        |        | returned buffer is used by FIT            |
>>     +----------+--------+--------+-------------------------------------------+
>>
>>
>>
>>>> +}
>>>> +
>>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>>>> +                                     hwaddr dsm_mem_addr)
>>> function name doesn't make any sense to me
>>
>> As i explained above, handle 0x10000 indicates the reserved _DSM method is
>> called on the root device...
>>
>> It makes sense now? :)
> function name should reflect what it does,
> i.e use verb and I see only nouns here.

Got it, will change it to: nvdimm_handle_reserved_root_method().

Igor Mammedov Nov. 3, 2016, 2:49 p.m. UTC | #8

On Thu, 3 Nov 2016 21:02:22 +0800
Xiao Guangrong <[e-mail]> wrote:

> 
> 
> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> 
> 
> 
> 
> >>> just drop this and describe properly 'len' in spec section
> >>> i.e. len: length of entire returned data (including the header)
> >>
> >> Okay, i will change the spec like this:
> >>
> >>     QEMU Writes Output Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >> (including the header)
> >>
> >> And drop the length field in Read_Fit return buffer, doc
> >> the fit buffer like this:
> >>
> >>     +----------+--------+--------+-------------------------------------------+
> >>     |  Field   | Length | Offset |
> >> Description               |
> >> +----------+--------+--------+-------------------------------------------+
> > you need to add length here, otherwise this table is not correct
> 
> Ah, so i am confused.
> 
> struct NvdimmFuncReadFITOut definition is based on the layout of
> Read_FI output. You suggested to drop the length filed in
> NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.
> 
> I missed something?

+struct NvdimmFuncReadFITOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;

--------------------------------
| field       | len | off | desc...
--------------------------------
| length      |  4  |  0  | ....
--------------------------------
| status      |  4  |  4  | ....
--------------------------------
| fit data    | ................

i.e. you were forgetting to add length in spec so offsets were wrong
even for described fields.

> 
> >
> >
> >>     |          |        |        | return status
> >> codes                       | |          |        |        | 0x100
> >> - error caused by NFIT update while | | status   |   4    |   0
> >> | read by _FIT wasn't completed, other      | |          |
> >> |        | codes follow Chapter 3 in DSM Spec Rev1   |
> >> +----------+--------+--------+-------------------------------------------+
> >> | fit data | Varies |   8    | FIT data, The remaining size in
> >> the       | |          |        |        | returned buffer is used
> >> by FIT            |
> >> +----------+--------+--------+-------------------------------------------+
> >>
> >>
> >>
> >>>> +}
> >>>> +
> >>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state,
> >>>> NvdimmDsmIn *in,
> >>>> +                                     hwaddr dsm_mem_addr)
> >>> function name doesn't make any sense to me
> >>
> >> As i explained above, handle 0x10000 indicates the reserved _DSM
> >> method is called on the root device...
> >>
> >> It makes sense now? :)
> > function name should reflect what it does,
> > i.e use verb and I see only nouns here.
> 
> Got it, will change it to: nvdimm_handle_reserved_root_method().
>

Xiao Guangrong Nov. 3, 2016, 2:53 p.m. UTC | #9

On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> On Thu, 3 Nov 2016 21:02:22 +0800
> Xiao Guangrong <[e-mail]> wrote:
>
>>
>>
>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>
>>
>>
>>
>>>>> just drop this and describe properly 'len' in spec section
>>>>> i.e. len: length of entire returned data (including the header)
>>>>
>>>> Okay, i will change the spec like this:
>>>>
>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>> (including the header)
>>>>
>>>> And drop the length field in Read_Fit return buffer, doc
>>>> the fit buffer like this:
>>>>
>>>>     +----------+--------+--------+-------------------------------------------+
>>>>     |  Field   | Length | Offset |
>>>> Description               |
>>>> +----------+--------+--------+-------------------------------------------+
>>> you need to add length here, otherwise this table is not correct
>>
>> Ah, so i am confused.
>>
>> struct NvdimmFuncReadFITOut definition is based on the layout of
>> Read_FI output. You suggested to drop the length filed in
>> NvdimmFuncReadFITOut but keep it in the layout, it is not consistent.
>>
>> I missed something?
>
> +struct NvdimmFuncReadFITOut {
> +    /* the size of buffer filled by QEMU. */
> +    uint32_t len;
> +    uint32_t func_ret_status; /* return status code. */
> +    uint8_t fit[0]; /* the FIT data. */
> +} QEMU_PACKED;
>
> --------------------------------
> | field       | len | off | desc...
> --------------------------------
> | length      |  4  |  0  | ....
> --------------------------------
> | status      |  4  |  4  | ....
> --------------------------------
> | fit data    | ................
>
> i.e. you were forgetting to add length in spec so offsets were wrong
> even for described fields.


We can not do this.

@len is used by QEMU emulation to count the size of the buffer that
_DSM should return. It's only used in NVDIMM_COMMON_DSM method which
is shared by the DSM method from VM and Read_Fit.

Igor Mammedov Nov. 3, 2016, 4:13 p.m. UTC | #10

On Thu, 3 Nov 2016 22:53:43 +0800
Xiao Guangrong <[e-mail]> wrote:

> 
> 
> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 21:02:22 +0800
> > Xiao Guangrong <[e-mail]> wrote:
> >
> >>
> >>
> >> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>
> >>
> >>
> >>
> >>>>> just drop this and describe properly 'len' in spec section
> >>>>> i.e. len: length of entire returned data (including the header)
> >>>>
> >>>> Okay, i will change the spec like this:
> >>>>
> >>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>> (including the header)
> >>>>
> >>>> And drop the length field in Read_Fit return buffer, doc
> >>>> the fit buffer like this:
> >>>>
> >>>>     +----------+--------+--------+-------------------------------------------+
> >>>>     |  Field   | Length | Offset |
> >>>> Description               |
> >>>> +----------+--------+--------+-------------------------------------------+
> >>> you need to add length here, otherwise this table is not correct
> >>
> >> Ah, so i am confused.
> >>
> >> struct NvdimmFuncReadFITOut definition is based on the layout of
> >> Read_FI output. You suggested to drop the length filed in
> >> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >> consistent.
> >>
> >> I missed something?
> >
> > +struct NvdimmFuncReadFITOut {
> > +    /* the size of buffer filled by QEMU. */
> > +    uint32_t len;
> > +    uint32_t func_ret_status; /* return status code. */
> > +    uint8_t fit[0]; /* the FIT data. */
> > +} QEMU_PACKED;
> >
> > --------------------------------
> > | field       | len | off | desc...
> > --------------------------------
> > | length      |  4  |  0  | ....
> > --------------------------------
> > | status      |  4  |  4  | ....
> > --------------------------------
> > | fit data    | ................
> >
> > i.e. you were forgetting to add length in spec so offsets were wrong
> > even for described fields.
> 
> 
> We can not do this.
> 
> @len is used by QEMU emulation to count the size of the buffer that
> _DSM should return. It's only used in NVDIMM_COMMON_DSM method which
> is shared by the DSM method from VM and Read_Fit.
spec describes buffer layout independently from AML that uses it,
so it should describe whole data structure.

Then it's upto guest how to read this data, it could be QEMU generated
AML (as it's here) or some other driver or even BIOS.

Xiao Guangrong Nov. 3, 2016, 4:17 p.m. UTC | #11

On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> On Thu, 3 Nov 2016 22:53:43 +0800
> Xiao Guangrong <[e-mail]> wrote:
>
>>
>>
>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>> Xiao Guangrong <[e-mail]> wrote:
>>>
>>>>
>>>>
>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>
>>>>
>>>>
>>>>
>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>> i.e. len: length of entire returned data (including the header)
>>>>>>
>>>>>> Okay, i will change the spec like this:
>>>>>>
>>>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>> (including the header)
>>>>>>
>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>> the fit buffer like this:
>>>>>>
>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>     |  Field   | Length | Offset |
>>>>>> Description               |
>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>> you need to add length here, otherwise this table is not correct
>>>>
>>>> Ah, so i am confused.
>>>>
>>>> struct NvdimmFuncReadFITOut definition is based on the layout of
>>>> Read_FI output. You suggested to drop the length filed in
>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>> consistent.
>>>>
>>>> I missed something?
>>>
>>> +struct NvdimmFuncReadFITOut {
>>> +    /* the size of buffer filled by QEMU. */
>>> +    uint32_t len;
>>> +    uint32_t func_ret_status; /* return status code. */
>>> +    uint8_t fit[0]; /* the FIT data. */
>>> +} QEMU_PACKED;
>>>
>>> --------------------------------
>>> | field       | len | off | desc...
>>> --------------------------------
>>> | length      |  4  |  0  | ....
>>> --------------------------------
>>> | status      |  4  |  4  | ....
>>> --------------------------------
>>> | fit data    | ................
>>>
>>> i.e. you were forgetting to add length in spec so offsets were wrong
>>> even for described fields.
>>
>>
>> We can not do this.
>>
>> @len is used by QEMU emulation to count the size of the buffer that
>> _DSM should return. It's only used in NVDIMM_COMMON_DSM method which
>> is shared by the DSM method from VM and Read_Fit.
> spec describes buffer layout independently from AML that uses it,
> so it should describe whole data structure.
>
> Then it's upto guest how to read this data, it could be QEMU generated
> AML (as it's here) or some other driver or even BIOS.

However, what we are talking about is Read_FIT method, so this is
the layout of Read_FIT output rather than all memory in the dsm page.

Actually, in the spec we already have documented the common len field:

    QEMU Writes Output Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, the length of result
    [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU

Also, i really do not hope to use this field to count the buffer size
returned by Read_FIT, we'd try the best to reuse existing DSM method
(NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.

Igor Mammedov Nov. 3, 2016, 4:49 p.m. UTC | #12

On Fri, 4 Nov 2016 00:17:00 +0800
Xiao Guangrong <[e-mail]> wrote:

> 
> 
> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 22:53:43 +0800
> > Xiao Guangrong <[e-mail]> wrote:
> >
> >>
> >>
> >> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>> Xiao Guangrong <[e-mail]> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>> header)
> >>>>>>
> >>>>>> Okay, i will change the spec like this:
> >>>>>>
> >>>>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>> (including the header)
> >>>>>>
> >>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>> the fit buffer like this:
> >>>>>>
> >>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>     |  Field   | Length | Offset |
> >>>>>> Description               |
> >>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>> you need to add length here, otherwise this table is not correct
> >>>>
> >>>> Ah, so i am confused.
> >>>>
> >>>> struct NvdimmFuncReadFITOut definition is based on the layout of
> >>>> Read_FI output. You suggested to drop the length filed in
> >>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>> consistent.
> >>>>
> >>>> I missed something?
> >>>
> >>> +struct NvdimmFuncReadFITOut {
> >>> +    /* the size of buffer filled by QEMU. */
> >>> +    uint32_t len;
> >>> +    uint32_t func_ret_status; /* return status code. */
> >>> +    uint8_t fit[0]; /* the FIT data. */
> >>> +} QEMU_PACKED;
> >>>
> >>> --------------------------------
> >>> | field       | len | off | desc...
> >>> --------------------------------
> >>> | length      |  4  |  0  | ....
> >>> --------------------------------
> >>> | status      |  4  |  4  | ....
> >>> --------------------------------
> >>> | fit data    | ................
> >>>
> >>> i.e. you were forgetting to add length in spec so offsets were
> >>> wrong even for described fields.
> >>
> >>
> >> We can not do this.
> >>
> >> @len is used by QEMU emulation to count the size of the buffer that
> >> _DSM should return. It's only used in NVDIMM_COMMON_DSM method
> >> which is shared by the DSM method from VM and Read_Fit.
> > spec describes buffer layout independently from AML that uses it,
> > so it should describe whole data structure.
> >
> > Then it's upto guest how to read this data, it could be QEMU
> > generated AML (as it's here) or some other driver or even BIOS.
> 
> However, what we are talking about is Read_FIT method, so this is
> the layout of Read_FIT output rather than all memory in the dsm page.
> 
> Actually, in the spec we already have documented the common len field:
> 
>     QEMU Writes Output Data (based on the offset in the page):
>     [0x0 - 0x3]: 4 bytes, the length of result
>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> 
> Also, i really do not hope to use this field to count the buffer size
> returned by Read_FIT, we'd try the best to reuse existing DSM method
> (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.
buffer layout describes interface between QEMU and firmware (AML)
and it should describe it completely every time to avoid confusion.

See for example ACPI spec, NFIT table, SRAT table, ...
all table descriptions start with complete header.

If you skip length it rises question how much fit data are there,
meaning interface description isn't complete.

if you want to describe AML there you can do it after interface
description saying that common NCAL method extracts status and fit
data form dsm page and returns that as buffer object, but it's AML
impl. specific. I could write an alternative AML code that would deal
with dms page in its own way as long as I would know what write/read at
what offset.


> 
> 
> 
> 
>

Xiao Guangrong Nov. 3, 2016, 4:53 p.m. UTC | #13

On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> On Fri, 4 Nov 2016 00:17:00 +0800
> Xiao Guangrong <[e-mail]> wrote:
>
>>
>>
>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
>>> On Thu, 3 Nov 2016 22:53:43 +0800
>>> Xiao Guangrong <[e-mail]> wrote:
>>>
>>>>
>>>>
>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>>>> Xiao Guangrong <[e-mail]> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>>>> i.e. len: length of entire returned data (including the
>>>>>>>>> header)
>>>>>>>>
>>>>>>>> Okay, i will change the spec like this:
>>>>>>>>
>>>>>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>>>>>     [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>>>> (including the header)
>>>>>>>>
>>>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>>>> the fit buffer like this:
>>>>>>>>
>>>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>>>     |  Field   | Length | Offset |
>>>>>>>> Description               |
>>>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>>>> you need to add length here, otherwise this table is not correct
>>>>>>
>>>>>> Ah, so i am confused.
>>>>>>
>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout of
>>>>>> Read_FI output. You suggested to drop the length filed in
>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>>>> consistent.
>>>>>>
>>>>>> I missed something?
>>>>>
>>>>> +struct NvdimmFuncReadFITOut {
>>>>> +    /* the size of buffer filled by QEMU. */
>>>>> +    uint32_t len;
>>>>> +    uint32_t func_ret_status; /* return status code. */
>>>>> +    uint8_t fit[0]; /* the FIT data. */
>>>>> +} QEMU_PACKED;
>>>>>
>>>>> --------------------------------
>>>>> | field       | len | off | desc...
>>>>> --------------------------------
>>>>> | length      |  4  |  0  | ....
>>>>> --------------------------------
>>>>> | status      |  4  |  4  | ....
>>>>> --------------------------------
>>>>> | fit data    | ................
>>>>>
>>>>> i.e. you were forgetting to add length in spec so offsets were
>>>>> wrong even for described fields.
>>>>
>>>>
>>>> We can not do this.
>>>>
>>>> @len is used by QEMU emulation to count the size of the buffer that
>>>> _DSM should return. It's only used in NVDIMM_COMMON_DSM method
>>>> which is shared by the DSM method from VM and Read_Fit.
>>> spec describes buffer layout independently from AML that uses it,
>>> so it should describe whole data structure.
>>>
>>> Then it's upto guest how to read this data, it could be QEMU
>>> generated AML (as it's here) or some other driver or even BIOS.
>>
>> However, what we are talking about is Read_FIT method, so this is
>> the layout of Read_FIT output rather than all memory in the dsm page.
>>
>> Actually, in the spec we already have documented the common len field:
>>
>>     QEMU Writes Output Data (based on the offset in the page):
>>     [0x0 - 0x3]: 4 bytes, the length of result
>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
>>
>> Also, i really do not hope to use this field to count the buffer size
>> returned by Read_FIT, we'd try the best to reuse existing DSM method
>> (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM method.
> buffer layout describes interface between QEMU and firmware (AML)
> and it should describe it completely every time to avoid confusion.
>
> See for example ACPI spec, NFIT table, SRAT table, ...
> all table descriptions start with complete header.

Okay. Understood. :)

>
> If you skip length it rises question how much fit data are there,
> meaning interface description isn't complete.

So how about introduce a length field in the output buffer just
as this patch did? I understand we are able to count the size
from dsm len, however, it can simplify the code a lot...

>
> if you want to describe AML there you can do it after interface
> description saying that common NCAL method extracts status and fit
> data form dsm page and returns that as buffer object, but it's AML
> impl. specific. I could write an alternative AML code that would deal
> with dms page in its own way as long as I would know what write/read at
> what offset.

Yes, i agree with you.

Igor Mammedov Nov. 3, 2016, 5:29 p.m. UTC | #14

On Fri, 4 Nov 2016 00:53:06 +0800
Xiao Guangrong <[e-mail]> wrote:

> 
> 
> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:17:00 +0800
> > Xiao Guangrong <[e-mail]> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>> Xiao Guangrong <[e-mail]> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>> Xiao Guangrong <[e-mail]> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>>>> header)
> >>>>>>>>
> >>>>>>>> Okay, i will change the spec like this:
> >>>>>>>>
> >>>>>>>>     QEMU Writes Output Data (based on the offset in the
> >>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>>>> (including the header)
> >>>>>>>>
> >>>>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>>>> the fit buffer like this:
> >>>>>>>>
> >>>>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>>>     |  Field   | Length | Offset |
> >>>>>>>> Description               |
> >>>>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>>>> you need to add length here, otherwise this table is not
> >>>>>>> correct
> >>>>>>
> >>>>>> Ah, so i am confused.
> >>>>>>
> >>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
> >>>>>> of Read_FI output. You suggested to drop the length filed in
> >>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>>>> consistent.
> >>>>>>
> >>>>>> I missed something?
> >>>>>
> >>>>> +struct NvdimmFuncReadFITOut {
> >>>>> +    /* the size of buffer filled by QEMU. */
> >>>>> +    uint32_t len;
> >>>>> +    uint32_t func_ret_status; /* return status code. */
> >>>>> +    uint8_t fit[0]; /* the FIT data. */
> >>>>> +} QEMU_PACKED;
> >>>>>
> >>>>> --------------------------------
> >>>>> | field       | len | off | desc...
> >>>>> --------------------------------
> >>>>> | length      |  4  |  0  | ....
> >>>>> --------------------------------
> >>>>> | status      |  4  |  4  | ....
> >>>>> --------------------------------
> >>>>> | fit data    | ................
> >>>>>
> >>>>> i.e. you were forgetting to add length in spec so offsets were
> >>>>> wrong even for described fields.
> >>>>
> >>>>
> >>>> We can not do this.
> >>>>
> >>>> @len is used by QEMU emulation to count the size of the buffer
> >>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >>>> method which is shared by the DSM method from VM and Read_Fit.
> >>> spec describes buffer layout independently from AML that uses it,
> >>> so it should describe whole data structure.
> >>>
> >>> Then it's upto guest how to read this data, it could be QEMU
> >>> generated AML (as it's here) or some other driver or even BIOS.
> >>
> >> However, what we are talking about is Read_FIT method, so this is
> >> the layout of Read_FIT output rather than all memory in the dsm
> >> page.
> >>
> >> Actually, in the spec we already have documented the common len
> >> field:
> >>
> >>     QEMU Writes Output Data (based on the offset in the page):
> >>     [0x0 - 0x3]: 4 bytes, the length of result
> >>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>
> >> Also, i really do not hope to use this field to count the buffer
> >> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >> method.
> > buffer layout describes interface between QEMU and firmware (AML)
> > and it should describe it completely every time to avoid confusion.
> >
> > See for example ACPI spec, NFIT table, SRAT table, ...
> > all table descriptions start with complete header.
> 
> Okay. Understood. :)
> 
> >
> > If you skip length it rises question how much fit data are there,
> > meaning interface description isn't complete.
> 
> So how about introduce a length field in the output buffer just
> as this patch did? I understand we are able to count the size
> from dsm len, however, it can simplify the code a lot...
it's redundant as there already is length for whole buffer,
interface should be kept as simple as possible and not include
redundant data for convenience.

> 
> >
> > if you want to describe AML there you can do it after interface
> > description saying that common NCAL method extracts status and fit
> > data form dsm page and returns that as buffer object, but it's AML
> > impl. specific. I could write an alternative AML code that would
> > deal with dms page in its own way as long as I would know what
> > write/read at what offset.
> 
> Yes, i agree with you.
>

Xiao Guangrong Nov. 3, 2016, 5:39 p.m. UTC | #15

On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> On Fri, 4 Nov 2016 00:53:06 +0800
> Xiao Guangrong <[e-mail]> wrote:
>
>>
>>
>> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
>>> On Fri, 4 Nov 2016 00:17:00 +0800
>>> Xiao Guangrong <[e-mail]> wrote:
>>>
>>>>
>>>>
>>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
>>>>> On Thu, 3 Nov 2016 22:53:43 +0800
>>>>> Xiao Guangrong <[e-mail]> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
>>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
>>>>>>> Xiao Guangrong <[e-mail]> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> just drop this and describe properly 'len' in spec section
>>>>>>>>>>> i.e. len: length of entire returned data (including the
>>>>>>>>>>> header)
>>>>>>>>>>
>>>>>>>>>> Okay, i will change the spec like this:
>>>>>>>>>>
>>>>>>>>>>     QEMU Writes Output Data (based on the offset in the
>>>>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
>>>>>>>>>> (including the header)
>>>>>>>>>>
>>>>>>>>>> And drop the length field in Read_Fit return buffer, doc
>>>>>>>>>> the fit buffer like this:
>>>>>>>>>>
>>>>>>>>>>     +----------+--------+--------+-------------------------------------------+
>>>>>>>>>>     |  Field   | Length | Offset |
>>>>>>>>>> Description               |
>>>>>>>>>> +----------+--------+--------+-------------------------------------------+
>>>>>>>>> you need to add length here, otherwise this table is not
>>>>>>>>> correct
>>>>>>>>
>>>>>>>> Ah, so i am confused.
>>>>>>>>
>>>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
>>>>>>>> of Read_FI output. You suggested to drop the length filed in
>>>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
>>>>>>>> consistent.
>>>>>>>>
>>>>>>>> I missed something?
>>>>>>>
>>>>>>> +struct NvdimmFuncReadFITOut {
>>>>>>> +    /* the size of buffer filled by QEMU. */
>>>>>>> +    uint32_t len;
>>>>>>> +    uint32_t func_ret_status; /* return status code. */
>>>>>>> +    uint8_t fit[0]; /* the FIT data. */
>>>>>>> +} QEMU_PACKED;
>>>>>>>
>>>>>>> --------------------------------
>>>>>>> | field       | len | off | desc...
>>>>>>> --------------------------------
>>>>>>> | length      |  4  |  0  | ....
>>>>>>> --------------------------------
>>>>>>> | status      |  4  |  4  | ....
>>>>>>> --------------------------------
>>>>>>> | fit data    | ................
>>>>>>>
>>>>>>> i.e. you were forgetting to add length in spec so offsets were
>>>>>>> wrong even for described fields.
>>>>>>
>>>>>>
>>>>>> We can not do this.
>>>>>>
>>>>>> @len is used by QEMU emulation to count the size of the buffer
>>>>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
>>>>>> method which is shared by the DSM method from VM and Read_Fit.
>>>>> spec describes buffer layout independently from AML that uses it,
>>>>> so it should describe whole data structure.
>>>>>
>>>>> Then it's upto guest how to read this data, it could be QEMU
>>>>> generated AML (as it's here) or some other driver or even BIOS.
>>>>
>>>> However, what we are talking about is Read_FIT method, so this is
>>>> the layout of Read_FIT output rather than all memory in the dsm
>>>> page.
>>>>
>>>> Actually, in the spec we already have documented the common len
>>>> field:
>>>>
>>>>     QEMU Writes Output Data (based on the offset in the page):
>>>>     [0x0 - 0x3]: 4 bytes, the length of result
>>>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
>>>>
>>>> Also, i really do not hope to use this field to count the buffer
>>>> size returned by Read_FIT, we'd try the best to reuse existing DSM
>>>> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
>>>> method.
>>> buffer layout describes interface between QEMU and firmware (AML)
>>> and it should describe it completely every time to avoid confusion.
>>>
>>> See for example ACPI spec, NFIT table, SRAT table, ...
>>> all table descriptions start with complete header.
>>
>> Okay. Understood. :)
>>
>>>
>>> If you skip length it rises question how much fit data are there,
>>> meaning interface description isn't complete.
>>
>> So how about introduce a length field in the output buffer just
>> as this patch did? I understand we are able to count the size
>> from dsm len, however, it can simplify the code a lot...
> it's redundant as there already is length for whole buffer,
> interface should be kept as simple as possible and not include
> redundant data for convenience.

Okay.

So the doc should be changed to:

    Output layout in the dsm memory page:

    +----------+--------+--------+-------------------------------------------+
    |  Field   | Length | Offset |                 Description               |
    +----------+--------+--------+-------------------------------------------+
    | length   |   4    |   4    | length of entire returned data            |
    |          |        |        | (including the header)                    |
    +----------+-----------------+-------------------------------------------+
    |          |        |        | return status codes                       |
    |          |        |        | 0x100 - error caused by NFIT update while |
    | status   |   4    |   4    | read by _FIT wasn't completed, other      |
    |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
    +----------+--------+--------+-------------------------------------------+
    | fit data | Varies |   8    | FIT data, the remaining size is used by   |
    |          |        |        | fit data if status is success;            |
    |          |        |        | otherwise it is not valid                 |
    +----------+--------+--------+-------------------------------------------+

As you know, i am not good at doc, any suggestion is welcome. :)

Igor Mammedov Nov. 3, 2016, 5:54 p.m. UTC | #16

On Fri, 4 Nov 2016 01:39:31 +0800
Xiao Guangrong <[e-mail]> wrote:

> 
> 
> On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:53:06 +0800
> > Xiao Guangrong <[e-mail]> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> >>> On Fri, 4 Nov 2016 00:17:00 +0800
> >>> Xiao Guangrong <[e-mail]> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>>>> Xiao Guangrong <[e-mail]> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>>>> Xiao Guangrong <[e-mail]> wrote:
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 11/03/2016 09:00 PM, Igor Mammedov wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>>> just drop this and describe properly 'len' in spec section
> >>>>>>>>>>> i.e. len: length of entire returned data (including the
> >>>>>>>>>>> header)
> >>>>>>>>>>
> >>>>>>>>>> Okay, i will change the spec like this:
> >>>>>>>>>>
> >>>>>>>>>>     QEMU Writes Output Data (based on the offset in the
> >>>>>>>>>> page): [0x0 - 0x3]: 4 bytes, length of entire returned data
> >>>>>>>>>> (including the header)
> >>>>>>>>>>
> >>>>>>>>>> And drop the length field in Read_Fit return buffer, doc
> >>>>>>>>>> the fit buffer like this:
> >>>>>>>>>>
> >>>>>>>>>>     +----------+--------+--------+-------------------------------------------+
> >>>>>>>>>>     |  Field   | Length | Offset |
> >>>>>>>>>> Description               |
> >>>>>>>>>> +----------+--------+--------+-------------------------------------------+
> >>>>>>>>> you need to add length here, otherwise this table is not
> >>>>>>>>> correct
> >>>>>>>>
> >>>>>>>> Ah, so i am confused.
> >>>>>>>>
> >>>>>>>> struct NvdimmFuncReadFITOut definition is based on the layout
> >>>>>>>> of Read_FI output. You suggested to drop the length filed in
> >>>>>>>> NvdimmFuncReadFITOut but keep it in the layout, it is not
> >>>>>>>> consistent.
> >>>>>>>>
> >>>>>>>> I missed something?
> >>>>>>>
> >>>>>>> +struct NvdimmFuncReadFITOut {
> >>>>>>> +    /* the size of buffer filled by QEMU. */
> >>>>>>> +    uint32_t len;
> >>>>>>> +    uint32_t func_ret_status; /* return status code. */
> >>>>>>> +    uint8_t fit[0]; /* the FIT data. */
> >>>>>>> +} QEMU_PACKED;
> >>>>>>>
> >>>>>>> --------------------------------
> >>>>>>> | field       | len | off | desc...
> >>>>>>> --------------------------------
> >>>>>>> | length      |  4  |  0  | ....
> >>>>>>> --------------------------------
> >>>>>>> | status      |  4  |  4  | ....
> >>>>>>> --------------------------------
> >>>>>>> | fit data    | ................
> >>>>>>>
> >>>>>>> i.e. you were forgetting to add length in spec so offsets were
> >>>>>>> wrong even for described fields.
> >>>>>>
> >>>>>>
> >>>>>> We can not do this.
> >>>>>>
> >>>>>> @len is used by QEMU emulation to count the size of the buffer
> >>>>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM
> >>>>>> method which is shared by the DSM method from VM and Read_Fit.
> >>>>> spec describes buffer layout independently from AML that uses it,
> >>>>> so it should describe whole data structure.
> >>>>>
> >>>>> Then it's upto guest how to read this data, it could be QEMU
> >>>>> generated AML (as it's here) or some other driver or even BIOS.
> >>>>
> >>>> However, what we are talking about is Read_FIT method, so this is
> >>>> the layout of Read_FIT output rather than all memory in the dsm
> >>>> page.
> >>>>
> >>>> Actually, in the spec we already have documented the common len
> >>>> field:
> >>>>
> >>>>     QEMU Writes Output Data (based on the offset in the page):
> >>>>     [0x0 - 0x3]: 4 bytes, the length of result
> >>>>     [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
> >>>>
> >>>> Also, i really do not hope to use this field to count the buffer
> >>>> size returned by Read_FIT, we'd try the best to reuse existing DSM
> >>>> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM
> >>>> method.
> >>> buffer layout describes interface between QEMU and firmware (AML)
> >>> and it should describe it completely every time to avoid confusion.
> >>>
> >>> See for example ACPI spec, NFIT table, SRAT table, ...
> >>> all table descriptions start with complete header.
> >>
> >> Okay. Understood. :)
> >>
> >>>
> >>> If you skip length it rises question how much fit data are there,
> >>> meaning interface description isn't complete.
> >>
> >> So how about introduce a length field in the output buffer just
> >> as this patch did? I understand we are able to count the size
> >> from dsm len, however, it can simplify the code a lot...
> > it's redundant as there already is length for whole buffer,
> > interface should be kept as simple as possible and not include
> > redundant data for convenience.
> 
> Okay.
> 
> So the doc should be changed to:
> 
>     Output layout in the dsm memory page:
> 
>     +----------+--------+--------+-------------------------------------------+
>     |  Field   | Length | Offset |                 Description               |
>     +----------+--------+--------+-------------------------------------------+
>     | length   |   4    |   4    | length of entire returned data            |
>     |          |        |        | (including the header)                    |
wrong offset

>     +----------+-----------------+-------------------------------------------+
>     |          |        |        | return status codes                       |
>     |          |        |        | 0x100 - error caused by NFIT update while |
>     | status   |   4    |   4    | read by _FIT wasn't completed, other      |
>     |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
it wouldn't be bad to specify success status code here too.

>     +----------+--------+--------+-------------------------------------------+
>     | fit data | Varies |   8    | FIT data, the remaining size is used by   |
>     |          |        |        | fit data if status is success;            |
>     |          |        |        | otherwise it is not valid                 |
>     +----------+--------+--------+-------------------------------------------+
contains FIT data, this field is present if status field is [concrete number here]

> 
> As you know, i am not good at doc, any suggestion is welcome. :)
> 
>

Patch

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
index 0fdd251..364e832 100644
--- a/docs/specs/acpi_nvdimm.txt
+++ b/docs/specs/acpi_nvdimm.txt
@@ -65,8 +65,8 @@  _FIT(Firmware Interface Table)
    The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
    NVDIMM Firmware Interface Table (NFIT).
 
-QEMU NVDIMM Implemention
-========================
+QEMU NVDIMM Implementation
+==========================
 QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
 for NVDIMM ACPI.
 
@@ -82,6 +82,16 @@  Memory:
    ACPI writes _DSM Input Data (based on the offset in the page):
    [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
                 Root device.
+
+                The handle is completely QEMU internal thing, the values in
+                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
+                root device named NVDR), other values are reserved by other
+                purpose.
+
+                Current reserved handle:
+                0x10000 is reserved for QEMU internal DSM function called on
+                the root device.
+
    [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
    [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
    [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
@@ -127,6 +137,49 @@  _DSM process diagram:
  | result from the page     |      |              |
  +--------------------------+      +--------------+
 
- _FIT implementation
- -------------------
- TODO (will fill it when nvdimm hotplug is introduced)
+QEMU internal use only _DSM function
+------------------------------------
+There is the function introduced by QEMU and only used by QEMU internal.
+
+1) Read FIT
+   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
+   function (private QEMU function)
+
+   _FIT method uses Read_FIT function to fetch NFIT structures blob from
+   QEMU in 1 page sized increments which are then concatenated and returned
+   as _FIT method result.
+
+   Input parameters:
+   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
+   Arg1 – Revision ID (set to 1)
+   Arg2 - Function Index, 0x1
+   Arg3 - A package containing a buffer whose layout is as follows:
+
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
+   |          |        |        | read from                                 |
+   +----------+--------+--------+-------------------------------------------+
+
+   Output:
+   +----------+--------+--------+-------------------------------------------+
+   |  Field   | Length | Offset |                 Description               |
+   +----------+--------+--------+-------------------------------------------+
+   |          |        |        | return status codes                       |
+   |          |        |        | 0x100 - error caused by NFIT update while |
+   | status   |   4    |   0    | read by _FIT wasn't completed, other      |
+   |          |        |        | codes follow Chapter 3 in DSM Spec Rev1   |
+   +----------+--------+--------+-------------------------------------------+
+   | length   |   4    |   4    | The fit size                              |           
+   +----------+-----------------+-------------------------------------------+
+   | fit data | Varies |   8    | FIT data, its size is indicated by length |
+   |          |        |        | filed above                               |
+   +----------+--------+--------+-------------------------------------------+
+
+   The FIT offset is maintained by the OSPM itself, current offset plus
+   the length returned by the function is the next offset we should read.
+   When all the FIT data has been read out, zero length is returned.
+
+   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
+   again).
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fee077..593ac0d 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -484,6 +484,23 @@  typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
                   offsetof(NvdimmDsmIn, arg3) > 4096);
 
+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) > 4096);
+
+struct NvdimmFuncReadFITOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint32_t length; /* the length of fit. */
+    uint8_t fit[0]; /* the FIT data. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096);
+
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
 {
@@ -504,6 +521,77 @@  nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
     cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
 }
 
+#define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
+#define NVDIMM_DSM_RET_STATUS_UNSUPPORT      1 /* Not Supported */
+#define NVDIMM_DSM_RET_STATUS_INVALID        3 /* Invalid Input Parameters */
+#define NVDIMM_DSM_RET_STATUS_FIT_CHANGED    0x100 /* FIT Changed */
+
+#define NVDIMM_QEMU_RSVD_HANDLE_ROOT         0x10000
+
+/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */
+static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+                                     hwaddr dsm_mem_addr)
+{
+    NvdimmFitBuffer *fit_buf = &state->fit_buf;
+    NvdimmFuncReadFITIn *read_fit;
+    NvdimmFuncReadFITOut *read_fit_out;
+    GArray *fit;
+    uint32_t read_len = 0, func_ret_status, offset;
+    int size;
+
+    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
+    offset = le32_to_cpu(read_fit->offset);
+
+    fit = fit_buf->fit;
+
+    nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
+                 offset, fit->len, fit_buf->dirty ? "Yes" : "No");
+
+    /* It is the first time to read FIT. */
+    if (!offset) {
+        fit_buf->dirty = false;
+    } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
+        func_ret_status = NVDIMM_DSM_RET_STATUS_FIT_CHANGED;
+        goto exit;
+    }
+
+    if (offset > fit->len) {
+        func_ret_status = NVDIMM_DSM_RET_STATUS_INVALID;
+        goto exit;
+    }
+
+    func_ret_status = NVDIMM_DSM_RET_STATUS_SUCCESS;
+    read_len = MIN(fit->len - offset, 4096 - sizeof(NvdimmFuncReadFITOut));
+
+exit:
+    size = sizeof(NvdimmFuncReadFITOut) + read_len;
+    read_fit_out = g_malloc(size);
+
+    read_fit_out->len = cpu_to_le32(size);
+    read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+    read_fit_out->length = cpu_to_le32(read_len);
+    memcpy(read_fit_out->fit, fit->data + offset, read_len);
+
+    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+
+    g_free(read_fit_out);
+}
+
+static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in,
+                                     hwaddr dsm_mem_addr)
+{
+    switch (in->function) {
+    case 0x0:
+        nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr);
+        return;
+    case 0x1 /* Read FIT */:
+        nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr);
+        return;
+    }
+
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
+}
+
 static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     /*
@@ -563,7 +651,7 @@  static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
 
     nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
 
-    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
+    label_size_out.func_ret_status = cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
     label_size_out.label_size = cpu_to_le32(label_size);
     label_size_out.max_xfer = cpu_to_le32(mxfer);
 
@@ -574,7 +662,7 @@  static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
 static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
                                            uint32_t offset, uint32_t length)
 {
-    uint32_t ret = 3 /* Invalid Input Parameters */;
+    uint32_t ret = NVDIMM_DSM_RET_STATUS_INVALID;
 
     if (offset + length < offset) {
         nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
@@ -594,7 +682,7 @@  static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
         return ret;
     }
 
-    return 0 /* Success */;
+    return NVDIMM_DSM_RET_STATUS_SUCCESS;
 }
 
 /*
@@ -618,7 +706,7 @@  static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
                                         get_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
         nvdimm_dsm_no_payload(status, dsm_mem_addr);
         return;
     }
@@ -628,7 +716,8 @@  static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     get_label_data_out = g_malloc(size);
 
     get_label_data_out->len = cpu_to_le32(size);
-    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
+    get_label_data_out->func_ret_status =
+                                cpu_to_le32(NVDIMM_DSM_RET_STATUS_SUCCESS);
     nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
                          get_label_data->length, get_label_data->offset);
 
@@ -656,7 +745,7 @@  static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
                                         set_label_data->length);
-    if (status != 0 /* Success */) {
+    if (status != NVDIMM_DSM_RET_STATUS_SUCCESS) {
         nvdimm_dsm_no_payload(status, dsm_mem_addr);
         return;
     }
@@ -666,7 +755,7 @@  static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     nvc->write_label_data(nvdimm, set_label_data->in_buf,
                           set_label_data->length, set_label_data->offset);
-    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_SUCCESS, dsm_mem_addr);
 }
 
 static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
@@ -717,7 +806,7 @@  static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
         break;
     }
 
-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
 }
 
 static uint64_t
@@ -730,6 +819,7 @@  nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 static void
 nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
+    AcpiNVDIMMState *state = opaque;
     NvdimmDsmIn *in;
     hwaddr dsm_mem_addr = val;
 
@@ -753,7 +843,12 @@  nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
         nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
                      in->revision, 0x1);
-        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+        nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
+        goto exit;
+    }
+
+    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
+        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
         goto exit;
     }
 
@@ -809,9 +904,13 @@  void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
 #define NVDIMM_DSM_OUT_BUF      "ODAT"
 
+#define NVDIMM_DSM_RFIT_STATUS  "RSTA"
+
+#define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
+
 static void nvdimm_build_common_dsm(Aml *dev)
 {
-    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
+    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
     Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
     uint8_t byte_list[1];
@@ -900,9 +999,15 @@  static void nvdimm_build_common_dsm(Aml *dev)
                /* UUID for NVDIMM Root Device */, expected_uuid));
     aml_append(method, ifctx);
     elsectx = aml_else();
-    aml_append(elsectx, aml_store(
+    ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)));
+    aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID
+               /* UUID for QEMU internal use */), expected_uuid));
+    aml_append(elsectx, ifctx);
+    elsectx2 = aml_else();
+    aml_append(elsectx2, aml_store(
                aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
                /* UUID for NVDIMM Devices */, expected_uuid));
+    aml_append(elsectx, elsectx2);
     aml_append(method, elsectx);
 
     uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
@@ -919,7 +1024,7 @@  static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(unsupport, ifctx);
 
     /* No function is supported yet. */
-    byte_list[0] = 1 /* Not Supported */;
+    byte_list[0] = NVDIMM_DSM_RET_STATUS_UNSUPPORT;
     aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
     aml_append(method, unsupport);
 
@@ -982,6 +1087,101 @@  static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
     aml_append(dev, method);
 }
 
+static void nvdimm_build_fit_method(Aml *dev)
+{
+    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
+    Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit;
+
+    buf = aml_local(0);
+    buf_size = aml_local(1);
+    fit = aml_local(2);
+
+    aml_append(dev, aml_name_decl(NVDIMM_DSM_RFIT_STATUS, aml_int(0)));
+
+    /* build helper function, RFIT. */
+    method = aml_method("RFIT", 1, AML_SERIALIZED);
+    aml_append(method, aml_name_decl("OFST", aml_int(0)));
+
+    /* prepare input package. */
+    pkg = aml_package(1);
+    aml_append(method, aml_store(aml_arg(0), aml_name("OFST")));
+    aml_append(pkg, aml_name("OFST"));
+
+    /* call Read_FIT function. */
+    call_result = aml_call5(NVDIMM_COMMON_DSM,
+                            aml_touuid(NVDIMM_QEMU_RSVD_UUID),
+                            aml_int(1) /* Revision 1 */,
+                            aml_int(0x1) /* Read FIT */,
+                            pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT));
+    aml_append(method, aml_store(call_result, buf));
+
+    /* handle _DSM result. */
+    aml_append(method, aml_create_dword_field(buf,
+               aml_int(0) /* offset at byte 0 */, "STAU"));
+
+    aml_append(method, aml_store(aml_name("STAU"),
+                                 aml_name(NVDIMM_DSM_RFIT_STATUS)));
+
+     /* if something is wrong during _DSM. */
+    ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU"));
+    ifctx = aml_if(aml_lnot(ifcond));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_create_dword_field(buf,
+               aml_int(4) /* offset at byte 4 */, "LENG"));
+    aml_append(method, aml_store(aml_name("LENG"), buf_size));
+    /* if we read the end of fit. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(aml_buffer(0, NULL)));
+    aml_append(method, ifctx);
+
+    aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)),
+                                 buf_size));
+    aml_append(method, aml_create_field(buf,
+                            aml_int(8 * BITS_PER_BYTE), /* offset at byte 4.*/
+                            buf_size, "BUFF"));
+    aml_append(method, aml_return(aml_name("BUFF")));
+    aml_append(dev, method);
+
+    /* build _FIT. */
+    method = aml_method("_FIT", 0, AML_SERIALIZED);
+    offset = aml_local(3);
+
+    aml_append(method, aml_store(aml_buffer(0, NULL), fit));
+    aml_append(method, aml_store(aml_int(0), offset));
+
+    whilectx = aml_while(aml_int(1));
+    aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf));
+    aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size));
+
+    /*
+     * if fit buffer was changed during RFIT, read from the beginning
+     * again.
+     */
+    ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS),
+                             aml_int(NVDIMM_DSM_RET_STATUS_FIT_CHANGED)));
+    aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit));
+    aml_append(ifctx, aml_store(aml_int(0), offset));
+    aml_append(whilectx, ifctx);
+
+    elsectx = aml_else();
+
+    /* finish fit read if no data is read out. */
+    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
+    aml_append(ifctx, aml_return(fit));
+    aml_append(elsectx, ifctx);
+
+    /* update the offset. */
+    aml_append(elsectx, aml_add(offset, buf_size, offset));
+    /* append the data we read out to the fit buffer. */
+    aml_append(elsectx, aml_concatenate(fit, buf, fit));
+    aml_append(whilectx, elsectx);
+    aml_append(method, whilectx);
+
+    aml_append(dev, method);
+}
+
 static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
 {
     uint32_t slot;
@@ -1040,6 +1240,7 @@  static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
 
     /* 0 is reserved for root device. */
     nvdimm_build_device_dsm(dev, 0);
+    nvdimm_build_fit_method(dev);
 
     nvdimm_build_nvdimm_devices(dev, ram_slots);