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

[zhenwei pi]

On 1/25/23 16:09, Michal Prívozník wrote:
> 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
> 

This sounds fine! A million thanks to you!

-- 
zhenwei pi