Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT

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

On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved
>> by QEMU to read the piece of FIT buffer. The buffer is concatenated
>> before _FIT return
> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve
> 0xFFFFFFFF for some purposes.
> So spec should be amended first or custom generated UUID should be used.

Okay.

I will change the changelog to reflect this fact and move the spec update
to this patch.

>
>>
>> Refer to docs/specs/acpi-nvdimm.txt for detailed design
> and amend docs to reflect that.

Already done in the spec, i will merge the spec changes into
this patch as you suggested later.

>
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 0e2b9f0..4bbd1e7 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
>>      aml_append(dev, method);
>>  }
>>
>> +static void nvdimm_build_fit(Aml *dev)
>> +{
>> +    Aml *method, *pkg, *buf, *buf_size, *offset, *call_result;
>> +    Aml *whilectx, *ifcond, *ifctx, *fit;
>> +
>> +    buf = aml_local(0);
>> +    buf_size = aml_local(1);
>> +    fit = aml_local(2);
>> +
>> +    /* build helper function, RFIT. */
>> +    method = aml_method("RFIT", 1, AML_NOTSERIALIZED);
> since you create named fields (global variable) in method scope,
> you should make method serialized. Same goes for _FIT method.

Indeed, will fix.

>
>
>> +    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("2F10E7A4-9E91-11E4-89D3-123B93F75CBA"
>> +                            /* UUID for NVDIMM Root Device */),
>> +                            aml_int(1) /* Revision 1 */,
>> +                            aml_int(0xFFFFFFFF) /* Read FIT. */,
>> +                            pkg, aml_int(0) /* for root device. */);
>> +    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"));
>> +
>> +     /* 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));
>
> Since you handle error case the same as EOF case you could replace
> it with EOF case here and on qemu side of interface as well. That should
> simplify code a bit as you won't need to strip out func_ret_status.
>

You mean returning NULL buffer if errors happen? However, the buffer is
generated by NVDIMM_COMMON_DSM function which is also used by _DSM based
on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway
no matter is successful or failed.

Or i missed your idea?