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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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.