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

From: Igor Mammedov <imammedo@redhat.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com,
	stefanha@redhat.com, mst@redhat.com, rth@twiddle.net,
	ehabkost@redhat.com, dan.j.williams@intel.com,
	kvm@vger.kernel.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT
Date: Fri, 30 Sep 2016 15:14:22 +0200	[thread overview]
Message-ID: <20160930151422.6327c7d1@nial.brq.redhat.com> (raw)
In-Reply-To: <1470984850-66891-4-git-send-email-guangrong.xiao@linux.intel.com>

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.

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

> 
> 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.

> +    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.

> +
> +    /* 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(4 * 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_NOTSERIALIZED);
> +    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));
> +
> +    /* 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(whilectx, ifctx);
> +
> +    /* update the offset. */
> +    aml_append(whilectx, aml_add(offset, buf_size, offset));
> +    /* append the data we read out to the fit buffer. */
> +    aml_append(whilectx, aml_concatenate(fit, buf, fit));
> +    aml_append(method, whilectx);
> +
> +    aml_append(dev, method);
> +}
> +
>  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
>  {
>      uint32_t slot;
> @@ -1001,6 +1082,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(dev);
>  
>      nvdimm_build_nvdimm_devices(dev, ram_slots);
>  