Re: [PATCH v2 02/14] Introduce NVDIMM memory model

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 02/27/2017 08:19 AM, Michal Privoznik wrote:
> NVDIMM is new type of memory introduced into QEMU 2.6. The idea
> is that we have a Non-Volatile memory module that keeps the data
> persistent across domain reboots.
> 
> At the domain XML level, we already have some representation of
> 'dimm' modules. Long story short, we have <memory/> element that
> lives under <devices/>. Now, the element even has @model
> attribute which we can use to introduce new memory type:

Starting with "Long story short..."

how about instead:

NVDIMM will utilize the existing <memory/> element that lives under
<devices/> by adding a new attribute 'nvdimm' to the existing @model and
introduce a new <path/> element for <source/> while reusing other
fields. The resulting XML would appear as:

> 
>     <memory model='nvdimm'>
>       <source>
>         <path>/tmp/nvdimm</path>
>       </source>
>       <target>
>         <size unit='KiB'>523264</size>
>         <node>0</node>
>       </target>
>       <address type='dimm' slot='0'/>
>     </memory>
> 
> So far, this is just a XML parser/formatter extension. QEMU
> driver implementation is in the next commit.
> 
> For more info on NVDIMM visit the following web page:
> 
>     http://pmem.io/

Are there "limitations" to the number of nvdimm's that could be
supported in a guest? Mostly curious, but it's one of those I thought I
read something type, but cannot remember where type things...

> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  docs/formatdomain.html.in                          | 55 ++++++++----
>  docs/schemas/domaincommon.rng                      | 32 ++++---
>  src/conf/domain_conf.c                             | 97 ++++++++++++++++------
>  src/conf/domain_conf.h                             |  2 +
>  src/qemu/qemu_command.c                            |  6 ++
>  src/qemu/qemu_domain.c                             |  5 ++
>  .../qemuxml2argv-memory-hotplug-nvdimm.xml         | 56 +++++++++++++
>  .../qemuxml2xmlout-memory-hotplug-nvdimm.xml       |  1 +
>  tests/qemuxml2xmltest.c                            |  1 +
>  9 files changed, 204 insertions(+), 51 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml
>  create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 02ce7924c..b76905cdc 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7007,7 +7007,6 @@ qemu-kvm -net nic,model=? /dev/null
>          guests' memory resource needs.
>  
>          Some hypervisors may require NUMA configured for the guest.
> -      <span class="since">Since 1.2.14</span>
>      </p>
>  
>      <p>
> @@ -7032,6 +7031,15 @@ qemu-kvm -net nic,model=? /dev/null
>        &lt;node&gt;1&lt;/node&gt;
>      &lt;/target&gt;
>    &lt;/memory&gt;
> +  &lt;memory model='nvdimm'&gt;
> +    &lt;source&gt;
> +      &lt;path&gt;/tmp/nvdimm&lt;/path&gt;
> +    &lt;/source&gt;
> +    &lt;target&gt;
> +      &lt;size unit='KiB'&gt;524288&lt;/size&gt;
> +      &lt;node&gt;1&lt;/node&gt;
> +    &lt;/target&gt;
> +  &lt;/memory&gt;
>  &lt;/devices&gt;
>  ...
>  </pre>
> @@ -7039,28 +7047,47 @@ qemu-kvm -net nic,model=? /dev/null
>        <dt><code>model</code></dt>
>        <dd>
>          <p>
> -          Currently only the <code>dimm</code> model is supported in order to
> -          add a virtual DIMM module to the guest.
> +          Provide <code>dimm</code> to add a virtual DIMM module to the guest.
> +          <span class="since">Since 1.2.14</span>
> +          Provide <code>nvdimm</code> model adds a Non-Volatile DIMM
> +          module. <span class="since">Since 3.1.0</span>

will be at least 3.2.0

