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

[Igor Mammedov <imammedo@redhat.com>]

On Sat, 8 Oct 2016 15:17:14 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> 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.
under spec, I've meant ACPI spec where this UUID is declared

> 
> >  
> >>
> >> 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?
ok, leave it as is.

> 
> 
> 
>