Re: [Qemu-devel] [PATCH v3 26/32] nvdimm: save arg3 for NVDIMM device _DSM method

[Xiao Guangrong]

On 10/19/2015 03:47 PM, Michael S. Tsirkin wrote:

> > > aml_arg(3) is used many times below.
> > > Pls give it a name that makes sense (not arg3! what is it for?)
> >
> > Er. aml_arg(3) is just the fourth parameter of _DSM method. Will add some
> > comments:
> >
> > /*
> >   * The fourth parameter (Arg3) of _DMS is a package which contains a buffer, 
> > the
> >   * layout of the buffer is specified by UUID (Arg0), Revision ID (Arg1) and
> >   * Function Index (Arg2) which are documented in the DSM specification.
> >   */
> >
> > > > +                                             aml_int(1))));
> > >
> > > Pls document AML constants used.
> > > Like this:
> > >
> > >              ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)),
> > >                                               aml_int(4 /* 4 - Package */) ),
> > >                                     aml_equal(aml_sizeof(aml_arg(3)),
> > >                                               aml_int(1))));
> > >
> > > > +            {
> > > > +                /* Local0 = Index(Arg3, 0) */
> > > > +                aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)),
> > > > +                                            aml_local(0)));
> > > > +                /* Local3 = DeRefOf(Local0) */
> > > > +                aml_append(ifctx, aml_store(aml_derefof(aml_local(0)),
> > > > +                                            aml_local(3)));
> > > > +                /* ARG3 = Local3 */
> > > > +                aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3")));
> > >
> > > This isn't a good way to comment things: you are
> > > just adding ASL before the equivalent C.
> > > Pls document what's going on.
> >
> > Okay... i just thought C is little readable than AML. Will change the comment
> > to:
> >
> > /* fetch buffer from the package (Arg3) and store it to DSM memory. */
> >
> > Thanks.
>
> You can use variables to make the logic clear. E.g.:
>
>         Aml *pckg = aml_arg(3);
>         Aml *pckg_idx = aml_local(0);
>         Aml *pckg_buf = aml_local(3);
>
>         aml_append(ifctx, aml_store(aml_index(pckg, aml_int(0)), pckg_idx);
>         aml_append(ifctx, aml_store(aml_derefof(pckg_idx), pckg_buf));
>
>
> This is also better than repeating aml_arg(3) many times.

Indeed, it's more clearer now.

Thanks for your review and really appreciate for your patience, Michael!