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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
