From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v5 1/5] nvdimm acpi: initialize the resource used by NVDIMM ACPI Date: Thu, 3 Mar 2016 00:10:25 +0800 Message-ID: <56D71071.8050802@linux.intel.com> References: <1456919441-101204-1-git-send-email-guangrong.xiao@linux.intel.com> <1456919441-101204-2-git-send-email-guangrong.xiao@linux.intel.com> <20160302135630-mutt-send-email-mst@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" Return-path: Received: from mga11.intel.com ([192.55.52.93]:48165 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755991AbcCBQKk (ORCPT ); Wed, 2 Mar 2016 11:10:40 -0500 In-Reply-To: <20160302135630-mutt-send-email-mst@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 03/02/2016 07:58 PM, Michael S. Tsirkin wrote: > On Wed, Mar 02, 2016 at 07:50:37PM +0800, Xiao Guangrong wrote: >> 32 bits IO port starting from 0x0a18 in guest is reserved for NVDIMM >> ACPI emulation. The table, NVDIMM_DSM_MEM_FILE, will be patched into >> NVDIMM ACPI binary code >> >> OSPM uses this port to tell QEMU the final address of the DSM memory >> and notify QEMU to emulate the DSM method >> >> Signed-off-by: Xiao Guangrong > > > two minor comments: don't respin just for these, can > be addressed later if necessary. Okay. > >> --- >> hw/acpi/Makefile.objs | 2 +- >> hw/acpi/nvdimm.c | 35 +++++++++++++++++++++++++++++++++++ >> hw/i386/acpi-build.c | 10 +--------- >> hw/i386/pc.c | 6 +++--- >> hw/i386/pc_piix.c | 5 +++++ >> hw/i386/pc_q35.c | 8 +++++++- >> include/hw/i386/pc.h | 4 +++- >> include/hw/mem/nvdimm.h | 28 +++++++++++++++++++++++++++- >> 8 files changed, 82 insertions(+), 16 deletions(-) > > How about a spec document to document the interface? Sure, good to me. Actually, there was a spec file in the old versions (e.g, https://github.com/xiaogr/qemu/commit/3e30799182ec53fd56af1e753a24ccf9f6a8f047), I will add it in the last part which will implement the real DSM functions. >> +/* >> + * AcpiNVDIMMState: >> + * @is_enabled: detect if NVDIMM support is enabled. >> + * >> + * @dsm_mem: the data of the fw_cfg file NVDIMM_DSM_MEM_FILE. >> + * @io_mr: the IO region used by OSPM to transfer control to QEMU. >> + */ > > this is not the way we document structures normally. > comments belong near fields. > Okay. keep that in my mind. ;)