Re: [Qemu-devel] [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong]

On 03/01/2016 05:08 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 04:53:23PM +0800, Xiao Guangrong wrote:
> > On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:
> >
> > > +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> > > + * and return the offset to 0x00000000 for runtime patching.
> > > + *
> > > + * Warning: runtime patching is best avoided. Only use this as
> > > + * a replacement for DataTableRegion (for guests that don't
> > > + * support it).
> > > + */
> > > +int
> > > +build_append_named_dword(GArray *array, const char *name_format, ...)
> > > +{
> > > +    int offset;
> > > +    va_list ap;
> > > +
> > > +    va_start(ap, name_format);
> > > +    build_append_namestringv(array, name_format, ap);
> > > +    va_end(ap);
> >
> > The NameOP was missed here...
> >
> > The idea is great and i fixed and applied it on the top this patchset, the patch
> > is attached, would it be good to you?
>
> OK but I can't review this patch on top of patch.
> Please split this in aml-build and nvdimm changes,
> then squash the am-build change with my patch and include it
> as 5/8, then append yours squashed with the nvdimm.c changes.

Okay... will do.


> Rename it something that implies what it does, not it's value. Offset of
> what is it?
>
> For example
>         nvdimm_ssdt = table_data->len;

Yep, good to me.

> > -    aml_append(sb_scope, mem_addr);
> > -    aml_append(ssdt, sb_scope);
> >       /* copy AML table into ACPI tables blob and patch header there */
> >       g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> > -
> > -    offset = table_data->len - 4;
> > -
> > -    /*
> > -     * zero the last 4 bytes, i.e, it is the offset of
> > -     * NVDIMM_ACPI_MEM_ADDR object.
> > -     */
> > -    g_array_remove_range(table_data, offset, 4);
> > -    g_array_append_vals(table_data, &zero_offset, 4);
> > +    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);
>
> Here too, please give it a better name
>         mem_addr_offset = ....; ?

Yup, it is better.