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

2017-01-09 Thread 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:
>>
>>
>>
>>
>>
>> Signed-off-by: Longpeng(Mike) 
>> ---
>> 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, "\n", model);
>> +virBufferAdjustIndent(buf, 2);
>> +virBufferAsprintf(buf, "> +
>> +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)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-01-09 Thread Martin Kletzander 

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:

   
   
   

Signed-off-by: Longpeng(Mike) 
---
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).

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


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.


@@ -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, "\n", model);
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "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.


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 haven't tested it or read it thoroughly, but the rest looks
reasonable.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-01-08 Thread Gonglei (Arei) 
>
> Sent: Monday, January 09, 2017 10:15 AM
> To: libvir-list@redhat.com
> Cc: Wubin (H); Zhoujian (jay, Euler); Gonglei (Arei); berra...@redhat.com;
> longpeng
> Subject: [PATCH] conf: Parse virtio-crypto in the domain XML
> 
> As virtio-crypto has been supported in QEMU 2.9 and

Take a quick glance:

s/QEMU 2.9/QEMU 2.8/

> 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:
> 
> 
> 
> 
> 
> Signed-off-by: Longpeng(Mike) 
> ---

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2017-01-08 Thread Longpeng(Mike) 
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:





Signed-off-by: Longpeng(Mike) 
---
 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(+)

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",
   "shmem",
   "tpm",
   "panic",
@@ -797,6 +798,14 @@ VIR_ENUM_IMPL(virDomainRNGBackend,
   "random",
   "egd");
 
+VIR_ENUM_IMPL(virDomainCryptoModel,
+  VIR_DOMAIN_CRYPTO_MODEL_LAST,
+  "virtio");
+
+VIR_ENUM_IMPL(virDomainCryptoBackend,
+  VIR_DOMAIN_CRYPTO_BACKEND_LAST,
+  "builtin");
+
 VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST,
   "tpm-tis")
 
@@ -2337,6 +2346,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def)
 case VIR_DOMAIN_DEVICE_RNG:
 virDomainRNGDefFree(def->data.rng);
 break;
+case VIR_DOMAIN_DEVICE_CRYPTO:
+virDomainCryptoDefFree(def->data.crypto);
+break;
 case VIR_DOMAIN_DEVICE_CHR:
 virDomainChrDefFree(def->data.chr);
 break;
@@ -2600,6 +2612,10 @@ void virDomainDefFree(virDomainDefPtr def)
 virDomainRNGDefFree(def->rngs[i]);
 VIR_FREE(def->rngs);
 
+for (i = 0; i < def->ncryptos; i++)
+virDomainCryptoDefFree(def->cryptos[i]);
+VIR_FREE(def->cryptos);
+
 for (i = 0; i < def->nmems; i++)
 virDomainMemoryDefFree(def->mems[i]);
 VIR_FREE(def->mems);
@@ -3169,6 +3185,8 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
 return &device->data.shmem->info;
 case VIR_DOMAIN_DEVICE_RNG:
 return &device->data.rng->info;
+case VIR_DOMAIN_DEVICE_CRYPTO:
+return &device->data.crypto->info;
 case VIR_DOMAIN_DEVICE_TPM:
 return &device->data.tpm->info;
 case VIR_DOMAIN_DEVICE_PANIC:
@@ -3464,6 +3482,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
 if (cb(def, &device, &def->rngs[i]->info, opaque) < 0)
 return -1;
 }
+device.type = VIR_DOMAIN_DEVICE_CRYPTO;
+for (i = 0; i < def->ncryptos; i++) {
+device.data.crypto = def->cryptos[i];
+if (cb(def, &device, &def->cryptos[i]->info, opaque) < 0)
+return -1;
+}
 if (def->nvram) {
 device.type = VIR_DOMAIN_DEVICE_NVRAM;
 device.data.nvram = def->nvram;
@@ -3541,6 +3565,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
 case VIR_DOMAIN_DEVICE_PANIC:
 case VIR_DOMAIN_DEVICE_LAST:
 case VIR_DOMAIN_DEVICE_RNG:
+case VIR_DOMAIN_DEVICE_CRYPTO:
 case VIR_DOMAIN_DEVICE_MEMORY:
 break;
 }
@@ -4599,6 +4624,7 @@ virDomainDeviceDefValidateInternal(const 
virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_MEMBALLOON:
 case VIR_DOMAIN_DEVICE_NVRAM:
 case VIR_DOMAIN_DEVICE_RNG:
+case VIR_DOMAIN_DEVICE_CRYPTO:
 case VIR_DOMAIN_DEVICE_SHMEM:
 case VIR_DOMAIN_DEVICE_TPM:
 case VIR_DOMAIN_DEVICE_PANIC:
@@ -12022,6 +12048,88 @@ virDomainRNGDefParseXML(xmlNodePtr node,
 }
 
 
+static virDomainCryptoDefPtr
+virDomainCryptoDefParseXML(xmlNodePtr node,
+   xmlXPathContextPtr ctxt,
+   unsigned int flags)
+{
+char *model = NULL;
+char *backend = NULL;
+char *queues = NULL;
+virDomainCryptoDefPtr def;
+xmlNodePtr save = ctxt->node;
+xmlNodePtr *backends = NULL;
+int nbackends;
+
+if (VIR_ALLOC(def) < 0)
+return NULL;
+
+if (!(model = virXMLPropString(node, "model"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing Crypto device model"));
+goto error;
+}
+
+if ((def->model = virDomainCryptoModelTypeFromString(model)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown Crypto model '%s'"), model);
+goto error;
+}
+
+ctxt->node = node;
+
+if ((nbackends = virXPathNodeSet("./backend", ctxt, &backends)) < 0)
+goto error;
+
+if (nbackends !=