Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 11/02/2016 09:56 PM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:39 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> _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 <guangrong.xiao@linux.intel.com>
>> ---
> [...]
>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 5f728a6..fc1a012 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
>>  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
>>                    offsetof(NvdimmDsmIn, arg3) > 4096);
>>
>> +struct NvdimmFuncReadFITIn {
>> +    uint32_t offset; /* the offset of 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. */
>> +    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)
>>  {
>> @@ -516,6 +532,74 @@ 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_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;
>> +    int size;
>> +
>> +    read_fit = (NvdimmFuncReadFITIn *)in->arg3;
>> +    le32_to_cpus(&read_fit->offset);
> I'd prefer if you'd not do inplace conversion, just do
> offset = le32_to_cpu(read_fit->offset);

okay.

>
>> +
>> +    qemu_mutex_lock(&fit_buf->lock);
>> +    fit = fit_buf->fit;
>> +
>> +    nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n",
>> +                 read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No");
> as follow up path replace nvdimm_debug() with trace events

I will do it as a separate patch.

>
>> +
>> +    if (read_fit->offset > fit->len) {
>> +        func_ret_status = 3 /* Invalid Input Parameters */;
> should be macros instead of magic value

Yes.

>
>> +        goto exit;
>> +    }
>> +
>> +    /* It is the first time to read FIT. */
>> +    if (!read_fit->offset) {
>> +        fit_buf->dirty = false;
>> +    } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */
>> +        func_ret_status = 0x100 /* fit changed */;
> should be macros instead of magic value

okay.

>
>> +        goto exit;
>> +    }
>> +
>> +    func_ret_status = 0 /* Success */;
>> +    read_len = MIN(fit->len - read_fit->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);
>> +    memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
>> +
>> +    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
>> +
>> +    g_free(read_fit_out);
>> +    qemu_mutex_unlock(&fit_buf->lock);
>> +}
>> +
>> +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(1 /* Not Supported */, dsm_mem_addr);
> should be macros instead of magic value

Okay.

>
>> +}
>> +
>>  static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>>  {
>>      /*
>> @@ -742,6 +826,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;
>>
>> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>          goto exit;
>>      }
>>
>> +    if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) {
>> +        nvdimm_dsm_reserved_root(state, in, dsm_mem_addr);
>> +        goto exit;
>> +    }
>> +
>>       /* Handle 0 is reserved for NVDIMM Root Device. */
>>      if (!in->handle) {
>>          nvdimm_dsm_root(in, dsm_mem_addr);
>> @@ -821,9 +911,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];
>> @@ -912,9 +1006,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));
>> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>>      aml_append(dev, method);
>>  }
>>
>> +static void nvdimm_build_fit(Aml *dev)
> nvdimm_build_fit_method()

okay.

>
>> +{
>> +    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_create_dword_field(aml_buffer(4, NULL),
>> +               aml_int(0), NVDIMM_DSM_RFIT_STATUS));
> it doesn't have to be buffer as it's internal ASL integer object
> so it could be just named variable.

Let me try.

>
> I'd also move it to _FIT method instead of making it device global

We can not as it is used both in _FIT method and RFIT method.

>
> and if it could work try to pass it as argument to RFIT
> RefOf/DerefOf may help here
> or make return value instead of buffer and pass buffer as reference.
>

Let me try.

> Alternatively you can return buffer from RFIT with status field included
> and check/discard status value there.
>

As we can not create name object in a while-loop, it is not easy to check
the status in _FIT.

>> +
>> +    /* build helper function, RFIT. */
>> +    method = aml_method("RFIT", 1, AML_SERIALIZED);
>> +    aml_append(method, aml_create_dword_field(aml_buffer(4, NULL),
>> +                                              aml_int(0), "OFST"));
>> +
>> +    /* 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_store(aml_sizeof(buf), buf_size));
>> +    aml_append(method, aml_subtract(buf_size,
>> +                                    aml_int(4) /* the size of "STAU" */,
>> +                                    buf_size));
>> +
>> +    /* if we read the end of fit. */
>> +    ifctx = aml_if(aml_equal(buf_size, aml_int(0)));
>> +    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));
> there isn't need to convert bytes to bits and store it in the same variable
> it creates confusion

Okay, i will introduce a new variable named buf_size_bits.

>
>> +    aml_append(method, aml_create_field(buf,
>> +                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
>> +                            buf_size, "BUFF"));
> just inline conversion in aml_create_field()

okay.