>          </p>
>        </dd>
>  
>        <dt><code>source</code></dt>
>        <dd>
>          <p>
> -          The optional source element allows to fine tune the source of the
> -          memory used for the given memory device. If the element is not
> -          provided defaults configured via <code>numatune</code> are used.
> +          For model <code>dimm</code> this element is optional and allows to
> +          fine tune the source of the memory used for the given memory device.
> +          If the element is not provided defaults configured via
> +          <code>numatune</code> are used. If <code>dimm</code> is provided,
> +          then the following optional elements can be provided as well:
>          </p>
> -        <p>
> -          <code>pagesize</code> can optionally be used to override the default
> -          host page size used for backing the memory device.
>  
> -          The configured value must correspond to a page size supported by the
> -          host.
> -        </p>
> +        <dl>
> +          <dt><code>pagesize</code></dt>
> +          <dd>
> +            <p>
> +              This element can optionally be used to override the default

I think at this point optionally is redundant since the above says "the
following optional elements":

> +              host page size used for backing the memory device.
> +              The configured value must correspond to a page size supported by the
> +              host.
> +            </p>
> +          </dd>
> +
> +          <dt><code>nodemask</code></dt>
> +          <dd>
> +            <p>
> +              This element can optionally be used to override the default

same w/r/t optionally

> +              set of NUMA nodes where the memory would be allocated.
> +            </p>
> +          </dd>
> +        </dl>
> +
>          <p>
> -          <code>nodemask</code> can optionally be used to override the default
> -          set of NUMA nodes where the memory would be allocated.
> +          For model <code>nvdimm</code> this element is mandatory and has a
> +          single child element <code>path</code> that represents a path
> +          in the host that backs the nvdimm module in the guest.
>          </p>
>        </dd>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index c64544ac4..fafd3e982 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4737,6 +4737,7 @@
>        <attribute name="model">
>          <choice>
>            <value>dimm</value>
> +          <value>nvdimm</value>
>          </choice>
>        </attribute>
>        <interleave>
> @@ -4756,18 +4757,27 @@
>  
>    <define name="memorydev-source">
>      <element name="source">
> -      <interleave>
> -        <optional>
> -          <element name="pagesize">
> -            <ref name="scaledInteger"/>
> +      <choice>
> +        <group>
> +          <interleave>
> +            <optional>
> +              <element name="pagesize">
> +                <ref name="scaledInteger"/>
> +              </element>
> +            </optional>
> +            <optional>
> +              <element name="nodemask">
> +                <ref name="cpuset"/>
> +              </element>
> +            </optional>
> +          </interleave>
> +        </group>
> +        <group>
> +          <element name="path">
> +            <text/>

I would think this should be:

<ref name="absFilePath"/>

