Re: [libvirt] [PATCH 09/20] qemu: Save various defaults for shmem

On Thu, Sep 15, 2016 at 18:14:34 +0200, Martin Kletzander wrote: > When we change the device used for the shared memory, it should not > change the settings, so rather save them upfront then having problems > later. > > The only thing we are not saving is the role for old ivshmem and that's > because we were not specifying it, which means role=auto and that is > really bad idea to be using (due to various things). However, we don't > want to change the behaviour, so that's the reason for that. > > 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 <mkletzan(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++ > tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- > tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 3 ++- > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 3f16dbee2e6a..1fdbdf68fc8b 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -2747,6 +2747,39 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, > } > } > > + if (dev->type == VIR_DOMAIN_DEVICE_SHMEM) { > + if (!dev->data.shmem->server.enabled) { > + /* The size is 4M if not specified */ > + if (!dev->data.shmem->size) > + dev->data.shmem->size = 4 << 20; > + /* For old ivshmem we shouldn't change the role, the new > + * ones are peer by default, mostly to safeguard that > + * there should be no more than one master */ > + if (!dev->data.shmem->role) { > + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) > + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_MASTER; > + else > + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_PEER; > + } > + } else { > + /* In QEMU, role doesn't make sense for server-side shmem */ > + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_DEFAULT; Shouldn't this be VIR_DOMAIN_SHMEM_ROLE_PEER?

> + > + /* Defaults/Requirements for the newer device that we should save */ > + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { > + /* Size does not make much sense when claiming memory from > + * the server and so the newer version doesn't support that */ > + dev->data.shmem->size = 0; > + > + /* Also they can only exist with MSI and ioeventfd is > + * enabled unless specifically disabled */ > + dev->data.shmem->msi.enabled = true; > + if (!dev->data.shmem->msi.ioeventfd) > + dev->data.shmem->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON; This does not tweak the number of ioeventfd vectors, which will probably remain 0.

> + } > + } > + } The above code does not check the following two incompatible configs: 1) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN and server is enabled 2) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL and server is disabled

> + > ret = 0; > Peter