On Fri, Sep 16, 2016 at 10:32:48AM +0200, Peter Krempa wrote:
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@xxxxxxxxxx> --- 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?
No, this should be DEFAULT, just the naming is weird. Doorbell has no roles (e.g. no 'master' parameter) and "DEFAULT" should actually be called "NONE".
+ + /* 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.
Default is 1, we can configure that if someone wants that in the future.
+ } + } + }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
I'll move those checks here as you suggested in some other patch.
+ ret = 0;Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list