2:17 a.m.
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(a)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
>