Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Thu,  3 Nov 2016 11:51:29 +0800
Xiao Guangrong <address@hidden> 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 <address@hidden>
> ---
>  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);
>