Re: [libvirt] [PATCH 6/8] qemu: Save various defaults for shmem

[John Ferlan]

On 09/27/2016 08:24 AM, Martin Kletzander wrote:
> We're keeping some things at default and that's not something we want to
> do intentionaly.  Let's save some sensible defaults upfront then having

intentionally

s/then having/in order to avoid having/

> problems later.  The details for the defaults (of the newer
> implementation) can be found in qemu's commit 5400c02b90bb:
> 
>   http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_domain.c| 49 
> ++-
>  tests/qemuxml2argvdata/qemuxml2argv-shmem.args|  2 +-
>  tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml |  1 +
>  3 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9b1a32ec3897..83b1f98d9a43 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2551,12 +2551,55 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr 
> chr,
>  STRPREFIX(chr->source.data.nix.path, cfg->channelTargetDir)) {
>  VIR_FREE(chr->source.data.nix.path);
>  }
> -

Rogue editor change or OCD?

>  virObjectUnref(cfg);
>  }
> 
> 

Although I'm sure the qemu commit referenced explains the defaults,
maybe a bit of details here would help...  To paraphrase from my quick
read... The 'size' is only relevant for IVSHMEM, it's not used for plain
and doorbell.  The usage of plain or doorbell is based upon whether
someone chooses  or . One cannot choose both in this new
world. Selection of one or the other in the XML infers the type of shmem
device.

>  static int
> +qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm)
> +{
> +if (!shm->size)
> +shm->size = 4 << 20;

I guess since we're not saving/migrating blindly setting won't cause
strange failures for virDomainShmemDefFind/virDomainShmemDefEquals, but
will it cause a problem for virDomainShmemDefCheckABIStability? There's
a call to the ABI code from qemuDomainRevertToSnapshot which has
comments for the transitions that would fall into this code being a
migration, but without trying to page *that* back into my short term
memory - well I figured I'd just note it and see what the consensus was!

This also seem to have an effect on the default for -plain, but gets
reset for -doorbell. [1]

I think it'd be 'cleaner' if each model was part of a switch statement
rather than the opposite world here where "server" assumes "-plain" and
"msi" assumes "-doorbell".  In the long run it'll be cleaner to read the
code I think if we know what each has for a default...

> +
> +/* Nothing more to check/change for IVSHMEM */
> +if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM)
> +return 0;
> +
> +if (!shm->server.enabled) {
> +if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("shmem model '%s' is supported "
> + "only with server option enabled"),
> +   virDomainShmemModelTypeToString(shm->model));
> +return -1;
> +}
> +
> +if (shm->msi.enabled) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("shmem model '%s' doesn't support "
> + "msi"),
> +   virDomainShmemModelTypeToString(shm->model));
> +}
> +} else {
> +if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("shmem model '%s' is supported "
> + "only with server option disabled"),
> +   virDomainShmemModelTypeToString(shm->model));
> +return -1;
> +}
> +
> +shm->size = 0;

[1]
Is setting this to 0 the "correct" thing to do or should it be an error
if a "-doorbell" shmem device uses the 'size' field? Blindly setting it
to zero just doesn't seem right.

>From the qemu checkin note...

Changes from ivshmem to ivshmem-plain:
...

* Properties "shm" and "size" are gone.  Use property "memdev"
  instead.
...

Changes from ivshmem to ivshmem-doorbell:
...
* Property "size" is gone.  The new device can only map all the shared
  memory received from the server.
...


There's an implication in the following patch too...

I think it'd be good to post a v2 of this patch at least before an ACK.


John



> +shm->msi.enabled = true;
> +if (!shm->msi.ioeventfd)
> +shm->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON;
> +}
> +
> +return 0;
> +}
> +
> +
> +static int
>  qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>   const virDomainDef *def,
>   virCapsPtr caps,
> @@ -2760,6 +2803,10 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>  }
>  }
> 
> 

[libvirt] [PATCH 6/8] qemu: Save various defaults for shmem

[Martin Kletzander]

We're keeping some things at default and that's not something we want to
do intentionaly.  Let's save some sensible defaults upfront then having
problems later.  The details for the defaults (of the newer
implementation) can be found in qemu's commit 5400c02b90bb:

  http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_domain.c| 49 ++-
 tests/qemuxml2argvdata/qemuxml2argv-shmem.args|  2 +-
 tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml |  1 +
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9b1a32ec3897..83b1f98d9a43 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2551,12 +2551,55 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr,
 STRPREFIX(chr->source.data.nix.path, cfg->channelTargetDir)) {
 VIR_FREE(chr->source.data.nix.path);
 }
-
 virObjectUnref(cfg);
 }


 static int
+qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm)
+{
+if (!shm->size)
+shm->size = 4 << 20;
+
+/* Nothing more to check/change for IVSHMEM */
+if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM)
+return 0;
+
+if (!shm->server.enabled) {
+if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("shmem model '%s' is supported "
+ "only with server option enabled"),
+   virDomainShmemModelTypeToString(shm->model));
+return -1;
+}
+
+if (shm->msi.enabled) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("shmem model '%s' doesn't support "
+ "msi"),
+   virDomainShmemModelTypeToString(shm->model));
+}
+} else {
+if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("shmem model '%s' is supported "
+ "only with server option disabled"),
+   virDomainShmemModelTypeToString(shm->model));
+return -1;
+}
+
+shm->size = 0;
+shm->msi.enabled = true;
+if (!shm->msi.ioeventfd)
+shm->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON;
+}
+
+return 0;
+}
+
+
+static int
 qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
  const virDomainDef *def,
  virCapsPtr caps,
@@ -2760,6 +2803,10 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 }
 }

+if (dev->type == VIR_DOMAIN_DEVICE_SHMEM &&
+qemuDomainShmemDefPostParse(dev->data.shmem) < 0)
+goto cleanup;
+
 ret = 0;

  cleanup:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args 
b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
index 99fac119b04c..bdf660a3c435 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
@@ -17,7 +17,7 @@ QEMU_AUDIO_DRV=none \
 -no-acpi \
 -boot c \
 -usb \
--device ivshmem,id=shmem0,shm=shmem0,bus=pci.0,addr=0x3 \
+-device ivshmem,id=shmem0,size=4m,shm=shmem0,bus=pci.0,addr=0x3 \
 -device ivshmem,id=shmem1,size=128m,shm=shmem1,bus=pci.0,addr=0x5 \
 -device ivshmem,id=shmem2,size=256m,shm=shmem2,bus=pci.0,addr=0x4 \
 -device ivshmem,id=shmem3,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml
index 5602913648bc..04b463a27892 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml
@@ -23,6 +23,7 @@
 
 
   
+  4
   
 
 
-- 
2.10.0

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