From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Subject: Re: [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory
Date: Tue, 1 Mar 2016 17:18:01 +0800
Message-ID: <56D55E49.8080405@linux.intel.com>
References: <1455439865-75784-1-git-send-email-guangrong.xiao@linux.intel.com>
 <1455439865-75784-6-git-send-email-guangrong.xiao@linux.intel.com>
 <20160229112956-mutt-send-email-mst@redhat.com>
 <56D55883.3090901@linux.intel.com> <20160301090836.GA27812@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: pbonzini@redhat.com, imammedo@redhat.com, gleb@kernel.org,
	mtosatti@redhat.com, stefanha@redhat.com, rth@twiddle.net,
	ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org,
	qemu-devel@nongnu.org
To: "Michael S. Tsirkin" <mst@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga14.intel.com ([192.55.52.115]:29116 "EHLO mga14.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752671AbcCAJSO (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 1 Mar 2016 04:18:14 -0500
In-Reply-To: <20160301090836.GA27812@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>



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.