[PATCH v2 4/5] qemu: command: support crypto device

[Michal Prívozník]

On 1/25/23 02:54, zhenwei pi wrote:
> On 1/25/23 01:08, Michal Prívozník wrote:
>> On 1/17/23 02:46, zhenwei pi wrote:
>>> Support virtio-crypto device, also support cryptodev types:
>>> - builtin
>>> - lkcf
>>>
>>> Finally, we can launch a VM(QEMU) with one or more crypto devices by
>>> libvirt.
>>>
>>> Signed-off-by: zhenwei pi <pizhenwei at bytedance.com>
>>> ---
>>>   src/qemu/qemu_command.c | 110 +++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 109 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index bb7031f66d..996a13a77b 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -926,6 +926,12 @@ qemuBuildVirtioDevGetConfigDev(const
>>> virDomainDeviceDef *device,
>>>           }
>>>               break;
>>>   +        case VIR_DOMAIN_DEVICE_CRYPTO: {
>>> +            *baseName = "virtio-crypto";
>>> +            *virtioOptions = device->data.crypto->virtio;
>>> +            break;
>>> +        }
>>> +
>>>           case VIR_DOMAIN_DEVICE_LEASE:
>>>           case VIR_DOMAIN_DEVICE_SOUND:
>>>           case VIR_DOMAIN_DEVICE_WATCHDOG:
>>> @@ -942,7 +948,6 @@ qemuBuildVirtioDevGetConfigDev(const
>>> virDomainDeviceDef *device,
>>>           case VIR_DOMAIN_DEVICE_MEMORY:
>>>           case VIR_DOMAIN_DEVICE_IOMMU:
>>>           case VIR_DOMAIN_DEVICE_AUDIO:
>>> -        case VIR_DOMAIN_DEVICE_CRYPTO:
>>>           case VIR_DOMAIN_DEVICE_LAST:
>>>           default:
>>>               break;
>>> @@ -9894,6 +9899,106 @@ qemuBuildVsockCommandLine(virCommand *cmd,
>>>   }
>>>     +static int
>>> +qemuBuildCryptoBackendProps(virDomainCryptoDef *crypto,
>>> +                            virJSONValue **props)
>>> +{
>>> +    g_autofree char *objAlias = NULL;
>>> +
>>> +    objAlias = g_strdup_printf("obj%s", crypto->info.alias);
>>> +
>>> +    switch ((virDomainCryptoBackend) crypto->backend) {
>>> +    case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN:
>>> +        if (qemuMonitorCreateObjectProps(props,
>>> "cryptodev-backend-builtin",
>>> +                                         objAlias, NULL) < 0)
>>> +            return -1;
>>> +
>>> +        break;
>>> +
>>> +    case VIR_DOMAIN_CRYPTO_BACKEND_LKCF:
>>> +        if (qemuMonitorCreateObjectProps(props,
>>> "cryptodev-backend-lkcf",
>>> +                                         objAlias, NULL) < 0)
>>> +            return -1;
>>> +
>>> +        break;
>>> +
>>> +    case VIR_DOMAIN_CRYPTO_BACKEND_LAST:
>>> +        break;
>>> +    }
>>
>> This can be simplified a bit:
>>
>> const char *backend = NULL;
>>
>> switch(crypto->backend) {
>> case ..._BUILTIN: backend = "...-builtin"; break;
>> case ..._LKCF: backend = "...-lkcf"; break;
>> case ..._LAST: break;
>> }
>>
> 
> Hi Michal
> 
> The *builtin* and *lkcf* driver uses no more parameters currently, and
> the simplified code seems fine.
> I suppose that other new drivers may be added into QEMU in future, and
> the new driver may use complex parameters. To avoid trunk of changes in
> this *switch-case*, I chose the original style in the v1/v2 version.

Well, for that we have another trick up our sleeve: the formatting
string for virJSONValueObjectAdd() (well, virJSONValueObjectAddVArgs())
allows adding attributes conditionally. For instance:

virJSONValueObjectAdd(props, "M:bitmask", bitmap);

adds bitmap to props only if bitmap != NULL.

I had another idea though:

We can declare an virDomainCryptoBackend enum -> qemu backend string
conversion, like this:

VIR_ENUM_DECL(qemuCryptoBackend);
VIR_ENUM_IMPL(qemuCryptoBackend,
              VIR_DOMAIN_CRYPTO_BACKEND_LAST,
              "cryptodev-backend-builtin",
              "cryptodev-backend-lkcf",
);


and then create props object simply as:

    if (qemuMonitorCreateObjectProps(props,
                                     qemuCryptoBackendTypeToString(crypto->backend),
                                     objAlias,
                                     "p:queues", crypto->queues,
                                     NULL) < 0)
        return -1;
And if we ever need to introduce additional attributes for new backends,
we can introduce the switch() statement:

switch (crypto->backend) {
case VIR_DOMAIN_CRYPTO_BACKEND_SOMETHING:
  virJSONValueObjectAdd(props, "s:newattrib", newAttrib);
  break;
  ....
}

My rationale behind all of this is to avoid initializing props only
sometime. But I guess I watch too much "defense" programming videos. I
can stick with your version if you want.

Michal