Re: [Qemu-devel] [PATCH 0/6] NVDIMM ACPI: introduce the framework of QEMU emulated DSM

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

On 01/07/2016 10:13 PM, Igor Mammedov wrote:
> On Tue,  5 Jan 2016 02:52:02 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
>
>> This patchset is against commit 5530427f0ca (acpi: extend aml_and() to
>> accept target argument) on pci branch of Michael's git tree
>> and can be found at:
>>        https://github.com/xiaogr/qemu.git nvdimm-acpi-v1
>>
>> This is the second part of vNVDIMM implementation which implements the
>> BIOS patched dsm memory and introduces the framework that allows QEMU
>> to emulate DSM method
>>
>> Thanks to Michael's idea, we do not reserve any memory for NVDIMM ACPI,
>> instead we let BIOS allocate the memory and patch the address to the
>> offset we want
>>
>> IO port is still enabled as it plays as the way to notify QEMU and pass
>> the patched dsm memory address, so that IO port region, 0x0a18 - 0xa20,
>> is reserved and it is divided into two 32 bits ports and used to pass
>> the low 32 bits and high 32 bits of dsm memory address to QEMU
>>
>> Thanks Igor's idea, this patchset also extends DSDT/SSDT to revision 2
>> to apply 64 bit operations, in order to keeping compatibility, old
>> version (<= 2.5) still uses revision 1. Since 64 bit operations breaks
>> old guests (such as windows XP), we should keep the 64 bits stuff in
>> the private place where common ACPI operation does not touch it
>>
>
> general notes:
> 1. could you split out AML API additions/changes into separate patches?
>     even if series nvdims patches couldn't be accepted on next respin,
>     AML API patches could be good and we could pick them up just
>     for API completeness. That also would make them easier to review
>     and reduces count of patches you'd need to respin.

Yes, it is definitely better. Have done it in the v2.

> 2. add test case for NVDIMM table blob, see tests/bios-tables-test.c
>     at the beginning of series.
> 3. make V=1 check would show you ASL diff your patches are introducing,
>     it will save you from booting real guest and dumping/decompiling
>     tables manually.
> 4. at the end of series add NVDIMM table test blob with new table.
>     you can use tests/acpi-test-data/rebuild-expected-aml.sh to make it
> 5. if make check by some miracle passes with these patches,
>     dump NVDIMM table in guest and try to decompile and then compile it
>     back with IASL, it will show you what needs to be fixed.

Igor, really appreciate the tips you shared to me, that helped me a lot
in the development of the new version.

BTW, i did not touch the core bios_linker_loader_add_pointer() API as
i think it can be changed in a separated patchset. In the new version
we zeroed the offset by nvdimm itself and dropped aml_int64().

The new version has been posted out, please review.

Thank you!