Re: [Qemu-devel] [PATCH v5 1/5] nvdimm acpi: initialize the resource used by NVDIMM ACPI

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

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 <guangrong.xiao@linux.intel.com>
>
>
> 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. ;)