[libvirt] [PATCH] conf: Parse virtio-crypto in the domain XML

[Longpeng (Mike)]

Hi Martin,

On 2017/1/9 16:44, Martin Kletzander wrote:

> On Mon, Jan 09, 2017 at 10:15:25AM +0800, Longpeng(Mike) wrote:
>> As virtio-crypto has been supported in QEMU 2.9 and
>> the frontend driver has been merged in linux 4.10,
>> so it's necessary to support virtio-crypto in domain
>> XML.
>>
>> This patch parse the domain XML with virtio-crypto
>> support, the virtio-crypto XML looks like this:
>>
>>    <crypto model='virtio'>
>>        <backend type='builtin' queues='2'/>
>>    </crypto>
>>
>> Signed-off-by: Longpeng(Mike) <longpeng2 at huawei.com>
>> ---
>> src/conf/domain_conf.c         | 212 +++++++++++++++++++++++++++++++++++++++++
>> src/conf/domain_conf.h         |  32 +++++++
>> src/qemu/qemu_alias.c          |  20 ++++
>> src/qemu/qemu_alias.h          |   4 +
>> src/qemu/qemu_capabilities.c   |   4 +
>> src/qemu/qemu_capabilities.h   |   2 +
>> src/qemu/qemu_command.c        | 129 +++++++++++++++++++++++++
>> src/qemu/qemu_command.h        |   4 +
>> src/qemu/qemu_domain_address.c |  17 ++++
>> src/qemu/qemu_driver.c         |   6 ++
>> src/qemu/qemu_hotplug.c        |   1 +
>> 11 files changed, 431 insertions(+)
>>
> 
> No tests, no documentation, no schema => no acks.
> 
> The patch should be split into at least two parts (conf, schema,
> qemuxml2xmltest and docs in one; then hypervisor functionality and tests
> in the second one).
> 


OK, I will split the patch and add some testcases in V2.

> Also I see no hunk adding anything to qemuDomainAssignDevicePCISlots()
> or similar.
> 


I added in fact :)

@@ -1236,6 +1242,17 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             goto error;
     }

+    /* VirtIO CRYPTO */
+    for (i = 0; i < def->ncryptos; i++) {
+        if (def->cryptos[i]->model != VIR_DOMAIN_CRYPTO_MODEL_VIRTIO ||
+            !virDeviceInfoPCIAddressWanted(&def->cryptos[i]->info))
+            continue;
+
+        if (virDomainPCIAddressReserveNextSlot(addrs,
+                                               &def->cryptos[i]->info, flags) < 0)
+            goto error;
+    }
+

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9f7b906..fcfccd5 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -237,6 +237,7 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
>>               "memballoon",
>>               "nvram",
>>               "rng",
>> +              "crypto",
> 
> Why are you adding this in a random place rather than at the end?  That
> could be fixed in most of the hunks.
> 


OK, I will fix it in V2.

>> @@ -21590,6 +21749,51 @@ virDomainRNGDefFree(virDomainRNGDefPtr def)
>>
>>
>> static int
>> +virDomainCryptoDefFormat(virBufferPtr buf,
>> +                         virDomainCryptoDefPtr def,
>> +                         unsigned int flags)
>> +{
>> +    const char *model = virDomainCryptoModelTypeToString(def->model);
>> +    const char *backend = virDomainCryptoBackendTypeToString(def->backend);
>> +
>> +    virBufferAsprintf(buf, "<crypto model='%s'>\n", model);
>> +    virBufferAdjustIndent(buf, 2);
>> +    virBufferAsprintf(buf, "<backend type='%s'", backend);
>> +
>> +    switch ((virDomainCryptoBackend) def->backend) {
>> +    case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN:
>> +        if (def->queues)
>> +            virBufferAsprintf(buf, " queues='%u'", def->queues);
>> +
>> +        virBufferAddLit(buf, "/>\n");
>> +        break;
>> +
>> +    case VIR_DOMAIN_CRYPTO_BACKEND_LAST:
>> +        break;
>> +    }
>> +
>> +    if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) {
> 
> It should always have an address, this is not an old device with
> back-compat problems.
> 


All right, thanks

>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 2c0b29d..b1b718b 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -337,6 +337,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>               "drive-detect-zeroes",
>>
>>               "tls-creds-x509", /* 230 */
>> +              "cryptodev-backend-builtin",
>> +              "virtio-crypto",
>>     );
>>
>>
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index affb639..98cb817 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -370,6 +370,8 @@ typedef enum {
>>
>>     /* 230 */
>>     QEMU_CAPS_OBJECT_TLS_CREDS_X509, /* -object tls-creds-x509 */
>> +    QEMU_CAPS_OBJECT_CRYPTO_BUILTIN,
>> +    QEMU_CAPS_DEVICE_VIRTIO_CRYPTO,
>>
> 
> Base your patch on master, not on some old, private, branch.  Also see
> how every capability has a nice comment even if it's very trivial.  It
> can help sometimes.
> 


I got it, thanks :)

> I haven't tested it or read it thoroughly, but the rest looks
> reasonable.
> 


I had tested this patch and 'make syntax-check' before sent it. I will rework
this patch according to your suggestion and send V2 in these days :)

> Martin


-- 
Regards,
Longpeng(Mike)