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 <mkletzan@xxxxxxxxxx> > --- > 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 <server> or <msi>. 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, > } > } > > + 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 @@ > <memballoon model='none'/> > <shmem name='shmem0'> > <model type='ivshmem'/> > + <size unit='M'>4</size> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > </shmem> > <shmem name='shmem1'> > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list