[libvirt] [RFC v2 2/3] nvdimm: update qemu command-line generating for NVDIMM memory

[Luyao Zhong]

On 2018/11/29 下午4:52, Peter Krempa wrote:
> On Thu, Nov 29, 2018 at 12:08:58 +0800, Luyao Zhong wrote:
>> On 2018/11/28 下午10:32, Peter Krempa wrote:
>>> On Wed, Nov 28, 2018 at 22:09:01 +0800, Luyao Zhong wrote:
>>>> According to the result parsing from xml, add corresponding properties
>>>> into QEMU command line, including 'align', 'pmem', 'unarmed' and
>>>> 'nvdimm-persistence'.
>>>>
>>>> Signed-off-by: Luyao Zhong <luyao.zhong at intel.com>
>>>> ---
> 
> [...]
> 
>>> This is completely broken. This code just clears the capability bits for
>>> older versions but really does never set them anywhere. So they'll never
>>> be present in real caps.
>>>
>>> That is visible that in the fact that this patch does not trigger an
>>> update of capability output test files, which it should.
>>>
>>> Additionally these really should check the presence of the fields in the
>>> device properties. For properties of object memory-backend-file we
>>> already call the appropriate qom-list-properties, so they will be
>>> trivial to modify.
>>>
>>> For the nvdimm property it will be slightly more hassle, but we really
>>> don't want to see version number based capabilities.
>>>
>> Sorry, I'm a very new libvirt developer and not very familiar with libvirt,
>> so I feel a little confused about this. I don't know how to add a new qemu
>> capability correctly, could you give an example as my reference？ Thank you
>> in advance.
> 
> The properties of memory-backed-file are automatically detected
> declaratively by adding the property and required flag into the
> virQEMUCapsObjectPropsMemoryBackendFile array.
> 
> Device properties have a similar approach but we don't have anything for
> nvdimm yet. You'll need to add nvdimm into virQEMUCapsDeviceProps[] with
> appropriate arrays for detecting the features.
> 
> Note that the above will trigger test failures in the capabilities test
> as it will add an additional command into the query procedure.
> 
> You'll need to regenerate/fix the test data for any version of qemu we
> have in tests/qemucapabilitiesdata/caps_* which supports NVDIMM.
> 
> Note that only relevant changes should be included in such regeneration,
> e.g. if you have a different CPU than the original files, the CPU
> related data should not be changed.
> 
> Regeneratin of the *.replies files can be done with tests/qemucapsprobe
> tool, or if you opt to add the relevant sections manually, you can use
> tests/qemucapsfixreplies to fix the numbering of the commands.
> 
Thank you so much for these details. I learned a lot from the comments.

>> Besides, I noticed that the one previous patch 'Introduce label-size for
>> NVDIMMs' (See e433546bef6ff5be92aa0cfca7794fff67c80cac) do not touch the
>> qemu capabilities things, Can I do like that?
> 
> I don't really know when that field was introduced. If it was there from
> the time NVDIMM was added into qemu, no capability bit is necessary.
> 
No, the 'label-size' later than NVDIMM.

> Also in some cases, when the feture is really niche and not expected to
> be widely used (as in fact the whole nvdimm stuff is) we can skip that
> and just rely on errors from qemu.
> 
So you recommend keeping this way to rely on qemu errors?