[libvirt] [RFC v3 0/4] update NVDIMM support
Luyao Zhong
luyao.zhong at intel.com
Fri Dec 14 08:17:38 UTC 2018
On 2018/12/14 上午9:06, John Ferlan wrote:
>
> 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 at redhat.com 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.
>
Thank you for pointing out these issues. I'll refer to the docs to avoid
these mistakes.
> 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.
>
Yes, I thought I can send RFC for collecting comments until the code is
perfect. It seems I was wrong. :D
Next version will be "PATCH v4".
>>
>> 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
>
Thank you for the details. I have done this part and it seems work well.
I modified the *.replies manually because the file didn't introduce
nvdimm properties like this:
{
"execute": "device-list-properties",
"arguments": {
"typename": "nvdimm"
},
"id": "libvirt-35"
}
{
"return": [
{
"name": "unarmed",
"type": "bool"
},
....
],
"id": "libvirt-35"
}
>>
>> 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 \
>
>
Thank you so much for the details. But I couldn't get the result we
expected using the 'diff' command, because the existed tests about
nvdimm use DO_TEST but not DO_CAPS_LATEST_TEST, so there'll be many
mismatches. I guess it's better to change them to the same
DO_CAPS_LATEST_TEST first.
> 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
>
Thank you again for your comments.
Luyao Zhong
>> 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
>>
More information about the libvir-list
mailing list