>            </element>
> -        </optional>
> -        <optional>
> -          <element name="nodemask">
> -            <ref name="cpuset"/>
> -          </element>
> -        </optional>
> -      </interleave>
> +        </group>
> +      </choice>
>      </element>
>    </define>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 97d42fe99..4ffca7dc8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -858,8 +858,11 @@ VIR_ENUM_DECL(virDomainBlockJob)
>  VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST,
>                "", "", "copy", "", "active-commit")
>  
> -VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST,
> -              "", "dimm")
> +VIR_ENUM_IMPL(virDomainMemoryModel,
> +              VIR_DOMAIN_MEMORY_MODEL_LAST,
> +              "",
> +              "dimm",
> +              "nvdimm")
>  
>  VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
>                "ivshmem",
> @@ -2418,6 +2421,7 @@ void virDomainMemoryDefFree(virDomainMemoryDefPtr def)
>      if (!def)
>          return;
>  
> +    VIR_FREE(def->path);
>      virBitmapFree(def->sourceNodes);
>      virDomainDeviceInfoClear(&def->info);
>      VIR_FREE(def);
> @@ -13755,20 +13759,36 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
>      xmlNodePtr save = ctxt->node;
>      ctxt->node = node;
>  
> -    if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt,
> -                             &def->pagesize, false, false) < 0)
> -        goto cleanup;
> -
> -    if ((nodemask = virXPathString("string(./nodemask)", ctxt))) {
> -        if (virBitmapParse(nodemask, &def->sourceNodes,
> -                           VIR_DOMAIN_CPUMASK_LEN) < 0)
> +    switch ((virDomainMemoryModel) def->model) {
> +    case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +        if (virDomainParseMemory("./pagesize", "./pagesize/@unit", ctxt,
> +                                 &def->pagesize, false, false) < 0)
>              goto cleanup;
>  
> -        if (virBitmapIsAllClear(def->sourceNodes)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("Invalid value of 'nodemask': %s"), nodemask);
> +        if ((nodemask = virXPathString("string(./nodemask)", ctxt))) {
> +            if (virBitmapParse(nodemask, &def->sourceNodes,
> +                               VIR_DOMAIN_CPUMASK_LEN) < 0)
> +                goto cleanup;
> +
> +            if (virBitmapIsAllClear(def->sourceNodes)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("Invalid value of 'nodemask': %s"), nodemask);
> +                goto cleanup;
> +            }
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +        if (!(def->path = virXPathString("string(./path)", ctxt))) {
> +            virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                           _("path is required for model nvdimm'"));
>              goto cleanup;
>          }
> +        break;
> +
> +    case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +    case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +        break;
>      }
>  
>      ret = 0;
> @@ -15173,12 +15193,25 @@ virDomainMemoryFindByDefInternal(virDomainDefPtr def,
>              tmp->size != mem->size)
>              continue;
>  
> -        /* source stuff -> match with device */
> -        if (tmp->pagesize != mem->pagesize)
> -            continue;
> +        switch ((virDomainMemoryModel) mem->model) {
> +        case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +            /* source stuff -> match with device */
> +            if (tmp->pagesize != mem->pagesize)
> +                continue;
>  
> -        if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes))
> -            continue;
> +            if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes))
> +                continue;
> +            break;
> +
> +        case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +            if (STRNEQ(tmp->path, mem->path))
> +                continue;
> +            break;
> +
> +        case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +        case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +            break;
> +        }
>  
>          break;
>      }
> @@ -22571,23 +22604,35 @@ virDomainMemorySourceDefFormat(virBufferPtr buf,
>      char *bitmap = NULL;
>      int ret = -1;
>  
> -    if (!def->pagesize && !def->sourceNodes)
> +    if (!def->pagesize && !def->sourceNodes && !def->path)
>          return 0;
>  
>      virBufferAddLit(buf, "<source>\n");
>      virBufferAdjustIndent(buf, 2);
>  
> -    if (def->sourceNodes) {
> -        if (!(bitmap = virBitmapFormat(def->sourceNodes)))
> -            goto cleanup;
> +    switch ((virDomainMemoryModel) def->model) {
> +    case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +        if (def->sourceNodes) {
> +            if (!(bitmap = virBitmapFormat(def->sourceNodes)))
> +                goto cleanup;
>  
> -        virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap);
> +            virBufferAsprintf(buf, "<nodemask>%s</nodemask>\n", bitmap);
> +        }
> +
> +        if (def->pagesize)
> +            virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n",
> +                              def->pagesize);
> +        break;
> +
> +    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +        virBufferAsprintf(buf, "<path>%s</path>\n", def->path);
> +        break;

This will need to be escaped properly (any weirdness with ,, too?).
FWIW: I'm using 'romfile' (the nvram property) as my reference.

> +
> +    case VIR_DOMAIN_MEMORY_MODEL_NONE:
> +    case VIR_DOMAIN_MEMORY_MODEL_LAST:
> +        break;
>      }
>  
> -    if (def->pagesize)
> -        virBufferAsprintf(buf, "<pagesize unit='KiB'>%llu</pagesize>\n",
> -                          def->pagesize);
> -
>      virBufferAdjustIndent(buf, -2);
>      virBufferAddLit(buf, "</source>\n");
>  

