[libvirt] [PATCH 7/8] qemu: Support newer ivshmem device variants

[Martin Kletzander]

On Fri, Oct 07, 2016 at 12:20:19PM -0400, John Ferlan wrote:
>
>
>On 09/27/2016 08:24 AM, Martin Kletzander wrote:
>> QEMU added support for ivshmem-plain and ivshmem-doorbell.  Those are
>> reworked varians of legacy ivshmem that are compatible from the guest
>> POV, but not from host's POV and have sane specification and handling.
>>
>
>It seems less "added support for" and more "forced libvirt to choose"
>between one or the other as of qemu v2.6.
>

Well, not really, it's not removed from newer QEMU, it's just deprecated.

>> Details about the newer device type 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_command.c                            | 88 +++++++++++++++++++++-
>>  src/qemu/qemu_command.h                            | 10 +++
>>  .../qemuxml2argv-shmem-plain-doorbell.args         | 43 +++++++++++
>>  .../qemuxml2argv-shmem-plain-doorbell.xml          | 63 ++++++++++++++++
>>  tests/qemuxml2argvtest.c                           |  3 +
>>  5 files changed, 204 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index aa8cff80e8b1..29f7130ef911 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def,
>>      return NULL;
>>  }
>>
>> +char *
>> +qemuBuildShmemDevStr(virDomainDefPtr def,
>> +                     virDomainShmemDefPtr shmem,
>> +                     virQEMUCapsPtr qemuCaps)
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model));
>> +    virBufferAsprintf(&buf, ",id=%s", shmem->info.alias);
>> +
>> +    if (shmem->server.enabled)
>> +        virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias);
>> +    else
>> +        virBufferAsprintf(&buf, ",memdev=shmmem-%s", shmem->info.alias);
>> +
>> +    if (shmem->msi.vectors)
>> +        virBufferAsprintf(&buf, ",vectors=%u", shmem->msi.vectors);
>> +    if (shmem->msi.ioeventfd) {
>> +        virBufferAsprintf(&buf, ",ioeventfd=%s",
>> +                          virTristateSwitchTypeToString(shmem->msi.ioeventfd));
>> +    }
>> +
>> +    if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) {
>> +        virBufferFreeAndReset(&buf);
>> +        return NULL;
>> +    }
>> +
>> +    if (virBufferCheckError(&buf) < 0)
>
>Still would need to FreeAndReset - I'd be OK if it were an || to the
>previous if, although I know that causes agita for others.
>

Well, not really.  The content gets free()'d whenever the buf->error is
set, so it is already cleared if there was an error.

>I suppose you could to the error: label path/option too.
>
>> +        return NULL;
>> +
>> +    return virBufferContentAndReset(&buf);
>> +}
>> +
>>  static char *
>>  qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
>>                              virCommandPtr cmd,
>> @@ -8476,6 +8509,50 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
>>      return devstr;
>>  }
>>
>> +
>> +virJSONValuePtr
>> +qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem)
>> +{
>> +    char *mem_path = NULL;
>> +    virJSONValuePtr ret = NULL;
>> +
>> +    if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0)
>> +        return NULL;
>> +
>> +    virJSONValueObjectCreate(&ret,
>> +                             "s:mem-path", mem_path,
>> +                             "U:size", shmem->size,
>> +                             NULL);
>
>Hmm... perhaps not so much of an issue as previously thought... This
>will only be called for the -plain anyway and well shmem->size had
>better not be zero (since you're using "U:")..
>
>So still leaves me wondering if we should fail if a size is provided for
>-doorbell
>

OK, I added that.

>> +
>> +    VIR_FREE(mem_path);
>> +    return ret;
>> +}
>> +
>> +
>> +static char *
>> +qemuBuildShmemBackendMemStr(virDomainShmemDefPtr shmem)
>> +{
>> +    char *ret = NULL;
>> +    char *alias = NULL;
>> +    virJSONValuePtr props = qemuBuildShmemBackendMemProps(shmem);
>> +
>> +    if (!props)
>> +        return NULL;
>> +
>> +    if (virAsprintf(&alias, "shmmem-%s", shmem->info.alias) < 0)
>> +        goto cleanup;
>> +
>> +    ret = virQEMUBuildObjectCommandlineFromJSON("memory-backend-file",
>> +                                                alias,
>> +                                                props);
>> + cleanup:
>> +    VIR_FREE(alias);
>> +    virJSONValueFree(props);
>> +
>> +    return ret;
>> +}
>> +
>> +
>>  static int
>>  qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>>                            virCommandPtr cmd,
>> @@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>>          break;
>>
>
>Should there be caps checks for :
>
>QEMU_CAPS_DEVICE_IVSHMEM_PLAIN
>QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL
>

Sure, I should've added that, good point.

>You added the caps in the xml2argvtest, so I'd expect them...  Obviously
>they wouldn't work on qemu 2.5 or earlier.
>
>As long as the memory leak is handled and there's an answer for the caps
>checks, this is ACK-able from my POV.
>

I have to send v4 of a prerequisite patch, so I'll just repost it.

Martin
-------------- 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/20161014/0d52eb11/attachment-0001.sig>