Re: [libvirt] [RFC 1/3] xml: introduce more config elements for NVDIMM memory

On 10/16/18 10:21 PM, Luyao Zhong wrote: caveat: I didn't research too deeply into each of these options, but I'll give you some feedback from the aspect of how you should formulate your patches. Rather than essentially replicate what was added in formatdomain provide the examples here and a summary of what the proposed XML would "look like", e.g. Since 'alignsize' and 'pmem' seem to be related specifically to memory mapped files: <memory model='nvdimm'> <source> <path>/tmp/nvdimm</path> <alignsize unit='KiB'>2048</alignsize> <pmem>on</pmem> </source> ... </memory> The <alignsize> would seem to similar in description to the existing <pagesize> - although yes for a different model type..... The <pmem> seems to have two states "on" or "off", thus having just <pmem/> similar to perhaps how nosharepages is handled. Since they're both memory mapped related changes, keep them together from conf, xml, rng, etc. Then the "next" patch would be to do the qemu and capability changes. With the last being any virsh changes to allow modification on the command line (if possible). The "persistence" seems to be less of a <memory> option and more of a <machine> option, true/false? It would thus need to be separated from the others. Still similar to the first one, separate patch for conf, xml, rng, html... Then patch for qemu... The "unarmed" is a <target> option since it's being added in patch2 after node and label are processed, so it should be separate from the <source> and <machine> option patches. If it's just going to have 2 states, then it doesn't need <unarmed>on</unarmed> - instead it could just be <unarmed/>. Not a commit message worthy... Place it under the ---... As noted above, but perhaps difficult to determine - the html.in, .rng, domain_conf, xml2xml tests, etc. should be in one patch. Then another patch for the qemu, capabilities, xml2argv tests, etc. in the next patch. Then if you're going to add virsh options a separate patch for that (although probably doesn't apply here). After each patch, running make check syntax-check needs to pass. You'd need to add a since tags here for the "new" work...e.g.: <span class='since'>since 4.10.0</span> Whenever something new is added for a specific version, that's what's necessary. ^ there's two spaces between backend and is The probably should be reworded in syntax more suitable for libvirt's usage without including QEMU details. The pmem one in particular may be hard to describe... The fact that it doesn't work without libpmem does raise a couple of warning flags. I would just say that if <pmem/> is there, then it's on if it's not, then it's off. Internally that then isn't a tristate, it just an on/off. "mem-ctrl" means the platform That's a hard read - someone has to know about ACPI 6.2 Errata A and whatever it is and where to find it? Also, I'm not sure this belongs on the NVDIMM line... Allowing it means more than one could use this and they could use separate values, each then being placed on the command line and then you don't know what goes with what. That is what happens if we have <memory model='nvdimm'> ... <target> ... <persistence>mem-ctrl</persistence> ... </memory> <memory model='nvdimm'> ... <target> ... <persistence>cpu</persistence> ... </memory> What is NFIT? Add a space before Currently What is DAX? unarmed will be similar to pmem... Once these are properly separated it'll be easier to review the text in more detail. But again, like I noted earlier try to use more generic terms and avoid specifically calling out QEMU. Although that may be difficult - you have to consider the audience - they just want to know what the feature does in general and how it will help them or make things better for them. The details of what we rename it to under the covers hides the complexity. Again, I'd follow the nosharepages example here. While the data is correct, the placement isn't... Similar here to be like nosharepages. John