No need to CC developers for libvirt, we're all subscribed to the list anyways and generally are faithful in reading. Reviews are a different story. On 12/12/18 7:52 AM, Luyao Zhong wrote: > Hi libvirt experts, > > This is the RFC v3 for updating NVDIMM support in libvirt. <sigh> https://libvirt.org/hacking.html ... git send-email --cover-letter --no-chain-reply-to --annotate \ --confirm=always --to=libvir-list@xxxxxxxxxx master You've missed the '--no-chain-reply-to'... In order to test how something would look on the list, alter the above "--to" to be yourself and see how things would look. Seems this more of a v3 and less of an RFC, true? Still based on what I know, there'll need to be a v4 anyway. > > There are some gaps between qemu and libvirt, libvirt has not > supported several config options about NVDIMM memory while > qemu is ready now, including 'align', 'pmem', 'unarmed'. > > I reworded and recoded my patches according to some feedback > comments from community once more. > > But I met some issues I can't handle. I list them as follows: > > a. add qemu_capabilities check > I want to add some nvdimm-related qemu_capabilities check, just > like 'QEMU_CAPS_OBJECT_MEMORY_FILE_ALIGN' in patch 2/4, and > I try to add the relevant sections into *.replies files manually. > But the qemucapabilitiestest failed, I don't know why. It seems > something wrong with the *.replies file. I think the *.replies > doesn't depends on other code or file, right? Could you help me address > this issue? The test log doesn't give me any useful info. You don't hand modify the .replies and .xml files - they are generated via tools, see the end of tests/qemucapabilitiestest.c for details. For the two caps you've added - one (*ALIGN) was introduced in qemu.git commit 9837684. It's already represented in the *2.12*.replies file, search on "memory-backend-file" and then the "align". The other (*PMEM) was introduced in qemu.git commit a4de8552. It's already represented in the *3.1*.replies file, using a similar search except for "pmem". As for the 3rd feature w/ nvdimm flags, well that's a little different than a memory-backend-file object attribute. The nvdimm is a -device and so you need to follow a different model - see other 'static struct virQEMUCapsStringFlags virQEMUCapsDeviceProps*'. You may need to go look at some previous commits to find examples that will generate the .replies/.xml changes to create a flag. Look for "-device xxx,yyy=zzz..." type changes > > b. DO_TEST & DO_TEST_CAPS_LATEST > In the previous patches, several experts suggest me using > DO_TEST_CAPS_LATEST, but the testcases will fail. I guess it may > be related to the qemu_capabilities check I mentioned above. I'm > not sure if this issue will disappeared when the first one is be > resolved. > Not entirely - mostly the current problem is how I believe you generated what you have. First, there's a slightly different naming scheme for the caps_latest files, but you can git mv what you have... $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.args $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args $ git mv tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.x86_64-latest.args for what you have as of patch3 will use the new/right naming scheme. Additionally when using the CAPS_LATEST option that means you're going to get "all" the caps enabled. IOW: just copying some existing similar output that doesn't use all the caps or CAPS_LATEST and modifying it to suit the needs of your change probably won't work very well because the output will include *all* the caps changes not just "some". So, if I was starting from scratch and just adding a new .args file, then what I usually do after "building" the patch with the qemu_command and qemuxml2argvtest changes is: $ touch tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.arg Repeat the touch for as many new tests you add - if you don't then running the test will fail and tell you the output file doesn't exist. Then use: $ VIR_TEST_REGENERATE_OUTPUT=1 tests/qemuxml2argvtest to "create" the new output. Then look at the output to make sure it has what I want. So let's see how you probably did this... Looking at the differences between your new XML files in patch1, I assume that you just copied: tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml then renamed to suit each test while changing each appropriately, since: $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 49d48 < <alignsize unit='KiB'>2048</alignsize> $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 49d48 < <pmem/> $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml 53d52 < <unarmed/> $ and likewise for the .args files (NB: my command is after the git mv) $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 15c15 < share=no,size=536870912,align=2097152 \ --- > share=no,size=536870912 \ $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 15c15 < share=no,size=536870912,pmem=on \ --- > share=no,size=536870912 \ $ diff tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.x86_64-latest.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.args 16c16 < -device nvdimm,node=0,unarmed=on,memdev=memnvdimm0,id=nvdimm0,slot=0 \ --- > -device nvdimm,node=0,memdev=memnvdimm0,id=nvdimm0,slot=0 \ So if I now run: $ VIR_TEST_REGENERATE_OUTPUT=1 tests/qemuxml2argvtest TEST: qemuxml2argvtest ........................................ 40 ........................................ 80 ........................................ 120 ........................................ 160 ........................................ 200 ........................................ 240 ........................................ 280 ........................................ 320 ........................................ 360 ........................................ 400 ........................................ 440 ........................................ 480 ........................................ 520 ........................................ 560 ........................................ 600 ........................................ 640 ........................................ 680 ........................................ 720 ........................................ 760 ....................!!!................. 800 ........................................ 840 ........................................ 880 ....... 887 FAIL You'll note the FAIL and the 3 ! (bangs). That indicates 3 tests that have changed which a git diff will show. If you search that output for a difference on what changed in each of your new commands, you'll note that specific output for the nvdimm didn't change only other things were adjusted (which are all normal for a full capability run). I'll provide some feedback in the other patches, but you still have a bit to go. John > Besides, the whole nvdimm stuff do not introduce enough qemu_capabilities > check and do not use DO_TEST_CAPS_LATEST. Maybe it is better to do these > modification in another patch set. Or we can rely on qemu errors, it's just > what libvirt do currently. What' your comments? > > Thank you in advance. > > Regards, > Luyao Zhong > > Luyao Zhong (4): > nvdimm: introduce more config elements into xml for NVDIMM memory > nvdimm: add nvdimm-related qemucapabilities check > nvdimm: update qemu command-line generating for NVDIMM memory > nvdimm: update news.xml > > docs/formatdomain.html.in | 80 ++++++++++++++++++---- > docs/news.xml | 9 +++ > docs/schemas/domaincommon.rng | 23 ++++++- > src/conf/domain_conf.c | 61 +++++++++++++++-- > src/conf/domain_conf.h | 3 + > src/qemu/qemu_capabilities.c | 8 ++- > src/qemu/qemu_capabilities.h | 4 ++ > src/qemu/qemu_command.c | 32 +++++++++ > tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + > tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 + > tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 + > tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + > tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + > tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 + > tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 + > .../memory-hotplug-nvdimm-align.args | 31 +++++++++ > .../memory-hotplug-nvdimm-align.xml | 58 ++++++++++++++++ > .../memory-hotplug-nvdimm-pmem.args | 31 +++++++++ > .../memory-hotplug-nvdimm-pmem.xml | 58 ++++++++++++++++ > .../memory-hotplug-nvdimm-unarmed.args | 31 +++++++++ > .../memory-hotplug-nvdimm-unarmed.xml | 58 ++++++++++++++++ > tests/qemuxml2argvtest.c | 11 +++ > .../memory-hotplug-nvdimm-align.xml | 1 + > .../memory-hotplug-nvdimm-pmem.xml | 1 + > .../memory-hotplug-nvdimm-unarmed.xml | 1 + > tests/qemuxml2xmltest.c | 3 + > 30 files changed, 491 insertions(+), 26 deletions(-) > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.args > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.args > create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-unarmed.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-pmem.xml > create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-unarmed.xml > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
