Re: [PATCH v2 4/5] qemu: command: support crypto device
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(a)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