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

[Martin Kletzander]

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 at redhat.com>
>> ---
>>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160916/d353afbf/attachment-0001.sig>