So no need
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1e53cc328..dc949d3c9 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1996,6 +1996,7 @@ struct _virDomainRNGDef {
>  typedef enum {
>      VIR_DOMAIN_MEMORY_MODEL_NONE,
>      VIR_DOMAIN_MEMORY_MODEL_DIMM, /* dimm hotpluggable memory device */
> +    VIR_DOMAIN_MEMORY_MODEL_NVDIMM, /* nvdimm memory device */
>  
>      VIR_DOMAIN_MEMORY_MODEL_LAST
>  } virDomainMemoryModel;
> @@ -2004,6 +2005,7 @@ struct _virDomainMemoryDef {
>      /* source */
>      virBitmapPtr sourceNodes;
>      unsigned long long pagesize; /* kibibytes */
> +    char *path;

Since it's "hard" to find path in sources in a simple (hah) search, can
this be nvdimm_path or something more specific?

I would also think there'd need to be some Mem ABI Stability check added
in virDomainMemoryDefCheckABIStability that the src/dst path's are the same.

Searching on 'nmems':

 1. Will virDomainDefPostParseMemory need any adjustment to not account
for this type of memory or should it be included?

 2. Similar for virDomainDefGetMemoryInitial

 3. Will alignment be needed? qemuDomainAlignMemorySizes



>  
>      /* target */
>      int model; /* virDomainMemoryModel */
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7c52712d1..f628a9929 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3421,6 +3421,12 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
>  
>          break;
>  
> +    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                       _("nvdimm not supported yet"));
> +        return NULL;
> +        break;
> +
>      case VIR_DOMAIN_MEMORY_MODEL_NONE:
>      case VIR_DOMAIN_MEMORY_MODEL_LAST:
>          break;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c187214dc..5ec610564 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5910,6 +5910,11 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
>          }
>          break;
>  
> +    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("nvdimm hotplug not supported yet"));
> +        return -1;
> +
>      case VIR_DOMAIN_MEMORY_MODEL_NONE:
>      case VIR_DOMAIN_MEMORY_MODEL_LAST:
>          return -1;

What about migration support?  That would assume the file exists in both
places or is somehow passed from source to target, right?

John

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml
> new file mode 100644
> index 000000000..1578db453
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml
> @@ -0,0 +1,56 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
> +  <memory unit='KiB'>1267710</memory>
> +  <currentMemory unit='KiB'>1267710</currentMemory>
> +  <vcpu placement='static' cpuset='0-1'>2</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <idmap>
> +    <uid start='0' target='1000' count='10'/>
> +    <gid start='0' target='1000' count='10'/>
> +  </idmap>
> +  <cpu>
> +    <topology sockets='2' cores='1' threads='1'/>
> +    <numa>
> +      <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/>
> +    </numa>
> +  </cpu>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <disk type='block' device='disk'>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='ide' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> +    </controller>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> +    </controller>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> +    </memballoon>
> +    <memory model='nvdimm'>
> +      <source>
> +        <path>/tmp/nvdimm</path>
> +      </source>
> +      <target>
> +        <size unit='KiB'>523264</size>
> +        <node>0</node>
> +      </target>
> +      <address type='dimm' slot='0'/>
> +    </memory>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml
> new file mode 120000
> index 000000000..4cac477a9
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-nvdimm.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 4353ad245..e1c341dd5 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1078,6 +1078,7 @@ mymain(void)
>      DO_TEST("memory-hotplug", NONE);
>      DO_TEST("memory-hotplug-nonuma", NONE);
>      DO_TEST("memory-hotplug-dimm", NONE);
> +    DO_TEST("memory-hotplug-nvdimm", NONE);
>      DO_TEST("net-udp", NONE);
>  
>      DO_TEST("video-virtio-gpu-device", NONE);
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux