7:31 a.m.
On 03/07/2017 05:55 PM, John Ferlan wrote:
On 02/27/2017 08:19 AM, Michal Privoznik wrote:
> For NVDIMM devices it is optionally possible to specify the size
> of internal storage for namespaces. Namespaces are a feature that
> allows users to partition the NVDIMM for different uses.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> docs/formatdomain.html.in | 9 ++++
> docs/schemas/domaincommon.rng | 7 +++
> src/conf/domain_conf.c | 19 +++++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 3 ++
> .../qemuxml2argv-memory-hotplug-nvdimm-label.args | 26 ++++++++++
> .../qemuxml2argv-memory-hotplug-nvdimm-label.xml | 59 ++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 2 +
> .../qemuxml2xmlout-memory-hotplug-nvdimm-label.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 10 files changed, 128 insertions(+)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-label.args
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-label.xml
> create mode 120000
tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm-label.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 00c0df2ce..4a078b577 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7038,6 +7038,9 @@ qemu-kvm -net nic,model=? /dev/null
> <target>
> <size unit='KiB'>524288</size>
> <node>1</node>
> + <label>
> + <size unit='KiB'>128</size>
> + </label>
> </target>
> </memory>
> </devices>
> @@ -7117,6 +7120,12 @@ qemu-kvm -net nic,model=? /dev/null
> attach the memory to. The element shall be used only if the guest has
> NUMA nodes configured.
> </p>
> + <p>
> + For NVDIMM type devices one can optionally use
> + <code>label</code> and its subelement
<code>size</code>
> + to configure the size of namespaces label storage
> + within the NVDIMM module.
and the "unit=" is also required, right?
No. By default the unit is KiB. If users want to use different one, they
have to specify it in @unit attribute. This is just like domain memory.
That's why we can use the same parsing function.
> + </p>
> </dd>
> </dl>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 78195aae9..aafc731f5 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4800,6 +4800,13 @@
> <ref name="unsignedInt"/>
> </element>
> </optional>
> + <optional>
> + <element name="label">
> + <element name="size">
> + <ref name="scaledInteger"/>
> + </element>
> + </element>
> + </optional>
> </interleave>
> </element>
> </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a31114cc7..7840f8e02 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13824,6 +13824,18 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
> &def->size, true, false) < 0)
> goto cleanup;
>
> + if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> + if (virDomainParseMemory("./label/size",
"./label/size/@unit", ctxt,
> + &def->labelsize, false, false) < 0)
So one could provide a fairly large size for this?
Yeah, if they have enough (disk/actual nvdimm) space. Now that I'm
thinking about this, maybe we can at least check if the label-size is
not greater than the size of nvdimm itself.
Why is '@required' false?
Because it is not required to specify label size.
> + goto cleanup;
> +
> + if (def->labelsize && def->labelsize < 128) {
This makes it seem labelsize is optional, but it's not clear (to me at
least) from the description above whether must provide the size as well
as the label.
Of course as I read on - labelsize is expected.... Let's face it, if
label is provided and labelsize is 0, then well not much is going to be
allowed is it?
I think you can still use it as a data storage inside the guest. Labels
were not introduced in qemu at the same time as NVDIMM, so clearly you
can use NVDIMMs even without labels.
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("nvdimm label must be at least 128KiB"));
> + goto cleanup;
> + }
> + }
> +
> ret = 0;
>
> cleanup:
> @@ -22663,6 +22675,13 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, "<size
unit='KiB'>%llu</size>\n", def->size);
> if (def->targetNode >= 0)
> virBufferAsprintf(buf, "<node>%d</node>\n",
def->targetNode);
> + if (def->labelsize) {
> + virBufferAddLit(buf, "<label>\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<size
unit='KiB'>%llu</size>\n", def->labelsize);
There's no check here if labelsize wasn't provided.
Yes, there is.
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</label>\n");
> + }
>
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</target>\n");
Similar comments from before about ABI...
This might actually require ABI check as the label-size is visible
inside the guest. Will update it.
Additionally if NVDIMM's are included in the total memory allocation's
(from my comments in patch2), wouldn't the size of this label need to
also be included?
No. This is not some additional memory. Label size specifies the size of
a portion of NVDIMM that is used to store labels. For instance, if I
have 512MiB NVDIMM and set label-size to 2MiB, there's only 510MiB
available for data. If the label-size is 128MiB then there's only 128MiB
left for data.
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 87516ca69..0e68f5770 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2013,6 +2013,7 @@ struct _virDomainMemoryDef {
> int model; /* virDomainMemoryModel */
> int targetNode;
> unsigned long long size; /* kibibytes */
> + unsigned long long labelsize; /* kibibytes; valid only for NVDIMM */
>
> virDomainDeviceInfo info;
> };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 287387055..91ace8e38 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3440,6 +3440,9 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
> if (mem->targetNode >= 0)
> virBufferAsprintf(&buf, "node=%d,", mem->targetNode);
>
> + if (mem->labelsize)
> + virBufferAsprintf(&buf, "label-size=%llu,",
mem->labelsize * 1024);
> +
And this code checks for labelsize using the "assumption" (I suppose
that if label, then we have a labelsize too.
No. If mem->labelsize then mem->labelsize. Or maybe I'm misunderstanding
something?
Michal