Nothing special is happening here. All important changes were
done when for 'virtio-pmem' (adjusting the code to put virtio
memory on PCI bus, generating alias using
qemuDomainDeviceAliasIndex(). The only bit that might look
suspicious is no prealloc for virtio-mem. But if you think about
it, the whole purpose of this device is to change amount of
memory exposed to guest on the fly. There is no point in locking
the whole backend in memory.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_alias.c                         |  9 +++-
 src/qemu/qemu_command.c                       | 24 +++++++++--
 ...mory-hotplug-virtio-mem.x86_64-latest.args | 41 +++++++++++++++++++
 tests/qemuxml2argvtest.c                      |  1 +
 4 files changed, 70 insertions(+), 5 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 79e8953b2f..81a1e7eeed 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -475,8 +475,11 @@ qemuDeviceMemoryGetAliasID(virDomainDef *def,
     size_t i;
     int maxidx = 0;
 
-    /* virtio-pmem goes onto PCI bus and thus DIMM address is not valid */
-    if (!oldAlias && mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM)
+    /* virtio-pmem and virtio-mem go onto PCI bus and thus DIMM address is not
+     * valid */
+    if (!oldAlias &&
+        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&
+        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM)
         return mem->info.addr.dimm.slot;
 
     for (i = 0; i < def->nmems; i++) {
@@ -523,6 +526,8 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
         prefix = "virtiopmem";
         break;
     case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
+        prefix = "virtiomem";
+        break;
     case VIR_DOMAIN_MEMORY_MODEL_NONE:
     case VIR_DOMAIN_MEMORY_MODEL_LAST:
     default:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 594e5643b1..91f3094ac8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3136,9 +3136,19 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,
         virJSONValueObjectAdd(props, "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0)
         return -1;
 
-    if (!priv->memPrealloc &&
-        virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
-        return -1;
+    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM) {
+        /* Explicitly disable prealloc for virtio-mem */
+        if (priv->memPrealloc &&
+            virJSONValueObjectAppendBoolean(props, "prealloc", 0) < 0)
+            return -1;
+        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MEMORY_BACKEND_RESERVE) &&
+            virJSONValueObjectAppendBoolean(props, "reserve", 0) < 0)
+            return -1;
+    } else {
+        if (!priv->memPrealloc &&
+            virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
+            return -1;
+    }
 
     if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
         return -1;
@@ -3318,6 +3328,9 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
         break;
 
     case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
+        device = "virtio-mem-pci";
+        break;
+
     case VIR_DOMAIN_MEMORY_MODEL_NONE:
     case VIR_DOMAIN_MEMORY_MODEL_LAST:
     default:
@@ -3334,6 +3347,11 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
     if (mem->labelsize)
         virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
 
+    if (mem->blocksize) {
+        virBufferAsprintf(&buf, "block-size=%llu,", mem->blocksize * 1024);
+        virBufferAsprintf(&buf, "requested-size=%llu,", mem->requestedsize * 1024);
+    }
+
     if (mem->uuid) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
 
diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
new file mode 100644
index 0000000000..22ee1bc459
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
@@ -0,0 +1,41 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-i386 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \
+-machine pc,accel=kvm,usb=off,dump-guest-core=off \
+-cpu qemu64 \
+-m size=2095104k,slots=16,maxmem=1099511627776k \
+-overcommit mem-lock=off \
+-smp 2,sockets=2,dies=1,cores=1,threads=1 \
+-object '{"qom-type":"memory-backend-ram","id":"ram-node0","size":2145386496}' \
+-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
+-object '{"qom-type":"memory-backend-ram","id":"memvirtiomem0","reserve":false,"size":1073741824}' \
+-device virtio-mem-pci,node=0,block-size=2097152,requested-size=536870912,memdev=memvirtiomem0,id=virtiomem0,bus=pci.0,addr=0x2 \
+-object '{"qom-type":"memory-backend-file","id":"memvirtiomem1","mem-path":"/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1","reserve":false,"size":2147483648,"host-nodes":[1,2,3],"policy":"bind"}' \
+-device virtio-mem-pci,node=0,block-size=2097152,requested-size=1073741824,memdev=memvirtiomem1,id=virtiomem1,bus=pci.0,addr=0x3 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \
+-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \
+-audiodev id=audio1,driver=none \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 74af93b08f..23f4a8471f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3143,6 +3143,7 @@ mymain(void)
                  ARG_END);
     DO_TEST_CAPS_VER("memory-hotplug-virtio-pmem", "5.2.0");
     DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem");
+    DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem");
 
     DO_TEST("machine-aeskeywrap-on-caps",
             QEMU_CAPS_AES_KEY_WRAP,
-- 
2.32.0

On 13.09.21 16:52, Michal Privoznik wrote:
> Nothing special is happening here. All important changes were
> done when for 'virtio-pmem' (adjusting the code to put virtio
> memory on PCI bus, generating alias using
> qemuDomainDeviceAliasIndex(). The only bit that might look
> suspicious is no prealloc for virtio-mem. But if you think about
> it, the whole purpose of this device is to change amount of
> memory exposed to guest on the fly. There is no point in locking
> the whole backend in memory.

Do I understand correctly that we'll always try setting "reserve=off", 
and "prealloc=off"? That would be awesome :)

I do wonder if we want to warn or bail out if "priv->memPrealloc" is set 
but we are temporarily not able to support it -- as discussed, because 
virtio-mem in QEMU yet has to be taught about it.

FYI: QEMU will bail out if "reserve=off,prealloc=on" is set in combination.

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/qemu/qemu_alias.c                         |  9 +++-
>   src/qemu/qemu_command.c                       | 24 +++++++++--
>   ...mory-hotplug-virtio-mem.x86_64-latest.args | 41 +++++++++++++++++++
>   tests/qemuxml2argvtest.c                      |  1 +
>   4 files changed, 70 insertions(+), 5 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> 
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 79e8953b2f..81a1e7eeed 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -475,8 +475,11 @@ qemuDeviceMemoryGetAliasID(virDomainDef *def,
>       size_t i;
>       int maxidx = 0;
>   
> -    /* virtio-pmem goes onto PCI bus and thus DIMM address is not valid */
> -    if (!oldAlias && mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM)
> +    /* virtio-pmem and virtio-mem go onto PCI bus and thus DIMM address is not
> +     * valid */
> +    if (!oldAlias &&
> +        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&
> +        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM)
>           return mem->info.addr.dimm.slot;
>   
>       for (i = 0; i < def->nmems; i++) {
> @@ -523,6 +526,8 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
>           prefix = "virtiopmem";
>           break;
>       case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> +        prefix = "virtiomem";
> +        break;
>       case VIR_DOMAIN_MEMORY_MODEL_NONE:
>       case VIR_DOMAIN_MEMORY_MODEL_LAST:
>       default:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 594e5643b1..91f3094ac8 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3136,9 +3136,19 @@ qemuBuildMemoryBackendProps(virJSONValue **backendProps,
>           virJSONValueObjectAdd(props, "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0)
>           return -1;
>   
> -    if (!priv->memPrealloc &&
> -        virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
> -        return -1;
> +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM) {
> +        /* Explicitly disable prealloc for virtio-mem */
> +        if (priv->memPrealloc &&
> +            virJSONValueObjectAppendBoolean(props, "prealloc", 0) < 0)
> +            return -1;
> +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MEMORY_BACKEND_RESERVE) &&
> +            virJSONValueObjectAppendBoolean(props, "reserve", 0) < 0)
> +            return -1;
> +    } else {
> +        if (!priv->memPrealloc &&
> +            virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
> +            return -1;
> +    }
>   
>       if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
>           return -1;
> @@ -3318,6 +3328,9 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
>           break;
>   
>       case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> +        device = "virtio-mem-pci";
> +        break;
> +
>       case VIR_DOMAIN_MEMORY_MODEL_NONE:
>       case VIR_DOMAIN_MEMORY_MODEL_LAST:
>       default:
> @@ -3334,6 +3347,11 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
>       if (mem->labelsize)
>           virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
>   
> +    if (mem->blocksize) {
> +        virBufferAsprintf(&buf, "block-size=%llu,", mem->blocksize * 1024);
> +        virBufferAsprintf(&buf, "requested-size=%llu,", mem->requestedsize * 1024);
> +    }
> +
>       if (mem->uuid) {
>           char uuidstr[VIR_UUID_STRING_BUFLEN];
>   
> diff --git a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> new file mode 100644
> index 0000000000..22ee1bc459
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> @@ -0,0 +1,41 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> +USER=test \
> +LOGNAME=test \
> +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> +/usr/bin/qemu-system-i386 \
> +-name guest=QEMUGuest1,debug-threads=on \
> +-S \
> +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \
> +-machine pc,accel=kvm,usb=off,dump-guest-core=off \
> +-cpu qemu64 \
> +-m size=2095104k,slots=16,maxmem=1099511627776k \
> +-overcommit mem-lock=off \
> +-smp 2,sockets=2,dies=1,cores=1,threads=1 \
> +-object '{"qom-type":"memory-backend-ram","id":"ram-node0","size":2145386496}' \
> +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
> +-object '{"qom-type":"memory-backend-ram","id":"memvirtiomem0","reserve":false,"size":1073741824}' \
> +-device virtio-mem-pci,node=0,block-size=2097152,requested-size=536870912,memdev=memvirtiomem0,id=virtiomem0,bus=pci.0,addr=0x2 \
> +-object '{"qom-type":"memory-backend-file","id":"memvirtiomem1","mem-path":"/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1","reserve":false,"size":2147483648,"host-nodes":[1,2,3],"policy":"bind"}' \
> +-device virtio-mem-pci,node=0,block-size=2097152,requested-size=1073741824,memdev=memvirtiomem1,id=virtiomem1,bus=pci.0,addr=0x3 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot strict=on \
> +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
> +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \
> +-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \
> +-audiodev id=audio1,driver=none \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
> +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> +-msg timestamp=on
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 74af93b08f..23f4a8471f 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3143,6 +3143,7 @@ mymain(void)
>                    ARG_END);
>       DO_TEST_CAPS_VER("memory-hotplug-virtio-pmem", "5.2.0");
>       DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem");
> +    DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem");
>   
>       DO_TEST("machine-aeskeywrap-on-caps",
>               QEMU_CAPS_AES_KEY_WRAP,
> 


-- 
Thanks,

David / dhildenb

Hi Michal,
I tried to test the virtio-mem with upstream version v7.7.0-136-g9b49c2c6d3
adding the current patch
(with qemu-6.1.0-7.fc36.x86_64) -


<maxMemory slots='16' unit='KiB'>8388608</maxMemory>
  <memory unit='KiB'>1179648</memory>
  <currentMemory unit='KiB'>1179648</currentMemory>
 <memoryBacking>
    <allocation mode='immediate'/>
  </memoryBacking>
 ....
  <cpu mode='custom' match='exact' check='none'>
    <model fallback='forbid'>qemu64</model>
    <numa>
      <cell id='0' cpus='0-1' memory='1048576' unit='KiB' discard='yes'/>
    </numa>
  </cpu>
...
 <memory model='virtio-mem'>
      <source>
        <nodemask>0</nodemask>
        <pagesize unit='KiB'>2048</pagesize>
      </source>
      <target>
        <size unit='KiB'>131072</size>
        <node>0</node>
        <block unit='KiB'>2048</block>
        <requested unit='KiB'>131072</requested>
      </target>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
    </memory>

The vm was started successfully and there was no "prealloc: true"  in the
command line.
But I also found when <allocation mode='immediate'/> was added in the
domain and the "virtio-mem" device was not added, the qemu command line
also didn't have "prealloc:true". Is this correct? This result is
different in "libvirt-7.6.0-3.module+el8.5.0+12510+80564ecf.x86_64" &
"qemu-kvm-6.0.0-29.module+el8.5.0+12386+43574bac.x86_64".

Best Regards,
Jing Qi

On Tue, Sep 14, 2021 at 9:07 PM David Hildenbrand <david@redhat.com> wrote:

> On 13.09.21 16:52, Michal Privoznik wrote:
> > Nothing special is happening here. All important changes were
> > done when for 'virtio-pmem' (adjusting the code to put virtio
> > memory on PCI bus, generating alias using
> > qemuDomainDeviceAliasIndex(). The only bit that might look
> > suspicious is no prealloc for virtio-mem. But if you think about
> > it, the whole purpose of this device is to change amount of
> > memory exposed to guest on the fly. There is no point in locking
> > the whole backend in memory.
>
> Do I understand correctly that we'll always try setting "reserve=off",
> and "prealloc=off"? That would be awesome :)
>
> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
> but we are temporarily not able to support it -- as discussed, because
> virtio-mem in QEMU yet has to be taught about it.
>
> FYI: QEMU will bail out if "reserve=off,prealloc=on" is set in combination.
>
> >
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >   src/qemu/qemu_alias.c                         |  9 +++-
> >   src/qemu/qemu_command.c                       | 24 +++++++++--
> >   ...mory-hotplug-virtio-mem.x86_64-latest.args | 41 +++++++++++++++++++
> >   tests/qemuxml2argvtest.c                      |  1 +
> >   4 files changed, 70 insertions(+), 5 deletions(-)
> >   create mode 100644
> tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> >
> > diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> > index 79e8953b2f..81a1e7eeed 100644
> > --- a/src/qemu/qemu_alias.c
> > +++ b/src/qemu/qemu_alias.c
> > @@ -475,8 +475,11 @@ qemuDeviceMemoryGetAliasID(virDomainDef *def,
> >       size_t i;
> >       int maxidx = 0;
> >
> > -    /* virtio-pmem goes onto PCI bus and thus DIMM address is not valid
> */
> > -    if (!oldAlias && mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM)
> > +    /* virtio-pmem and virtio-mem go onto PCI bus and thus DIMM address
> is not
> > +     * valid */
> > +    if (!oldAlias &&
> > +        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&
> > +        mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM)
> >           return mem->info.addr.dimm.slot;
> >
> >       for (i = 0; i < def->nmems; i++) {
> > @@ -523,6 +526,8 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
> >           prefix = "virtiopmem";
> >           break;
> >       case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> > +        prefix = "virtiomem";
> > +        break;
> >       case VIR_DOMAIN_MEMORY_MODEL_NONE:
> >       case VIR_DOMAIN_MEMORY_MODEL_LAST:
> >       default:
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 594e5643b1..91f3094ac8 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3136,9 +3136,19 @@ qemuBuildMemoryBackendProps(virJSONValue
> **backendProps,
> >           virJSONValueObjectAdd(props,
> "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0)
> >           return -1;
> >
> > -    if (!priv->memPrealloc &&
> > -        virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
> > -        return -1;
> > +    if (mem->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM) {
> > +        /* Explicitly disable prealloc for virtio-mem */
> > +        if (priv->memPrealloc &&
> > +            virJSONValueObjectAppendBoolean(props, "prealloc", 0) < 0)
> > +            return -1;
> > +        if (virQEMUCapsGet(priv->qemuCaps,
> QEMU_CAPS_MEMORY_BACKEND_RESERVE) &&
> > +            virJSONValueObjectAppendBoolean(props, "reserve", 0) < 0)
> > +            return -1;
> > +    } else {
> > +        if (!priv->memPrealloc &&
> > +            virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL)
> < 0)
> > +            return -1;
> > +    }
> >
> >       if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL)
> < 0)
> >           return -1;
> > @@ -3318,6 +3328,9 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
> >           break;
> >
> >       case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> > +        device = "virtio-mem-pci";
> > +        break;
> > +
> >       case VIR_DOMAIN_MEMORY_MODEL_NONE:
> >       case VIR_DOMAIN_MEMORY_MODEL_LAST:
> >       default:
> > @@ -3334,6 +3347,11 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
> >       if (mem->labelsize)
> >           virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize *
> 1024);
> >
> > +    if (mem->blocksize) {
> > +        virBufferAsprintf(&buf, "block-size=%llu,", mem->blocksize *
> 1024);
> > +        virBufferAsprintf(&buf, "requested-size=%llu,",
> mem->requestedsize * 1024);
> > +    }
> > +
> >       if (mem->uuid) {
> >           char uuidstr[VIR_UUID_STRING_BUFLEN];
> >
> > diff --git
> a/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> > new file mode 100644
> > index 0000000000..22ee1bc459
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/memory-hotplug-virtio-mem.x86_64-latest.args
> > @@ -0,0 +1,41 @@
> > +LC_ALL=C \
> > +PATH=/bin \
> > +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> > +USER=test \
> > +LOGNAME=test \
> > +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> > +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> > +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> > +/usr/bin/qemu-system-i386 \
> > +-name guest=QEMUGuest1,debug-threads=on \
> > +-S \
> > +-object
> '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}'
> \
> > +-machine pc,accel=kvm,usb=off,dump-guest-core=off \
> > +-cpu qemu64 \
> > +-m size=2095104k,slots=16,maxmem=1099511627776k \
> > +-overcommit mem-lock=off \
> > +-smp 2,sockets=2,dies=1,cores=1,threads=1 \
> > +-object
> '{"qom-type":"memory-backend-ram","id":"ram-node0","size":2145386496}' \
> > +-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
> > +-object
> '{"qom-type":"memory-backend-ram","id":"memvirtiomem0","reserve":false,"size":1073741824}'
> \
> > +-device
> virtio-mem-pci,node=0,block-size=2097152,requested-size=536870912,memdev=memvirtiomem0,id=virtiomem0,bus=pci.0,addr=0x2
> \
> > +-object
> '{"qom-type":"memory-backend-file","id":"memvirtiomem1","mem-path":"/dev/hugepages2M/libvirt/qemu/-1-QEMUGuest1","reserve":false,"size":2147483648,"host-nodes":[1,2,3],"policy":"bind"}'
> \
> > +-device
> virtio-mem-pci,node=0,block-size=2097152,requested-size=1073741824,memdev=memvirtiomem1,id=virtiomem1,bus=pci.0,addr=0x3
> \
> > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> > +-display none \
> > +-no-user-config \
> > +-nodefaults \
> > +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> > +-mon chardev=charmonitor,id=monitor,mode=control \
> > +-rtc base=utc \
> > +-no-shutdown \
> > +-no-acpi \
> > +-boot strict=on \
> > +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
> > +-blockdev
> '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
> \
> > +-blockdev
> '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
> \
> > +-device
> ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \
> > +-audiodev id=audio1,driver=none \
> > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
> > +-sandbox
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> > +-msg timestamp=on
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 74af93b08f..23f4a8471f 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -3143,6 +3143,7 @@ mymain(void)
> >                    ARG_END);
> >       DO_TEST_CAPS_VER("memory-hotplug-virtio-pmem", "5.2.0");
> >       DO_TEST_CAPS_LATEST("memory-hotplug-virtio-pmem");
> > +    DO_TEST_CAPS_LATEST("memory-hotplug-virtio-mem");
> >
> >       DO_TEST("machine-aeskeywrap-on-caps",
> >               QEMU_CAPS_AES_KEY_WRAP,
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>
>

-- 
Thanks & Regards,
Jing,Qi

On 9/15/21 8:16 AM, Jing Qi wrote:
> Hi Michal,
> I tried to test the virtio-mem with upstream version v7.7.0-136-g9b49c2c6d3
> adding the current patch
> (with qemu-6.1.0-7.fc36.x86_64) -

Hey, thanks for that!

> 
> 
> <maxMemory slots='16' unit='KiB'>8388608</maxMemory>
>   <memory unit='KiB'>1179648</memory>
>   <currentMemory unit='KiB'>1179648</currentMemory>
>  <memoryBacking>
>     <allocation mode='immediate'/>
>   </memoryBacking>
>  ....
>   <cpu mode='custom' match='exact' check='none'>
>     <model fallback='forbid'>qemu64</model>
>     <numa>
>       <cell id='0' cpus='0-1' memory='1048576' unit='KiB' discard='yes'/>
>     </numa>
>   </cpu>
> ...
>  <memory model='virtio-mem'>
>       <source>
>         <nodemask>0</nodemask>
>         <pagesize unit='KiB'>2048</pagesize>
>       </source>
>       <target>
>         <size unit='KiB'>131072</size>
>         <node>0</node>
>         <block unit='KiB'>2048</block>
>         <requested unit='KiB'>131072</requested>
>       </target>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>     </memory>
> 
> The vm was started successfully and there was no "prealloc: true"  in the
> command line.
> But I also found when <allocation mode='immediate'/> was added in the
> domain and the "virtio-mem" device was not added, the qemu command line
> also didn't have "prealloc:true". Is this correct? This result is
> different in "libvirt-7.6.0-3.module+el8.5.0+12510+80564ecf.x86_64" &
> "qemu-kvm-6.0.0-29.module+el8.5.0+12386+43574bac.x86_64".

This doesn't sound correct. But I'm unable to reproduce locally. Can you
please share your domain XML that's failing?

Michal

On 9/14/21 3:05 PM, David Hildenbrand wrote:
> On 13.09.21 16:52, Michal Privoznik wrote:
>> Nothing special is happening here. All important changes were
>> done when for 'virtio-pmem' (adjusting the code to put virtio
>> memory on PCI bus, generating alias using
>> qemuDomainDeviceAliasIndex(). The only bit that might look
>> suspicious is no prealloc for virtio-mem. But if you think about
>> it, the whole purpose of this device is to change amount of
>> memory exposed to guest on the fly. There is no point in locking
>> the whole backend in memory.
> 
> Do I understand correctly that we'll always try setting "reserve=off",
> and "prealloc=off"? That would be awesome :)

prealloc=off would be set (almost) unconditionally for all
memory-backend-* objects that are associated with virtio-mem device. And
if QEMU is new enough to have .reserve attribute then reserve=off is set
too.

> 
> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
> but we are temporarily not able to support it -- as discussed, because
> virtio-mem in QEMU yet has to be taught about it.

priv->memPrealloc might not be what you think it is. The fact whether
immediate allocation was requested is stored in def->mem.allocation; and
priv->memPrealloc is just there to track whether -mem-prealloc what
already put onto (paritally) generated cmd line and thus the rest of
generators must refrain from setting prealloc=on.

Codewise speaking, if you'd look at qemuBuildCommandLine() (this is
where cmd line generation happens) then you'd see
qemuBuildMemCommandLine() being called first and only after that
qemuBuildMemoryDeviceCommandLine() being called second.

The former is responsible for generating generic memory for guest (e.g.
-m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a
combination of -machine memory-backend= + -object memory-backend-*  is
generated).

Long story short, if priv->memPrealloc isn't true at this point, then
libvirt doesn't have to set prealloc=off explicitly, because off is the
default.


> 
> FYI: QEMU will bail out if "reserve=off,prealloc=on" is set in combination.

Yeah, this combination should never happen.

Michal

On 15.09.21 13:07, Michal Prívozník wrote:
> On 9/14/21 3:05 PM, David Hildenbrand wrote:
>> On 13.09.21 16:52, Michal Privoznik wrote:
>>> Nothing special is happening here. All important changes were
>>> done when for 'virtio-pmem' (adjusting the code to put virtio
>>> memory on PCI bus, generating alias using
>>> qemuDomainDeviceAliasIndex(). The only bit that might look
>>> suspicious is no prealloc for virtio-mem. But if you think about
>>> it, the whole purpose of this device is to change amount of
>>> memory exposed to guest on the fly. There is no point in locking
>>> the whole backend in memory.
>>
>> Do I understand correctly that we'll always try setting "reserve=off",
>> and "prealloc=off"? That would be awesome :)
> 
> prealloc=off would be set (almost) unconditionally for all
> memory-backend-* objects that are associated with virtio-mem device. And
> if QEMU is new enough to have .reserve attribute then reserve=off is set
> too.
> 

Ah, perfect.

>>
>> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
>> but we are temporarily not able to support it -- as discussed, because
>> virtio-mem in QEMU yet has to be taught about it.
> 
> priv->memPrealloc might not be what you think it is. The fact whether
> immediate allocation was requested is stored in def->mem.allocation; and
> priv->memPrealloc is just there to track whether -mem-prealloc what
> already put onto (paritally) generated cmd line and thus the rest of
> generators must refrain from setting prealloc=on.
> 
> Codewise speaking, if you'd look at qemuBuildCommandLine() (this is
> where cmd line generation happens) then you'd see
> qemuBuildMemCommandLine() being called first and only after that
> qemuBuildMemoryDeviceCommandLine() being called second.
> 
> The former is responsible for generating generic memory for guest (e.g.
> -m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a
> combination of -machine memory-backend= + -object memory-backend-*  is
> generated).
> 
> Long story short, if priv->memPrealloc isn't true at this point, then
> libvirt doesn't have to set prealloc=off explicitly, because off is the
> default.

That makes sense. Yet I wonder if we should warn that preallocation is 
currently not supported for virtio-mem and will be ignored. Your decision :)

Thanks a bunch!


-- 
Thanks,

David / dhildenb

On 9/17/21 2:54 PM, David Hildenbrand wrote:
> On 15.09.21 13:07, Michal Prívozník wrote:
>> On 9/14/21 3:05 PM, David Hildenbrand wrote:
>>> On 13.09.21 16:52, Michal Privoznik wrote:
>>>> Nothing special is happening here. All important changes were
>>>> done when for 'virtio-pmem' (adjusting the code to put virtio
>>>> memory on PCI bus, generating alias using
>>>> qemuDomainDeviceAliasIndex(). The only bit that might look
>>>> suspicious is no prealloc for virtio-mem. But if you think about
>>>> it, the whole purpose of this device is to change amount of
>>>> memory exposed to guest on the fly. There is no point in locking
>>>> the whole backend in memory.
>>>
>>> Do I understand correctly that we'll always try setting "reserve=off",
>>> and "prealloc=off"? That would be awesome :)
>>
>> prealloc=off would be set (almost) unconditionally for all
>> memory-backend-* objects that are associated with virtio-mem device. And
>> if QEMU is new enough to have .reserve attribute then reserve=off is set
>> too.
>>
> 
> Ah, perfect.
> 
>>>
>>> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
>>> but we are temporarily not able to support it -- as discussed, because
>>> virtio-mem in QEMU yet has to be taught about it.
>>
>> priv->memPrealloc might not be what you think it is. The fact whether
>> immediate allocation was requested is stored in def->mem.allocation; and
>> priv->memPrealloc is just there to track whether -mem-prealloc what
>> already put onto (paritally) generated cmd line and thus the rest of
>> generators must refrain from setting prealloc=on.
>>
>> Codewise speaking, if you'd look at qemuBuildCommandLine() (this is
>> where cmd line generation happens) then you'd see
>> qemuBuildMemCommandLine() being called first and only after that
>> qemuBuildMemoryDeviceCommandLine() being called second.
>>
>> The former is responsible for generating generic memory for guest (e.g.
>> -m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a
>> combination of -machine memory-backend= + -object memory-backend-*  is
>> generated).
>>
>> Long story short, if priv->memPrealloc isn't true at this point, then
>> libvirt doesn't have to set prealloc=off explicitly, because off is the
>> default.
> 
> That makes sense. Yet I wonder if we should warn that preallocation is
> currently not supported for virtio-mem and will be ignored. Your
> decision :)

We could do that, yes. Let me respin the series with that change - these
patches don't apply cleanly anymore anyway. Should a similar warning be
produced when hugepages are configured?

Michal

On 21.09.21 10:56, Michal Prívozník wrote:
> On 9/17/21 2:54 PM, David Hildenbrand wrote:
>> On 15.09.21 13:07, Michal Prívozník wrote:
>>> On 9/14/21 3:05 PM, David Hildenbrand wrote:
>>>> On 13.09.21 16:52, Michal Privoznik wrote:
>>>>> Nothing special is happening here. All important changes were
>>>>> done when for 'virtio-pmem' (adjusting the code to put virtio
>>>>> memory on PCI bus, generating alias using
>>>>> qemuDomainDeviceAliasIndex(). The only bit that might look
>>>>> suspicious is no prealloc for virtio-mem. But if you think about
>>>>> it, the whole purpose of this device is to change amount of
>>>>> memory exposed to guest on the fly. There is no point in locking
>>>>> the whole backend in memory.
>>>>
>>>> Do I understand correctly that we'll always try setting "reserve=off",
>>>> and "prealloc=off"? That would be awesome :)
>>>
>>> prealloc=off would be set (almost) unconditionally for all
>>> memory-backend-* objects that are associated with virtio-mem device. And
>>> if QEMU is new enough to have .reserve attribute then reserve=off is set
>>> too.
>>>
>>
>> Ah, perfect.
>>
>>>>
>>>> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
>>>> but we are temporarily not able to support it -- as discussed, because
>>>> virtio-mem in QEMU yet has to be taught about it.
>>>
>>> priv->memPrealloc might not be what you think it is. The fact whether
>>> immediate allocation was requested is stored in def->mem.allocation; and
>>> priv->memPrealloc is just there to track whether -mem-prealloc what
>>> already put onto (paritally) generated cmd line and thus the rest of
>>> generators must refrain from setting prealloc=on.
>>>
>>> Codewise speaking, if you'd look at qemuBuildCommandLine() (this is
>>> where cmd line generation happens) then you'd see
>>> qemuBuildMemCommandLine() being called first and only after that
>>> qemuBuildMemoryDeviceCommandLine() being called second.
>>>
>>> The former is responsible for generating generic memory for guest (e.g.
>>> -m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a
>>> combination of -machine memory-backend= + -object memory-backend-*  is
>>> generated).
>>>
>>> Long story short, if priv->memPrealloc isn't true at this point, then
>>> libvirt doesn't have to set prealloc=off explicitly, because off is the
>>> default.
>>
>> That makes sense. Yet I wonder if we should warn that preallocation is
>> currently not supported for virtio-mem and will be ignored. Your
>> decision :)
> 
> We could do that, yes. Let me respin the series with that change - these
> patches don't apply cleanly anymore anyway. Should a similar warning be
> produced when hugepages are configured?

I think it might make sense to print a warning if we cannot support 
prealloc and it was requested. We can then skip the warning once 
virtio-mem supports the "prealloc" property that we can just enable.

Using hugepages without prealloc is in general unsafe, so I don't think 
we need a virtio-mem specific warning. :)

Thanks!

-- 
Thanks,

David / dhildenb

Hi Michal,
The domain xml is attached.

Regards,
Jing Qi

On Wed, Sep 15, 2021 at 7:14 PM Michal Prívozník <mprivozn@redhat.com>
wrote:

> On 9/14/21 3:05 PM, David Hildenbrand wrote:
> > On 13.09.21 16:52, Michal Privoznik wrote:
> >> Nothing special is happening here. All important changes were
> >> done when for 'virtio-pmem' (adjusting the code to put virtio
> >> memory on PCI bus, generating alias using
> >> qemuDomainDeviceAliasIndex(). The only bit that might look
> >> suspicious is no prealloc for virtio-mem. But if you think about
> >> it, the whole purpose of this device is to change amount of
> >> memory exposed to guest on the fly. There is no point in locking
> >> the whole backend in memory.
> >
> > Do I understand correctly that we'll always try setting "reserve=off",
> > and "prealloc=off"? That would be awesome :)
>
> prealloc=off would be set (almost) unconditionally for all
> memory-backend-* objects that are associated with virtio-mem device. And
> if QEMU is new enough to have .reserve attribute then reserve=off is set
> too.
>
> >
> > I do wonder if we want to warn or bail out if "priv->memPrealloc" is set
> > but we are temporarily not able to support it -- as discussed, because
> > virtio-mem in QEMU yet has to be taught about it.
>
> priv->memPrealloc might not be what you think it is. The fact whether
> immediate allocation was requested is stored in def->mem.allocation; and
> priv->memPrealloc is just there to track whether -mem-prealloc what
> already put onto (paritally) generated cmd line and thus the rest of
> generators must refrain from setting prealloc=on.
>
> Codewise speaking, if you'd look at qemuBuildCommandLine() (this is
> where cmd line generation happens) then you'd see
> qemuBuildMemCommandLine() being called first and only after that
> qemuBuildMemoryDeviceCommandLine() being called second.
>
> The former is responsible for generating generic memory for guest (e.g.
> -m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a
> combination of -machine memory-backend= + -object memory-backend-*  is
> generated).
>
> Long story short, if priv->memPrealloc isn't true at this point, then
> libvirt doesn't have to set prealloc=off explicitly, because off is the
> default.
>
>
> >
> > FYI: QEMU will bail out if "reserve=off,prealloc=on" is set in
> combination.
>
> Yeah, this combination should never happen.
>
> Michal
>
>

-- 
Thanks & Regards,
Jing,Qi

On 9/15/21 1:26 PM, Jing Qi wrote:
> Hi Michal,
> The domain xml is attached.

Thank you. I was able to reproduce but the problem is not in these
patches. I can reproduce even without them. I have a fix that I will
post shortly.

Michal