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

[Xiao Guangrong]

On 09/30/2016 09:14 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:05 +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
> >
> > 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 <address@hidden>
> > ---
> >  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?