[PATCH v2 5/6] tests: unit tests for iSCSI iSER
Signed-off-by: Han Han
---
.../qemuxml2argvdata/disk-network-iscsi.args | 8 +++-
.../disk-network-iscsi.x86_64-2.12.0.args | 7 ++-
.../disk-network-iscsi.x86_64-latest.args | 45 +++
tests/qemuxml2argvdata/disk-network-iscsi.xml | 9
tests/qemuxml2argvtest.c | 5 ++-
.../qemuxml2xmloutdata/disk-network-iscsi.xml | 10 +
tests/qemuxml2xmltest.c | 4 +-
tests/virstoragetest.c| 16 +++
8 files changed, 80 insertions(+), 24 deletions(-)
diff --git a/tests/qemuxml2argvdata/disk-network-iscsi.args
b/tests/qemuxml2argvdata/disk-network-iscsi.args
index 53b3821e..23ef32f2 100644
--- a/tests/qemuxml2argvdata/disk-network-iscsi.args
+++ b/tests/qemuxml2argvdata/disk-network-iscsi.args
@@ -30,7 +30,7 @@ server,nowait \
if=none,id=drive-virtio-disk0 \
-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
id=virtio-disk0,bootindex=1 \
--drive file=iscsi://example.org:6000/iqn.1992-01.com.example/1,format=raw,\
+-drive file=iser://example.org:6000/iqn.1992-01.com.example/1,format=raw,\
if=none,id=drive-virtio-disk1 \
-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\
id=virtio-disk1 \
@@ -47,4 +47,8 @@ id=virtio-disk3 \
-drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,format=raw,\
if=none,id=drive-scsi0-0-0-0 \
-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
-drive=drive-scsi0-0-0-0,id=scsi0-0-0-0
+drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
+-drive file=iser://example.org:3260/iqn.1992-01.com.example/1,format=raw,\
+if=none,id=drive-scsi0-0-0-1 \
+-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\
+drive=drive-scsi0-0-0-1,id=scsi0-0-0-1
diff --git a/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-2.12.0.args
b/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-2.12.0.args
index 930d8d5d..b6b89912 100644
--- a/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-2.12.0.args
+++ b/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-2.12.0.args
@@ -34,7 +34,7 @@ if=none,id=drive-virtio-disk0 \
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
id=virtio-disk0,bootindex=1 \
-drive file.driver=iscsi,file.portal=example.org:6000,\
-file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,format=raw,\
+file.target=iqn.1992-01.com.example,file.lun=1,file.transport=iser,format=raw,\
if=none,id=drive-virtio-disk1 \
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\
id=virtio-disk1 \
@@ -61,6 +61,11 @@
file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,format=raw,\
if=none,id=drive-scsi0-0-0-0 \
-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \
+-drive file.driver=iscsi,file.portal=example.org:3260,\
+file.target=iqn.1992-01.com.example,file.lun=1,file.transport=iser,format=raw,\
+if=none,id=drive-scsi0-0-0-1 \
+-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\
+drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-latest.args
index 98481280..716646d5 100644
--- a/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-network-iscsi.x86_64-latest.args
@@ -31,47 +31,54 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \
-blockdev '{"driver":"iscsi","portal":"example.org:6000",\
"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\
+"node-name":"libvirt-6-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-6-format","read-only":false,"driver":"raw",\
+"file":"libvirt-6-storage"}' \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-6-format,\
+id=virtio-disk0,bootindex=1 \
+-blockdev '{"driver":"iscsi","portal":"example.org:6000",\
+"target":"iqn.1992-01.com.example","lun":1,"transport":"iser",\
"node-name":"libvirt-5-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-5-format","read-only":false,"driver":"raw",\
"file":"libvirt-5-storage"}' \
--device virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-5-format,\
-id=virtio-disk0,bootindex=1 \
+-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=libvirt-5-format,\
+id=virtio-disk1 \
+-object secret,id=libvirt-4-storage-auth-secret0,\
+data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
+keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
-blockdev '{"driver":"iscsi","portal":"example.org:6000",\
-"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\
+"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\
+"user":"myname","password-secret":"libvirt-4-storage-auth-secret0",\
"node-name":"libvirt-4-storage","auto-read
[PATCH v2 6/6] news: Support iSCSI iSER
Signed-off-by: Han Han --- docs/news.xml | 10 ++ 1 file changed, 10 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 4cef804a..73e58a8f 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,16 @@ + + + qemu: support iSCSI iSER + + + The iSCSI Extensions for RDMA, also named iSER, is introduce to QEMU + since 2.9. In libvirt, it can be used by +in the iscsi source. + + -- 2.25.0
[PATCH v2 1/6] qemu_capabilities: Introduce iSCSI iSER flag
Introduce QEMU_CAPS_ISCSI_ISER for iSER(iSCSI Extensions for RDMA) of
iscsi backend.
Signed-off-by: Han Han
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml| 1 +
tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 +
38 files changed, 43 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7e711f22..c8e0169a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -582,6 +582,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"tcg",
"virtio-blk-pci.scsi.default.disabled",
"pvscsi",
+
+ /* 370 */
+ "iscsi.iser",
);
@@ -1501,6 +1504,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "query-named-block-nodes/arg-type/flat",
QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT },
{ "blockdev-snapshot/$allow-write-only-overlay",
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY },
{ "blockdev-add/arg-type/+file/aio/^io_uring", QEMU_CAPS_AIO_IO_URING },
+{ "blockdev-add/arg-type/+iscsi/transport/^iser", QEMU_CAPS_ISCSI_ISER },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6bfc7386..c5ef3a26 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -564,6 +564,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_VIRTIO_BLK_SCSI_DEFAULT_DISABLED, /* virtio-blk-pci.scsi
disabled by default */
QEMU_CAPS_SCSI_PVSCSI, /* -device pvscsi */
+/* 370 */
+QEMU_CAPS_ISCSI_ISER, /* -blockdev
{"driver":"iscsi","transport":"iser",...} */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
index cfa1962e..5172dd56 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml
@@ -142,6 +142,7 @@
+
201
0
61700287
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
index 0eee26c3..29822be8 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml
@@ -142,6 +142,7 @@
+
201
0
42900287
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
index 7b14f7c2..38ceaaf3 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
@@ -108,6 +108,7 @@
+
201
0
39100287
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
index 205a0675..a85145f5 100644
---
[PATCH v2 2/6] conf: Parse the iser element
Now libvirt supports to parse the iser element in source. It will be
used to enable the iSCSI iSER.
Signed-off-by: Han Han
---
src/conf/domain_conf.c| 10 ++
src/util/virstoragefile.c | 17 ++---
src/util/virstoragefile.h | 2 ++
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c201fc90..9008eead 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9488,6 +9488,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
g_autofree char *haveTLS = NULL;
g_autofree char *tlsCfg = NULL;
g_autofree char *sslverifystr = NULL;
+int iscsiIser = 0;
xmlNodePtr tmpnode;
if (!(protocol = virXMLPropString(node, "protocol"))) {
@@ -9598,6 +9599,12 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
}
}
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) {
+if ((iscsiIser = virXPathBoolean("boolean(./iser)", ctxt)) == -1)
+return -1;
+src->iscsiIser = (bool)iscsiIser;
+}
+
return 0;
}
@@ -24944,6 +24951,9 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
virStorageSourceInitiatorFormatXML(&src->initiator, childBuf);
+if (src->iscsiIser)
+virBufferAddLit(childBuf, "\n");
+
if (src->sslverify != VIR_TRISTATE_BOOL_ABSENT) {
virBufferAsprintf(childBuf, "\n",
virTristateBoolTypeToString(src->sslverify));
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 535a94e2..fd687e62 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2805,8 +2805,15 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src,
if (!(scheme = virStringSplit(uri->scheme, "+", 2)))
return -1;
+src->protocol = virStorageNetProtocolTypeFromString(scheme[0]);
+
+if (STREQ(scheme[0], "iser")) {
+src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
+src->iscsiIser = true;
+}
+
if (!scheme[0] ||
-(src->protocol = virStorageNetProtocolTypeFromString(scheme[0])) < 0) {
+src->protocol < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("invalid backing protocol '%s'"),
NULLSTR(scheme[0]));
@@ -3509,12 +3516,16 @@
virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src,
src->nhosts = 1;
-if (STRNEQ_NULLABLE(transport, "tcp")) {
+if (STRNEQ_NULLABLE(transport, "tcp") &&
+STRNEQ_NULLABLE(transport, "iser")) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("only TCP transport is supported for iSCSI volumes"));
+ _("only TCP or iSER transport is supported for iSCSI
volumes"));
return -1;
}
+if (STREQ(transport, "iser"))
+src->iscsiIser = true;
+
src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
if (!portal) {
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index c68bdc96..e1773ba0 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -383,6 +383,8 @@ struct _virStorageSource {
/* these must not be used apart from formatting the output JSON in the
qemu driver */
char *ssh_user;
bool ssh_host_key_check_disabled;
+
+bool iscsiIser; /* An optional setting for iSCSI iSER transport*/
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);
--
2.25.0
[PATCH v2 4/6] docs: Support iSCSI iser
Signed-off-by: Han Han --- docs/formatdomain.html.in | 5 + docs/schemas/domaincommon.rng | 5 + 2 files changed, 10 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 23eb0292..78fd8546 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3448,6 +3448,11 @@ initiator IQN needed to access the source via mandatory attribute name. + iser + Since libvirt 6.4.0 and QEMU 2.9, +For iscsi, the element iser is used to +enable the iSER(iSCSI Extensions for RDMA). + address For disk of type nvme this element specifies the PCI address of the host NVMe diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9d60b090..e722069c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1810,6 +1810,11 @@ + + + + + -- 2.25.0
[PATCH v2 3/6] qemu: Implement the iSCSI iSER
The iSCSI iSER transport is introdcued since QEMU 2.9. For blockdev json,
it will appear at 'transport' field:
'json:{...,{"driver": "iscsi","transport":"iser",...}}'
For legacy drive filename as iscsi uri, it will start with 'iser'
scheme:
iser://[[username][%]@][:]//
Signed-off-by: Han Han
---
src/qemu/qemu_block.c | 11 +--
src/qemu/qemu_command.c | 3 +++
src/qemu/qemu_domain.c | 9 +
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 6f9c7071..4261e110 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -421,7 +421,10 @@ qemuBlockStorageSourceGetURI(virStorageSourcePtr src)
if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
uri->port = src->hosts->port;
-uri->scheme =
g_strdup(virStorageNetProtocolTypeToString(src->protocol));
+if (src->iscsiIser)
+uri->scheme = g_strdup("iser");
+else
+uri->scheme =
g_strdup(virStorageNetProtocolTypeToString(src->protocol));
} else {
uri->scheme = g_strdup_printf("%s+%s",
virStorageNetProtocolTypeToString(src->protocol),
@@ -746,6 +749,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
char *lunStr = NULL;
char *username = NULL;
char *objalias = NULL;
+const char *transport = "tcp";
g_autofree char *portal = NULL;
unsigned int lun = 0;
virJSONValuePtr ret = NULL;
@@ -761,6 +765,9 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
* }
*/
+if (src->iscsiIser)
+transport = "iser";
+
target = g_strdup(src->path);
/* Separate the target and lun */
@@ -791,7 +798,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
"s:portal", portal,
"s:target", target,
"u:lun", lun,
- "s:transport", "tcp",
+ "s:transport", transport,
"S:user", username,
"S:password-secret", objalias,
"S:initiator-name",
src->initiator.iqn,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bfe70ed2..1d5a302a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5087,6 +5087,9 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
qemuDomainStorageSourcePrivatePtr srcPriv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+if (qemuDomainValidateStorageSource(iscsisrc->src, qemuCaps,
true) < 0)
+return -1;
+
if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ?
srcPriv->secinfo :
NULL) < 0)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a1b250fd..41e97bf7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5318,6 +5318,15 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
return -1;
}
+if (qemuCaps) {
+if (src->iscsiIser &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_ISER)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+_("iSCSI iSER is not supported with this QEMU binary"));
+return -1;
+}
+}
+
return 0;
}
--
2.25.0
[PATCH v2 0/6] qemu: Support iSCSI iSER
The iSER[1](iSCSI Extensions for RDMA) transport is introduced since QEMU 2.9 [2]. It is only valid in iscsi network disk. Note that for the legacy uri of iscsi iser transport, it will start with 'iser' instead of 'iscsi'. Diff from v1: Use iser element of the source to enable iSER instead of the transport attribute. [1]: https://tools.ietf.org/html/rfc5046#section-1.7 [2]: https://github.com/qemu/qemu/blob/ee573f5326046223b6eef4ae7fbfec31d274e093/qapi/block-core.json#L3507 git repo: https://gitlab.com/hhan2/libvirt/-/commits/iser-v2 v1: https://www.redhat.com/archives/libvir-list/2020-April/msg01247.html Han Han (6): qemu_capabilities: Introduce iSCSI iSER flag conf: Parse the iser element qemu: Implement the iSCSI iSER docs: Support iSCSI iser tests: unit tests for iSCSI iSER news: Support iSCSI iSER docs/formatdomain.html.in | 5 +++ docs/news.xml | 10 + docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c| 10 + src/qemu/qemu_block.c | 11 - src/qemu/qemu_capabilities.c | 4 ++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_domain.c| 9 src/util/virstoragefile.c | 17 +-- src/util/virstoragefile.h | 2 + .../caps_2.10.0.aarch64.xml | 1 + .../caps_2.10.0.ppc64.xml | 1 + .../caps_2.10.0.s390x.xml | 1 + .../caps_2.10.0.x86_64.xml| 1 + .../caps_2.11.0.s390x.xml | 1 + .../caps_2.11.0.x86_64.xml| 1 + .../caps_2.12.0.aarch64.xml | 1 + .../caps_2.12.0.ppc64.xml | 1 + .../caps_2.12.0.s390x.xml | 1 + .../caps_2.12.0.x86_64.xml| 1 + .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + .../caps_2.9.0.x86_64.xml | 1 + .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 1 + .../caps_3.0.0.riscv32.xml| 1 + .../caps_3.0.0.riscv64.xml| 1 + .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + .../caps_3.0.0.x86_64.xml | 1 + .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 1 + .../caps_3.1.0.x86_64.xml | 1 + .../caps_4.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + .../caps_4.0.0.riscv32.xml| 1 + .../caps_4.0.0.riscv64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml| 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../qemuxml2argvdata/disk-network-iscsi.args | 8 +++- .../disk-network-iscsi.x86_64-2.12.0.args | 7 ++- .../disk-network-iscsi.x86_64-latest.args | 45 +++ tests/qemuxml2argvdata/disk-network-iscsi.xml | 9 tests/qemuxml2argvtest.c | 5 ++- .../qemuxml2xmloutdata/disk-network-iscsi.xml | 10 + tests/qemuxml2xmltest.c | 4 +- tests/virstoragetest.c| 16 +++ 55 files changed, 190 insertions(+), 29 deletions(-) -- 2.25.0
Re: [PATCH v3 5/9] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY
On 5/14/20 11:32 AM, Daniel Henrique Barboza wrote:
On 5/14/20 11:09 AM, Ján Tomko wrote:
On a Wednesday in 2020, Daniel Henrique Barboza wrote:
Aside from trivial XML parsing/format changes, this patch adds
additional rules for TPM device support to better accomodate
[...]
Any reason why you store them separately?
It seems they are treated the same in every place except when building
QEMU command line. Switching to a def->tpms array would better reflect
the XML. The Validate function would then check wheteher there's just
one copy of each device type.
It is an attempt to minimize the amount of changes needed. For example, making
a def->tpms array would impact all the code related to the 'emulator' TPM type,
in particular the files qemu_tpm.c and qemu_extdevice.c, that would need to
handle an array instead of the def->tpm pointer.
It is also an attempt of making it easier for any future "TPM Proxy" device
style to be added in the future. Instead of revisiting the logic inside each
def->tpms loop one would simply deal with the def->tpmproxy pointer directly.
And, in the case this is wrong and no one else cares about it, at the very
least we didn't change the design of all TPM devices because of one single
PPC64 specific model.
This is all up to debate, of course. If we we decide that changing to a
def->tpms
array is worth the extra lines of code I'll make it happen in the v4.
I'm experimenting with the idea of def->tpms array and, aside from having to
change additional files, it's not as bad as I thought. What I'm doing here
to mitigate the changes in the 'emulator' backend code is to always assure that
the 'emulator' device, if present, will be always at def->tpms[0]. This
makes most changes in qemu_tpm.c and qemu_extdevice.c a replace of "def->tpm" to
"def->tpms[0]", instead of having to insert a for loop to interact with
the array.
This also has the benefit of not having to duplicate the handling of def->tpm
for the def->tpmproxy pointer, as Jano mentioned above.
Jano, what do you think about this idea of making def->tpms[0] always a
non-proxy
device?
DHB
Thanks,
DHB
Jano
+ if (def->tpm) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM non-proxy device is
supported"));
+ goto error;
+ }
+ def->tpm = g_steal_pointer(&dev);
+ }
+ }
}
VIR_FREE(nodes);
[libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'
Although the original patches to support controllers with
hotplug='off' were checking during hotplug/attach requests that the
device was being plugged into a PCI controller that didn't have
hotplug disabled, but I forgot to do the same for device detach (the
main impetus for adding the feature was to prevent unplugs originating
from within the guest, so it slipped my mind). So although the guest
OS was ultimately unable to honor the unplug request, libvirt could
still be used to make such a request, and since device attach/detach
are asynchronous operations, the caller to libvirt would receive a
success status back (the device would stubbornly/correctly remain in
the domain status XML however)
This patch remedies that, by looking at the controller for the device
in the detach request, and immediately failing the operation if that
controller has hotplug=off.
r changes. Lines starting
Signed-off-by: Laine Stump
---
src/qemu/qemu_hotplug.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ab5a7aef84..5fe125b1a7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
return -1;
}
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+int controllerIdx = virDomainControllerFind(vm->def,
+
VIR_DOMAIN_CONTROLLER_TYPE_PCI,
+info->addr.pci.bus);
+if (controllerIdx < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot hot unplug %s device with PCI guest
address: "
+ VIR_PCI_DEVICE_ADDRESS_FMT
+ " - controller not found"),
+ virDomainDeviceTypeToString(detach.type),
+ info->addr.pci.domain, info->addr.pci.bus,
+ info->addr.pci.slot, info->addr.pci.function);
+return -1;
+}
+
+if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug
+== VIR_TRISTATE_SWITCH_OFF) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot hot unplug %s device with PCI guest
address: "
+ VIR_PCI_DEVICE_ADDRESS_FMT
+ " - not allowed by controller"),
+ virDomainDeviceTypeToString(detach.type),
+ info->addr.pci.domain, info->addr.pci.bus,
+ info->addr.pci.slot, info->addr.pci.function);
+return -1;
+}
+}
/*
* Issue the qemu monitor command to delete the device (based on
* its alias), and optionally wait a short time in case the
--
2.25.4
Re: [libvirt-csharp 1/3] Update to target newer 4.0 .NET framework version
On Thu, May 14, 2020 at 06:46:55PM +0200, Andrea Bolognani wrote:
> On Tue, 2020-05-12 at 11:23 +0100, Daniel P. Berrangé wrote:
> > +++ b/examples/MonoDevelop/virDomainStats/virDomainStats.csproj
> > @@ -1 +1,56 @@
> > -
> > > xmlns="http://schemas.microsoft.com/developer/msbuild/2003";>
> >
> > Debug
> > AnyCPU
> > 9.0.21022
> > 2.0
> > {767373FC-96BE-420A-8219-97146D33B2CB}
> > WinExe
> > virDomainStats
> > virDomainStats
> > v3.5
>
> The patch seems to have gotten corrupted somehow, since all the
> leading "-"s are missing from this part and the diff is larger than
> it is for other files for not apparent reason.
I think your mail client has just inserted fake line breaks. This original
file had its entire contents on a single line, which is fixed in the new
version :-)
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt-appdev-guide-python 2/2] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Tue, 2020-05-12 at 11:26 +0100, Daniel P. Berrangé wrote: > +++ b/CONTRIBUTING.rst > @@ -0,0 +1,28 @@ > +== > +Contributing to libvirt-appdev-guide-python > +== The title markers don't match the length of the text. With that fixed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-appdev-guide-python 1/2] gitlab: introduce CI jobs for building content
On Tue, 2020-05-12 at 11:25 +0100, Daniel P. Berrangé wrote: > The docs build needs to validate one axis > > - A variety of publican versions > > We get coverage for this by running builds across various distros. > The CentOS 8 build is picked as the special one, from which we Based on the patch, this should be s/8/7/, since we don't build on CentOS 8 at all. But see below. > +# centos-7-docs is special as it is the one > +# we publish from > +pages: > + <<: *docs_job_definition > + variables: > +NAME: centos-7 > + artifacts: > +paths: > + - public We should drop support for RHEL 7 in about a year, so I suggest we run this job on something with a longer shelf life, like for example Ubuntu 20.04, to reduce churn. > +++ b/ci/refresh > @@ -0,0 +1,22 @@ > +#!/bin/sh > + > +if test -z "$1" > +then > +echo "syntax: $0 PATH-TO-LCITOOL" > +exit 1 > +fi > + > +LCITOOL=$1 > + > +if ! test -x "$LCITOOL" > +then > +echo "$LCITOOL is not executable" > +exit 1 > +fi > + > +HOSTS=$($LCITOOL hosts | grep -v -E '(freebsd|centos-8|opensus)') s/opensus/opensuse/ -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH] xenconfig: Add feature gfx_passthru
On 14.05.2020 03:41, Marek Marczykowski-Górecki wrote: > On Wed, May 13, 2020 at 01:56:52PM +0200, Artur Puzio wrote: >> >> On 12.05.2020 15:47, Artur Puzio wrote: >>> On 08.05.2020 18:41, Jim Fehlig wrote: On 4/30/20 6:07 AM, Artur Puzio wrote: > gfx_passthru xl.cfg option enables GPU specific quirks required for > working > Intel GPU passthru. Qemu (used for device model by xen) will refuse > to start > a VM when an IGD is passed, but this option was not set in Xen. Do we really need to expose this setting to the user? I'm not really sure what to think about it after reading the xl.cfg(5) man page. It starts with   gfx_passthru=BOOLEAN|"STRING" The setting can be a boolean or a string - nice. And it really seems specific to Intel graphics cards. The man page claims that a value of 1 "Enables graphics device PCI passthrough and autodetects the type of device which is being used.". It also says "Note that some graphics cards (AMD/ATI cards, for example) do not necessarily require the gfx_passthru option". Can't libxl just enable this itself if there's a PCI passthrough device and it's detected as type igd? >> I have checked possible ways for detection and reusing existing libxl >> functions is not possible. Most of relevant functions (including >> libxl__detect_gfx_passthru_kind & libxl__is_igd_vga_passthru) use >> libxl__gc (libxl garbage collector). AFAIK no libvirt code uses >> libxl__gc. We probably shouldn't start doing that(?) Also >> libxl__detect_gfx_passthru_kind is static. >> >> So the only way would be to reimplement detection if IGD is attached. >> Even if it would work the same as libxl detection, it would probably >> stop in the future... I think just exposing the option to the user is >> much, much easier and future proof. > Existing strategies: > 1. libxl__is_igd_vga_passthru checks for Intel device (0x8086 vendor) with > VGA class (0x03). This should be easy to duplicate using libvirt > API, like virPCIDeviceReadID(dev, "vendor") and same for "class". > > 2. qemu uses a different approach and checks for device being > :00:02.0. Again, simple to duplicate into libvirt. > > As we can see above, there are already two approaches, and it isn't > exactly clear which should be used. In fact, I think if those two > strategies won't match at some point, the domain may fail to start, > because qemu refuses to start with IGD attached (according to its > definition) but without the option enabled. > > Looking at the libxl code, the only thing that option is used for, is to > add it to the qemu command line (Artur, have I missed anything?). It's also used in libxl_pci.c:2449 in func: libxl__grant_vga_iomem_permission. This check is not really necessary as libxl__grant_vga_iomem_permission already grants VGA IO memory permissions only if it finds a VGA class device. > In that case, I think ideal solution would be to patch it at the qemu > level, to enable the option automatically. I believe this is exactly > the case for KVM case (vfio-pci). But I think that's non-trivial > approach in Xen path (if it would be easy, I think we wouldn't have this > option in the first place). For example the option makes qemu choose a > different PCI bridge configuration, which is I think done before > checking if IGD is attached. So, while there maybe a technical reason > for having this option in qmeu (instead of auto-detection), I don't > really see why user needs to provide it manually. > > Since libxl already has a function to detect IGD, then adding > autodetection there would be a better idea. I don't know what libxl > maintainers would prefer, but I guess either of "not failing if > gfx_passthru=1 but IGD isn't detected" or adding "gfx_passthru=auto". I will go ahead and contact libxl maintainers if they would agree to some autodetection solution. Maybe in the mean time we could expose this option to the user in case he want's to disable/enable the autodetection explicitly? When Xen will have some autodetection we could change the default. > BTW Here is the discussion when the option was added to qemu: > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg02756.html > It may be quite helpful to understand what is it about, but also why > (for example I see mentions of some specific Windows drivers). > signature.asc Description: OpenPGP digital signature
Re: [libvirt-csharp 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Tue, 2020-05-12 at 11:23 +0100, Daniel P. Berrangé wrote: > With the introduction of automated CI pipelines, we are now ready to switch > to using merge requests for the project. With this switch we longer wish > to have patches sent to the mailing list. > > Signed-off-by: Daniel P. Berrangé > --- > .gitpublish | 4 > CONTRIBUTING.rst | 28 > 2 files changed, 28 insertions(+), 4 deletions(-) > delete mode 100644 .gitpublish > create mode 100644 CONTRIBUTING.rst Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-csharp 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
On Tue, 2020-05-12 at 11:23 +0100, Daniel P. Berrangé wrote: > The csharp build needs to validate two axis > > - A variety of libvirt versions > - A variety of csharp versions > > We get coverage for both these axis by running a build against the > distro provided libvirt packages. All that is then missing is a build > against the latest libvirt git master, which only needs to be run on > a single distro, for which Fedora 32 is picked. > > Latest Debian, Ubuntu, openSUSE and CentOS all stopped shipping the > monodevelop package, pointing people to flatpaks instead. Thus the > set of distros built is somewhat limited > > Signed-off-by: Daniel P. Berrangé > --- > .gitlab-ci.yml | 96 > ci/README.rst| 14 > ci/libvirt-debian-9.Dockerfile | 58 + > ci/libvirt-fedora-31.Dockerfile | 52 +++ > ci/libvirt-fedora-32.Dockerfile | 61 ++ > ci/libvirt-fedora-rawhide.Dockerfile | 53 +++ > ci/refresh | 27 > 7 files changed, 361 insertions(+) > create mode 100644 ci/README.rst > create mode 100644 ci/libvirt-debian-9.Dockerfile > create mode 100644 ci/libvirt-fedora-31.Dockerfile > create mode 100644 ci/libvirt-fedora-32.Dockerfile > create mode 100644 ci/libvirt-fedora-rawhide.Dockerfile > create mode 100755 ci/refresh Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-csharp 1/3] Update to target newer 4.0 .NET framework version
On Tue, 2020-05-12 at 11:23 +0100, Daniel P. Berrangé wrote:
> +++ b/examples/MonoDevelop/virDomainStats/virDomainStats.csproj
> @@ -1 +1,56 @@
> -
> xmlns="http://schemas.microsoft.com/developer/msbuild/2003";>
>
> Debug
> AnyCPU
> 9.0.21022
> 2.0
> {767373FC-96BE-420A-8219-97146D33B2CB}
> WinExe
> virDomainStats
> virDomainStats
> v3.5
The patch seems to have gotten corrupted somehow, since all the
leading "-"s are missing from this part and the diff is larger than
it is for other files for not apparent reason.
Anyway, the changes look reasonable so, assuming your local version
of the patch is not corrupted and you've tested it thoroughly,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
Re: [ruby-libvirt 3/3] Remove obsolete mercurial ignore file
On Tue, 2020-05-12 at 11:21 +0100, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé > --- > .hgignore | 7 --- > 1 file changed, 7 deletions(-) > delete mode 100644 .hgignore Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [ruby-libvirt 2/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Tue, 2020-05-12 at 11:21 +0100, Daniel P. Berrangé wrote: > +++ b/CONTRIBUTING.rst > @@ -0,0 +1,28 @@ > + > +Contributing to ruby-libvirt > + > + > +The libvirt Ruby API binding accepts code contributions via merge requests > +on the GitLab project: > + > +https://gitlab.com/libvirt/ruby-libvirt/-/merge_requests > + > +It is required that automated CI pipelines succeed before a merge request > +will be accepted. The global pipeline status for the ``master`` branch is > +visible at: > + > +https://gitlab.com/libvirt/ruby-libvirt/pipelines > + > +CI pipeline results for merge requests will be visible via the contributors' > +own private repository fork: > + > +https://gitlab.com/yourusername/ruby-libvirt/pipelines > + > +Contributions submitted to the project must be in compliance with the > +Developer Certificate of Origin Version 1.1. This is documented at: > + > +https://developercertificate.org/ > + > +To indicate compliance, each commit in a series must have a "Signed-off-by" > +tag with the submitter's name and email address. This can be added by passing > +the ``-s`` flag to ``git commit`` when creating the patches. Assuming we rename the repo before merging this series, s/ruby-libvirt/libvirt-ruby/g -- Andrea Bolognani / Red Hat / Virtualization
Re: [ruby-libvirt 1/3] gitlab: introduce CI jobs testing git master & distro libvirt
On Tue, 2020-05-12 at 11:21 +0100, Daniel P. Berrangé wrote: > +.container_job_template: &container_job_definition > + image: docker:stable > + stage: containers > + services: > +- docker:dind > + before_script: > +- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest" > +- export COMMON_TAG="$CI_REGISTRY/libvirt/ruby-libvirt/ci-$NAME:latest" Can we rename the repo to libvirt-ruby already? > +.dist_build_job_template: &dist_build_job_definition > + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest > + stage: builds > + before_script: > +- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)" > + script: > +- rake build > +- if test "$NAME" != "opensuse-151" ; then rake package; fi Same comment made for the Java bindings: please make this an optional part of the build that can be skipped by using the appropriate variable at job definition time. Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-java PATCH 0/6] Enable GitLab CI and merge requests
On Tue, 2020-05-12 at 10:51 +0100, Daniel P. Berrangé wrote: > This fixes a bugs from bit-rot and then enables GitLab CI and use of > merge requests for contribution. > > Daniel P. Berrangé (6): > Fix test driver connection type > Add workaround for broken screenshot API on Ubuntu 18.04 vintage > Remove reference to test.java file that was deleted I looked at these first three patches and didn't really see anything obviously wrong with them, so under the assumption that you know what you're doing and you've tested your changes properly, Reviewed-by: Andrea Bolognani to them. > rpm: skip junit tests in RHEL-8 build > gitlab: introduce CI jobs testing git master & distro libvirt > gitlab: add CONTRIBUTING.rst file to indicate use of merge requests These I already commented on separately. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-java PATCH 4/6] rpm: skip junit tests in RHEL-8 build
On Tue, 2020-05-12 at 10:51 +0100, Daniel P. Berrangé wrote:
> +%if 0%{?rhel} != 8
> BuildRequires: ant-junit
> +%endif
This can probably be changed to
%if 0%{?rhel} < 8
under the assumption that packages that have been dropped from RHEL
8 are probably not going to be re-introduced in later major releases.
> +%if 0%{?rhel} != 8
> ant test
> +%endif
Same here.
With that changed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-java PATCH 6/6] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Tue, 2020-05-12 at 10:51 +0100, Daniel P. Berrangé wrote: > With the introduction of automated CI pipelines, we are now ready to switch > to using merge requests for the project. With this switch we longer wish > to have patches sent to the mailing list, and thus the git-publish > config is removed. > > Signed-off-by: Daniel P. Berrangé > --- > .gitpublish | 4 > CONTRIBUTING.rst | 28 > 2 files changed, 28 insertions(+), 4 deletions(-) > delete mode 100644 .gitpublish > create mode 100644 CONTRIBUTING.rst Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-java PATCH 5/6] gitlab: introduce CI jobs testing git master & distro libvirt
On Tue, 2020-05-12 at 10:51 +0100, Daniel P. Berrangé wrote: > +.dist_build_job_template: &dist_build_job_definition > + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest > + stage: builds > + script: > +- ant build jar docs > +- if test "$NAME" != "centos-8" ; then ant test ; fi I don't like the fact that you hardcode the name of a specific target in the build steps. We should do something similar to what I've implemented for libosinfo instead: in the script, use - if test "$TESTS" != "skip"; then ant test; fi so then, in the job definitions for CentOS 8, you can do centos-8-dist-build: <<: *dist_build_job_definition variables: NAME: centos-8 TESTS: skip The actual variable name and value can be different, of course, but you get the idea. > +fedora-32-git-build: > + <<: *git_build_job_definition > + variables: > +NAME: fedora-32 I understand that CentOS 8 is clearly not a good candidate for this job, but can we use something a little more long-term than Fedora to reduce churn? Ubuntu 20.04, for example. That would not give us coverage of the RPM build job, but honestly I don't think there's much of a chance of that breaking because of changes in *libvirt* git. Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 6/6] guests: add libvirt-publican and libvirt-appdev-guide-python projects
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > + publican: > +default: publican > +OpenSUSE: > +CentOS8: > +pkg: This should be publican: default: publican pkg: OpenSUSE: CentOS8: With that fixed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 4/6] guests: pull in the full JDK, not merely the JRE
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > The libvirt-java binding needs the JDK to build successfully, as that > provides the compiler. > > Signed-off-by: Daniel P. Berrangé > --- > guests/vars/mappings.yml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 5/6] guests: include libxml2 explicitly for libvirt-php
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > Some platforms pull in libxml2 indirectly, but since libvirt-php needs > it, we should list it explicitly. > > Signed-off-by: Daniel P. Berrangé > --- > guests/vars/projects/libvirt-php.yml | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 3/6] guests: add libvirt-csharp project packages
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > + mono: > +default: > +Fedora: mono-devel > +Debian9: mono-devel > + > + monodevelop: > +default: > +Fedora: monodevelop > +Debian9: monodevelop You don't need the 'default' entries. With those removed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 2/6] guests: add libvirt-java project packages
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > + ant-junit: > +default: ant-junit > +deb: ant-optional > +CentOS8: > +pkg: It looks like FreeBSD has the required bits after all: $ pkg list apache-ant | grep -i -E 'junit.*jar' /usr/local/share/java/apache-ant/lib/ant-junit.jar /usr/local/share/java/apache-ant/lib/ant-junit4.jar /usr/local/share/java/apache-ant/lib/ant-junitlauncher.jar so I think this should be ant-junit: deb: ant-optional pkg: apache-ant rpm: ant-junit CentOS8: Assuming I'm correct, with that changed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-ci PATCH 1/6] guests: add ruby-libvirt project packages
On Mon, 2020-05-11 at 18:22 +0100, Daniel P. Berrangé wrote: > + ruby: > +default: ruby > +deb: ruby-dev > +rpm: ruby-devel This should be ruby: deb: ruby-dev pkg: ruby rpm: ruby-devel With that changed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
[PATCH] RFC: Support network interface downscript
https://gitlab.com/libvirt/libvirt/-/issues/13
Add support for downscript:
But -net ...,script=/etc/qemu-ifup has some limitation:
https://github.com/qemu/qemu/blob/035b448b84f3557206abc44d786c5d3db2638f7d/net/tap.c#L818
Maybe virNetDevRunEthernetScript is another choice.
Signed-off-by: Chen Hanxiao
---
src/conf/domain_conf.c| 7 +++
src/conf/domain_conf.h| 1 +
src/qemu/qemu_extdevice.c | 2 ++
3 files changed, 10 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c201fc9..5cb39a3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2520,6 +2520,7 @@ virDomainNetDefClear(virDomainNetDefPtr def)
VIR_FREE(def->teaming.persistent);
VIR_FREE(def->virtPortProfile);
VIR_FREE(def->script);
+VIR_FREE(def->downscript);
VIR_FREE(def->domain_name);
VIR_FREE(def->ifname);
VIR_FREE(def->ifname_guest);
@@ -11977,6 +11978,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *ifname_guest = NULL;
g_autofree char *ifname_guest_actual = NULL;
g_autofree char *script = NULL;
+g_autofree char *downscript = NULL;
g_autofree char *address = NULL;
g_autofree char *port = NULL;
g_autofree char *localaddr = NULL;
@@ -12149,6 +12151,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
} else if (!script &&
virXMLNodeNameEqual(cur, "script")) {
script = virXMLPropString(cur, "path");
+} else if (!downscript &&
+ virXMLNodeNameEqual(cur, "downscript")) {
+downscript = virXMLPropString(cur, "path");
} else if (!domain_name &&
virXMLNodeNameEqual(cur, "backenddomain")) {
domain_name = virXMLPropString(cur, "name");
@@ -12482,6 +12487,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
if (script != NULL)
def->script = g_steal_pointer(&script);
+if (downscript != NULL)
+def->downscript = g_steal_pointer(&downscript);
if (domain_name != NULL)
def->domain_name = g_steal_pointer(&domain_name);
if (ifname != NULL)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ddc75d8..e152c59 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1055,6 +1055,7 @@ struct _virDomainNetDef {
unsigned long sndbuf;
} tune;
char *script;
+char *downscript;
char *domain_name; /* backend domain name */
char *ifname; /* interface name on the host () */
int managed_tap; /* enum virTristateBool - ABSENT == YES */
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 2096272..f8e440a 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -234,6 +234,8 @@ qemuExtDevicesStop(virQEMUDriverPtr driver,
if (slirp)
qemuSlirpStop(slirp, vm, driver, net);
+if (net->downscript)
+virNetDevRunEthernetScript(net->ifname, net->downscript);
}
for (i = 0; i < def->nfss; i++) {
--
1.8.3.1
[PATCH v2 3/3] qemuBuildNumaArgStr: Use modern -numa memdev= if old -numa mem= is unsupported
In previous commit we started tracking whether QEMU supports
'-numa mem='. This is tied to the machine type because migration
from '-numa mem=' to '-numa memdev' is impossible (or vice
versa). But since it's tied to a machine type (where migration
from one to another is also unsupported) we can allow QEMU to get
rid of the deprecated command line.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cf6c48d233..19c45cb4e3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7038,6 +7038,11 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
if (!virDomainNumatuneNodesetIsAvailable(def->numa, priv->autoNodeset))
goto cleanup;
+if (!virQEMUCapsGetMachineNumaMemSupported(qemuCaps,
+ def->virtType,
+ def->os.machine))
+needBackend = true;
+
if (VIR_ALLOC_N(nodeBackends, ncells) < 0)
goto cleanup;
@@ -7055,6 +7060,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
if (rc == 0)
needBackend = true;
}
+} else if (needBackend) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("numa not supported"));
+goto cleanup;
}
if (!needBackend &&
--
2.26.2
[PATCH v2 2/3] qemuBuildNumaArgStr: Switch order of if() and for()
When building -numa command line there is a for() loop that
builds '-numa memdev=' for each guest NUMA node. And also
records in a local variable whether any of memory-object-*
backends must be used to satisfy desired config. Well, instead of
checking in each iteration whether corresponding capabilities are
set, we can do swap if() and for() and check only once.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1783355
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bfe70ed228..cf6c48d233 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7043,11 +7043,11 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
/* using of -numa memdev= cannot be combined with -numa mem=, thus we
* need to check which approach to use */
-for (i = 0; i < ncells; i++) {
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD)) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD)) {
+for (i = 0; i < ncells; i++) {
if ((rc = qemuBuildMemoryCellBackendStr(def, cfg, i, priv,
&nodeBackends[i])) < 0)
goto cleanup;
--
2.26.2
[PATCH v2 0/3] qemu: Use -numa memdev= if -numa mem= is unavailable
v2 of: https://www.redhat.com/archives/libvir-list/2020-May/msg00490.html diff to v1: - different approach used. I completely forgot about the discussion we had, but after Dan pointed it out to me, it clicked. Michal Prívozník (3): qemu: Track numa-mem-supported machine attribute qemuBuildNumaArgStr: Switch order of if() and for() qemuBuildNumaArgStr: Use modern -numa memdev= if old -numa mem= is unsupported src/qemu/qemu_capabilities.c | 44 ++- src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_capspriv.h | 3 +- src/qemu/qemu_command.c | 17 +- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 11 + .../caps_1.5.3.x86_64.xml | 60 ++-- .../caps_1.6.0.x86_64.xml | 68 ++--- .../caps_1.7.0.x86_64.xml | 76 ++--- .../caps_2.1.1.x86_64.xml | 92 +++--- .../caps_2.10.0.aarch64.xml | 204 +++--- .../caps_2.10.0.ppc64.xml | 84 +++--- .../caps_2.10.0.s390x.xml | 28 +- .../caps_2.10.0.x86_64.xml| 140 +- .../caps_2.11.0.s390x.xml | 32 +-- .../caps_2.11.0.x86_64.xml| 140 +- .../caps_2.12.0.aarch64.xml | 228 +++ .../caps_2.12.0.ppc64.xml | 100 +++ .../caps_2.12.0.s390x.xml | 36 +-- .../caps_2.12.0.x86_64.xml| 148 +- .../caps_2.4.0.x86_64.xml | 116 .../caps_2.5.0.x86_64.xml | 124 .../caps_2.6.0.aarch64.xml| 164 +-- .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 60 ++-- .../caps_2.6.0.x86_64.xml | 100 +++ .../qemucapabilitiesdata/caps_2.7.0.s390x.xml | 16 +- .../caps_2.7.0.x86_64.xml | 108 +++ .../qemucapabilitiesdata/caps_2.8.0.s390x.xml | 20 +- .../caps_2.8.0.x86_64.xml | 124 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 80 +++--- .../qemucapabilitiesdata/caps_2.9.0.s390x.xml | 24 +- .../caps_2.9.0.x86_64.xml | 132 - .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 104 +++ .../caps_3.0.0.riscv32.xml| 10 +- .../caps_3.0.0.riscv64.xml| 10 +- .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 40 +-- .../caps_3.0.0.x86_64.xml | 156 +-- .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 108 +++ .../caps_3.1.0.x86_64.xml | 164 +-- .../caps_4.0.0.aarch64.xml| 264 +- .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 112 .../caps_4.0.0.riscv32.xml| 10 +- .../caps_4.0.0.riscv64.xml| 10 +- .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 48 ++-- .../caps_4.0.0.x86_64.xml | 164 +-- .../caps_4.1.0.x86_64.xml | 176 ++-- .../caps_4.2.0.aarch64.xml| 52 ++-- .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 72 ++--- .../caps_4.2.0.x86_64.xml | 184 ++-- .../caps_5.0.0.aarch64.xml| 52 ++-- .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 76 ++--- .../caps_5.0.0.x86_64.xml | 176 ++-- .../caps_5.1.0.x86_64.xml | 176 ++-- tests/testutilsqemu.c | 6 +- 54 files changed, 2408 insertions(+), 2345 deletions(-) -- 2.26.2
Re: [PATCH v3 5/9] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY
On 5/14/20 11:09 AM, Ján Tomko wrote:
On a Wednesday in 2020, Daniel Henrique Barboza wrote:
Aside from trivial XML parsing/format changes, this patch adds
additional rules for TPM device support to better accomodate
all the available scenarios with the new TPM Proxy.
The changes make no impact to existing domains. This means that
the scenario of a domain with a single TPM device is still
supported in the same way. The restriction of multiple TPM devices
got alleviated to allow a TPM Proxy device to be added together
with a TPM device in the same domain. All other combinations
are still forbidden.
To summarize, after this patch, the following combinations in the same
domain are valid:
- a single TPM device
- a single TPM Proxy device
- a single TPM + single TPM Proxy devices
These combinations in the same domain are NOT allowed:
- 2 or more TPM devices
- 2 or more TPM Proxy devices
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 45 ++
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01a32f62d1..8164cd58c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13730,6 +13730,14 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
+ /* TPM Proxy devices have 'passthrough' backend */
+ if (def->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY &&
+ def->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'Passthrough' backend is required for TPM Proxy
devices"));
+ goto error;
+ }
+
This check should be in a Validate function, not the parser.
Good catch.
if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
goto error;
@@ -21972,15 +21980,39 @@ virDomainDefParseXML(xmlDocPtr xml,
if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
goto error;
- if (n > 1) {
+ if (n > 2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("only a single TPM device is supported"));
+ _("a maximum of two TPM devices is supported, one of "
+ "them being a TPM Proxy device"));
goto error;
}
if (n > 0) {
- if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt,
flags)))
- goto error;
+ for (i = 0; i < n; i++) {
+ g_autoptr(virDomainTPMDef) dev = NULL;
+
+ if (!(dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt,
flags)))
+ goto error;
+
+ /* TPM Proxy devices must be held in def->tpmproxy. Error
+ * out if there's a TPM Proxy declared already */
+ if (dev->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
+ if (def->tpmproxy) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM Proxy device is
supported"));
+ goto error;
+ }
+ def->tpmproxy = g_steal_pointer(&dev);
+ } else {
+ /* all other TPM devices goes to def->tpm */
Any reason why you store them separately?
It seems they are treated the same in every place except when building
QEMU command line. Switching to a def->tpms array would better reflect
the XML. The Validate function would then check wheteher there's just
one copy of each device type.
It is an attempt to minimize the amount of changes needed. For example, making
a def->tpms array would impact all the code related to the 'emulator' TPM type,
in particular the files qemu_tpm.c and qemu_extdevice.c, that would need to
handle an array instead of the def->tpm pointer.
It is also an attempt of making it easier for any future "TPM Proxy" device
style to be added in the future. Instead of revisiting the logic inside each
def->tpms loop one would simply deal with the def->tpmproxy pointer directly.
And, in the case this is wrong and no one else cares about it, at the very
least we didn't change the design of all TPM devices because of one single
PPC64 specific model.
This is all up to debate, of course. If we we decide that changing to a
def->tpms
array is worth the extra lines of code I'll make it happen in the v4.
Thanks,
DHB
Jano
+ if (def->tpm) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM non-proxy device is
supported"));
+ goto error;
+ }
+ def->tpm = g_steal_pointer(&dev);
+ }
+ }
}
VIR_FREE(nodes);
Re: [PATCH v3 7/9] qemu: build command line for the TPM Proxy device
On a Wednesday in 2020, Daniel Henrique Barboza wrote:
This patch wraps it up all the wiring done in previous patches,
enabling a PPC64 guest to launch a guest using a TPM Proxy
device.
Note that device validation is already being done in qemu_validate.c,
qemuValidateDomainDeviceDefTPM(), on domain define time. We don't
need to verify QEMU capabilities for this device again inside
qemu_command.c.
Reviewed-by: Stefan Berger
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_alias.c | 16
src/qemu/qemu_command.c | 21 +
2 files changed, 37 insertions(+)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index b0ea62af39..08fe5aa501 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -413,6 +413,18 @@ qemuAssignDeviceTPMAlias(virDomainTPMDefPtr tpm,
}
+static int
+qemuAssignDeviceTPMProxyAlias(virDomainTPMDefPtr tpmproxy,
+ int idx)
+{
+if (tpmproxy->info.alias)
+return 0;
+
+tpmproxy->info.alias = g_strdup_printf("tpmproxy%d", idx);
+return 0;
+}
+
+
int
qemuAssignDeviceRedirdevAlias(virDomainDefPtr def,
virDomainRedirdevDefPtr redirdev,
@@ -673,6 +685,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
if (qemuAssignDeviceTPMAlias(def->tpm, 0) < 0)
return -1;
}
+if (def->tpmproxy) {
+if (qemuAssignDeviceTPMProxyAlias(def->tpmproxy, 0) < 0)
+return -1;
+}
for (i = 0; i < def->nmems; i++) {
if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0)
return -1;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bfe70ed228..0b97db7388 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8981,6 +8981,24 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
return 0;
}
+static int
+qemuBuildTPMProxyCommandLine(virCommandPtr cmd,
+ const virDomainDef *def)
+{
+const virDomainTPMDef *tpmproxy = def->tpmproxy;
+
+if (!tpmproxy)
+return 0;
+
+virCommandAddArg(cmd, "-device");
+virCommandAddArgFormat(cmd, "%s,id=%s,host-path=%s",
+ virDomainTPMModelTypeToString(tpmproxy->model),
+ tpmproxy->info.alias,
+ tpmproxy->data.passthrough.source.data.file.path);
The path is user-supplied and needs to be comma-escaped.
Jano
+
+return 0;
+}
+
static int
qemuBuildSEVCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
virDomainSEVDefPtr sev)
@@ -9662,6 +9680,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildTPMCommandLine(cmd, def, qemuCaps) < 0)
return NULL;
+if (qemuBuildTPMProxyCommandLine(cmd, def) < 0)
+return NULL;
+
if (qemuBuildInputCommandLine(cmd, def, qemuCaps) < 0)
return NULL;
--
2.26.2
signature.asc
Description: PGP signature
Re: [PATCH v3 5/9] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY
On a Wednesday in 2020, Daniel Henrique Barboza wrote:
Aside from trivial XML parsing/format changes, this patch adds
additional rules for TPM device support to better accomodate
all the available scenarios with the new TPM Proxy.
The changes make no impact to existing domains. This means that
the scenario of a domain with a single TPM device is still
supported in the same way. The restriction of multiple TPM devices
got alleviated to allow a TPM Proxy device to be added together
with a TPM device in the same domain. All other combinations
are still forbidden.
To summarize, after this patch, the following combinations in the same
domain are valid:
- a single TPM device
- a single TPM Proxy device
- a single TPM + single TPM Proxy devices
These combinations in the same domain are NOT allowed:
- 2 or more TPM devices
- 2 or more TPM Proxy devices
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 45 ++
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01a32f62d1..8164cd58c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13730,6 +13730,14 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
+/* TPM Proxy devices have 'passthrough' backend */
+if (def->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY &&
+def->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'Passthrough' backend is required for TPM Proxy
devices"));
+goto error;
+}
+
This check should be in a Validate function, not the parser.
if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
goto error;
@@ -21972,15 +21980,39 @@ virDomainDefParseXML(xmlDocPtr xml,
if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
goto error;
-if (n > 1) {
+if (n > 2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("only a single TPM device is supported"));
+ _("a maximum of two TPM devices is supported, one of "
+ "them being a TPM Proxy device"));
goto error;
}
if (n > 0) {
-if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt,
flags)))
-goto error;
+for (i = 0; i < n; i++) {
+g_autoptr(virDomainTPMDef) dev = NULL;
+
+if (!(dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt,
flags)))
+goto error;
+
+/* TPM Proxy devices must be held in def->tpmproxy. Error
+ * out if there's a TPM Proxy declared already */
+if (dev->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
+if (def->tpmproxy) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM Proxy device is
supported"));
+goto error;
+}
+def->tpmproxy = g_steal_pointer(&dev);
+} else {
+/* all other TPM devices goes to def->tpm */
Any reason why you store them separately?
It seems they are treated the same in every place except when building
QEMU command line. Switching to a def->tpms array would better reflect
the XML. The Validate function would then check wheteher there's just
one copy of each device type.
Jano
+if (def->tpm) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM non-proxy device is
supported"));
+goto error;
+}
+def->tpm = g_steal_pointer(&dev);
+}
+}
}
VIR_FREE(nodes);
signature.asc
Description: PGP signature
Re: [PATCH v3 1/9] docs: documentation and schema for the new TPM Proxy model
On a Wednesday in 2020, Daniel Henrique Barboza wrote: QEMU 4.1.0 introduced a new device type called TPM Proxy, currently implemented by PPC64 guests via a new virtual device called 'spapr-tpm-proxy' (see QEMU 0fb6bd073230 for more info). The TPM Proxy device interacts with a TPM Resource Manager, a host device capable of multiplexing the host TPM with multiple processes. This allows multiple guests to access some TPM features at the same time. Note that this mode of operation does not provide full TPM features to be available for the guest - for that case the guest still needs to assign a vTPM device (tpm-spapr for PPC64 guests). Although redundant, there is currently no technical limitation for a guest to assign both a vTPM and a TPM Proxy at the same time. This patch adds documentation and schema for a new TPM model type called 'spapr-tpm-proxy' that creates this new TPM Proxy device. This model is valid only for the 'passthrough' backend. An example of a TPM Proxy device connected to a TPM Resource Manager '/dev/tpmrm0' will look like this: Signed-off-by: Daniel Henrique Barboza --- docs/formatdomain.html.in | 18 +- docs/schemas/domaincommon.rng | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 23eb029234..15a92aa4f4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8792,6 +8792,17 @@ qemu-kvm -net nic,model=? /dev/null backend device is a TPM 2.0. Since 6.1.0, pSeries guests on PPC64 are supported and the default is tpm-spapr. + + Since 6.4.0, a new model called + spapr-tpm-proxy was added for pSeries guests. This model + only works with the 'passthrough' backend. It creates a TPM Proxy passthrough + device that communicates with an existing TPM Resource Manager in the host, + for example /dev/tpmrm0, enabling the guest to run in secure virtual machine /dev/tpmrm0 + mode with the help of an Ultravisor. Adding a TPM Proxy to a pSeries guest + brings no security benefits unless the guest is running on a PPC64 host that + has an Ultravisor and a TPM Resource Manager. Only one TPM Proxy device is + allowed per guest, but a TPM Proxy device can be added together with + other TPM devices. backend @@ -8804,7 +8815,7 @@ qemu-kvm -net nic,model=? /dev/null passthrough - Use the host's TPM device. + Use the host's TPM or TPM Resource Manager device. This backend type requires exclusive access to a TPM device on @@ -8812,6 +8823,11 @@ qemu-kvm -net nic,model=? /dev/null qualified file name is specified by path attribute of the source element. If no file name is specified then /dev/tpm0 is automatically used. + + Since 6.4.0, when choosing the + spapr-tpm-proxy model, the file name specified is + expected to be a TPM Resource Manager device, e.g. + /dev/tpmrm0. /dev/tpmrm0 signature.asc Description: PGP signature
Re: [PATCH v3 2/9] qemu: Extend QEMU capabilities with 'spapr-tpm-proxy'
On a Wednesday in 2020, Daniel Henrique Barboza wrote: Expose the TPM Proxy support for PPC64 guests by creating a new cap called QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY. This device is part of the machinery the guest need to orchestrate with the PPC64 Ultravisor the transition to the Secure VM (SVM) mode. Inside QEMU, this device will be used with the H_TPM_COMM hypercall to connect with the TPM Resource Manager, enabling the guest to open and close TPM sessions with the host TPM. Reviewed-by: Stefan Berger Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_capabilities.c| 4 src/qemu/qemu_capabilities.h| 3 +++ tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + 4 files changed, 9 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390
On Thu, 2020-05-14 at 15:34 +0200, Boris Fiuczynski wrote: > On 5/14/20 10:37 AM, Daniel P. Berrangé wrote: > > I don't see this existing behaviour as confusing. It looks like mostly > > being a docs ommission about auto-allocation taking place. > > Maybe I am repeating myself but I find e.g the below example confusing > if I take into consideration that uid=0 is NOT a valid value and fid is > a valid value. Please note that the valid fid is dislocated from its > original device! > > Specify this in the domain: > pcidev1: uid='0x' fid='0x' > pcidev2: uid='0x' > Results in a defined domain: > pcidev1: uid='0x0002' fid='0x0001' > pcidev2: uid='0x0001' fid='0x' > > If the user would be tying to fix the dislocating fid... one would very > likely try this: > Specify this in the domain: > pcidev1: uid='0x' fid='0x' > pcidev2: uid='0x' fid='0x0001' > Result: > error: Failed to define domain from mini-pcis.xml > error: XML error: Invalid PCI address uid='0x', must be > 0x and > <= 0x And partial assignments, which one might reasonably expect to work, actually don't: fid=0 -> interpreted the same as no information provided -> valid address but probably not fid=0 fid=x -> interpreted as uid=0 fid=x -> invalid address because uid=0 is invalid Plus, if you have two devices: uid=1, uid=2 -> interpreted as uid=1 fid=0, uid=2 fid=0 -> invalid addresses because of duplicated fid Dan, please go through the entire thread and look at the other examples that Shalini, Boris and I have provided: I think you'll see why I feel like it's hard to argue that the current behavior can be considered reasonable from the user's point of view. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390
On Thu, May 14, 2020 at 03:34:30PM +0200, Boris Fiuczynski wrote: > On 5/14/20 10:37 AM, Daniel P. Berrangé wrote: > > On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote: > > > On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote: > > > > On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote: > > > > > The behavior change would be > > > > > Current code: > > > > > uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated > > > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid > > > > > uid=0 -> uid=0 fid=0 -> address gets autogenerated > > > > > > > > IIUC, in the two cases here where the address gets auto-generated, > > > > the resulting guest VM successfully boots & runs > > > > > > > > > With the series applied > > > > > uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid > > > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid > > > > > uid=0 -> uid=0 fid=0 -> address is rejected as invalid > > > > > > > > ...so this proposed change is a functional regression for the > > > > user. > > > > > > > > > The documentation already specifies the uid value range correctly. > > > > > The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) > > > > > is > > > > > simple: Remove the zpci definition completely. > > > > > > > > This would be taking a users' currently working VM, intentionally > > > > breaking it, and then making the user pick up the pieces. This is > > > > an example of a behaviour regression that libvirt promises to not > > > > do to users. > > > > > > The bit of nuance that might be missing here is that existing guests > > > already have a full zPCI address stored in the domain XML, which > > > means the wouldn't be affected in any way; additionally, the case > > > where no zPCI address is provided when defining a new guest, which I > > > assume is the most common one, will keep working. > > > > > > The only scenarios that would no longer work are: > > > > > >* the user manually specifies uid=0 fid=0; > > >* the user manually specifies uid=0 and doesn't specify fid. > > > > > > In both cases the user would have gone out of their way to specify > > > a value for the uid attribute that is documented as being invalid: > > > > > >PCI addresses for S390 guests will have a zpci child element, with > > >two attributes: uid (a hex value between 0x0001 and 0x [...] > > > > > >https://libvirt.org/formatdomain.html#elementsAddress > > > > The effect of specifying zero though is that we perform allocation > > to assign a non-zero address, which is then valid. The same happens > > with regular PCI devices if you give slot="0". > > > > > As a result, they'd now get a pretty clear error message at define > > > time instead of confusing behavior across the board. I'm not really > > > sure anyone would complain about such a change. > > > > I don't see this existing behaviour as confusing. It looks like mostly > > being a docs ommission about auto-allocation taking place. > > Maybe I am repeating myself but I find e.g the below example confusing if I > take into consideration that uid=0 is NOT a valid value and fid is a valid > value. Please note that the valid fid is dislocated from its original > device! > > Specify this in the domain: >pcidev1: uid='0x' fid='0x' >pcidev2: uid='0x' > Results in a defined domain: >pcidev1: uid='0x0002' fid='0x0001' >pcidev2: uid='0x0001' fid='0x' > > If the user would be tying to fix the dislocating fid... one would very > likely try this: > Specify this in the domain: >pcidev1: uid='0x' fid='0x' >pcidev2: uid='0x' fid='0x0001' > Result: > error: Failed to define domain from mini-pcis.xml > error: XML error: Invalid PCI address uid='0x', must be > 0x and <= > 0x > > Btw setting uid=0 is defined by architecture for a mode that we do not > support in qemu AND setting fid=0 is an architectural valid assignment which > in the example above is not granted to the device it was defined for. Ok, that's the bit I didn't understand. I was assuming 0 was not a valid number, but it is valid for a feature we don't currently use. Thus we should be reporting usage as an error, such that we can implement correct semantics at a later date if desired. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 5/9] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY
On 5/13/20 3:30 PM, Daniel Henrique Barboza wrote:
Aside from trivial XML parsing/format changes, this patch adds
additional rules for TPM device support to better accomodate
all the available scenarios with the new TPM Proxy.
The changes make no impact to existing domains. This means that
the scenario of a domain with a single TPM device is still
supported in the same way. The restriction of multiple TPM devices
got alleviated to allow a TPM Proxy device to be added together
with a TPM device in the same domain. All other combinations
are still forbidden.
To summarize, after this patch, the following combinations in the same
domain are valid:
- a single TPM device
- a single TPM Proxy device
- a single TPM + single TPM Proxy devices
These combinations in the same domain are NOT allowed:
- 2 or more TPM devices
- 2 or more TPM Proxy devices
Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Stefan Berger
---
src/conf/domain_conf.c | 45 ++
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01a32f62d1..8164cd58c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13730,6 +13730,14 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
+/* TPM Proxy devices have 'passthrough' backend */
+if (def->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY &&
+def->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'Passthrough' backend is required for TPM Proxy
devices"));
+goto error;
+}
+
if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
goto error;
@@ -21972,15 +21980,39 @@ virDomainDefParseXML(xmlDocPtr xml,
if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0)
goto error;
-if (n > 1) {
+if (n > 2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("only a single TPM device is supported"));
+ _("a maximum of two TPM devices is supported, one of "
+ "them being a TPM Proxy device"));
goto error;
}
if (n > 0) {
-if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt,
flags)))
-goto error;
+for (i = 0; i < n; i++) {
+g_autoptr(virDomainTPMDef) dev = NULL;
+
+if (!(dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt,
flags)))
+goto error;
+
+/* TPM Proxy devices must be held in def->tpmproxy. Error
+ * out if there's a TPM Proxy declared already */
+if (dev->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
+if (def->tpmproxy) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM Proxy device is
supported"));
+goto error;
+}
+def->tpmproxy = g_steal_pointer(&dev);
+} else {
+/* all other TPM devices goes to def->tpm */
+if (def->tpm) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("only a single TPM non-proxy device is
supported"));
+goto error;
+}
+def->tpm = g_steal_pointer(&dev);
+}
+}
}
VIR_FREE(nodes);
@@ -29807,6 +29839,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
goto error;
}
+if (def->tpmproxy) {
+if (virDomainTPMDefFormat(buf, def->tpmproxy, flags) < 0)
+goto error;
+}
+
for (n = 0; n < def->ngraphics; n++) {
if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0)
goto error;
Re: [PATCH v3 4/9] conf, domain: register AUTOPTR_CLEANUP_FUNC for virDomainTPMDef
On 5/13/20 3:30 PM, Daniel Henrique Barboza wrote: Next patch will make use of g_autoptr() with virDomainTPMDefPtr. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8f178ade34..60dbba3b19 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3028,6 +3028,7 @@ virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); void virDomainDeviceSetData(virDomainDeviceDefPtr device, void *devicedata); void virDomainTPMDefFree(virDomainTPMDefPtr def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainTPMDef, virDomainTPMDefFree); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceDefPtr dev, It seems there are several other ones of those now already in this file. So it should be good. Reviewed-by: Stefan Berger
Re: [PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390
On 5/14/20 10:37 AM, Daniel P. Berrangé wrote: On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote: On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote: On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote: The behavior change would be Current code: uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid uid=0 -> uid=0 fid=0 -> address gets autogenerated IIUC, in the two cases here where the address gets auto-generated, the resulting guest VM successfully boots & runs With the series applied uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid uid=0 -> uid=0 fid=0 -> address is rejected as invalid ...so this proposed change is a functional regression for the user. The documentation already specifies the uid value range correctly. The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is simple: Remove the zpci definition completely. This would be taking a users' currently working VM, intentionally breaking it, and then making the user pick up the pieces. This is an example of a behaviour regression that libvirt promises to not do to users. The bit of nuance that might be missing here is that existing guests already have a full zPCI address stored in the domain XML, which means the wouldn't be affected in any way; additionally, the case where no zPCI address is provided when defining a new guest, which I assume is the most common one, will keep working. The only scenarios that would no longer work are: * the user manually specifies uid=0 fid=0; * the user manually specifies uid=0 and doesn't specify fid. In both cases the user would have gone out of their way to specify a value for the uid attribute that is documented as being invalid: PCI addresses for S390 guests will have a zpci child element, with two attributes: uid (a hex value between 0x0001 and 0x [...] https://libvirt.org/formatdomain.html#elementsAddress The effect of specifying zero though is that we perform allocation to assign a non-zero address, which is then valid. The same happens with regular PCI devices if you give slot="0". As a result, they'd now get a pretty clear error message at define time instead of confusing behavior across the board. I'm not really sure anyone would complain about such a change. I don't see this existing behaviour as confusing. It looks like mostly being a docs ommission about auto-allocation taking place. Maybe I am repeating myself but I find e.g the below example confusing if I take into consideration that uid=0 is NOT a valid value and fid is a valid value. Please note that the valid fid is dislocated from its original device! Specify this in the domain: pcidev1: uid='0x' fid='0x' pcidev2: uid='0x' Results in a defined domain: pcidev1: uid='0x0002' fid='0x0001' pcidev2: uid='0x0001' fid='0x' If the user would be tying to fix the dislocating fid... one would very likely try this: Specify this in the domain: pcidev1: uid='0x' fid='0x' pcidev2: uid='0x' fid='0x0001' Result: error: Failed to define domain from mini-pcis.xml error: XML error: Invalid PCI address uid='0x', must be > 0x and <= 0x Btw setting uid=0 is defined by architecture for a mode that we do not support in qemu AND setting fid=0 is an architectural valid assignment which in the example above is not granted to the device it was defined for. Regards, Daniel -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Re: [PATCH v3 1/9] docs: documentation and schema for the new TPM Proxy model
On 5/13/20 3:30 PM, Daniel Henrique Barboza wrote: QEMU 4.1.0 introduced a new device type called TPM Proxy, currently implemented by PPC64 guests via a new virtual device called 'spapr-tpm-proxy' (see QEMU 0fb6bd073230 for more info). The TPM Proxy device interacts with a TPM Resource Manager, a host device capable of multiplexing the host TPM with multiple processes. This allows multiple guests to access some TPM features at the same time. Note that this mode of operation does not provide full TPM features to be available for the guest - for that case the guest still needs to assign a vTPM device (tpm-spapr for PPC64 guests). Although redundant, there is currently no technical limitation for a guest to assign both a vTPM and a TPM Proxy at the same time. This patch adds documentation and schema for a new TPM model type called 'spapr-tpm-proxy' that creates this new TPM Proxy device. This model is valid only for the 'passthrough' backend. An example of a TPM Proxy device connected to a TPM Resource Manager '/dev/tpmrm0' will look like this: Signed-off-by: Daniel Henrique Barboza Reviewed-by: Stefan Berger --- docs/formatdomain.html.in | 18 +- docs/schemas/domaincommon.rng | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 23eb029234..15a92aa4f4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8792,6 +8792,17 @@ qemu-kvm -net nic,model=? /dev/null backend device is a TPM 2.0. Since 6.1.0, pSeries guests on PPC64 are supported and the default is tpm-spapr. + + Since 6.4.0, a new model called + spapr-tpm-proxy was added for pSeries guests. This model + only works with the 'passthrough' backend. It creates a TPM Proxy + device that communicates with an existing TPM Resource Manager in the host, + for example /dev/tpmrm0, enabling the guest to run in secure virtual machine + mode with the help of an Ultravisor. Adding a TPM Proxy to a pSeries guest + brings no security benefits unless the guest is running on a PPC64 host that + has an Ultravisor and a TPM Resource Manager. Only one TPM Proxy device is + allowed per guest, but a TPM Proxy device can be added together with + other TPM devices. backend @@ -8804,7 +8815,7 @@ qemu-kvm -net nic,model=? /dev/null passthrough - Use the host's TPM device. + Use the host's TPM or TPM Resource Manager device. This backend type requires exclusive access to a TPM device on @@ -8812,6 +8823,11 @@ qemu-kvm -net nic,model=? /dev/null qualified file name is specified by path attribute of the source element. If no file name is specified then /dev/tpm0 is automatically used. + + Since 6.4.0, when choosing the + spapr-tpm-proxy model, the file name specified is + expected to be a TPM Resource Manager device, e.g. + /dev/tpmrm0. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9d60b090f3..50860419c3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4610,6 +4610,7 @@ tpm-tis tpm-crb tpm-spapr +spapr-tpm-proxy
Re: [PATCH v3 3/9] conf, qemu: adding 'tpmproxy' in domain definition
On 5/13/20 3:30 PM, Daniel Henrique Barboza wrote:
A TPM Proxy device can coexist with a regular TPM. The TPM Proxy
is also always a 'passthrough' device of the 'spapr-tpm-proxy'
model.
This patch adds a pointer to this device in the domain definition
called 'tpmproxy'. This pointer is handled like the existing
'tpm' pointer of the VIR_DOMAIN_TPM_TYPE_PASSTHROUGH type.
Cgroup, DAC/SELinux and qemu validation code was adapted to handle
this new domain device.
XML functions to parse and format this new device from/to XML
will be added in the next patch, together with the logic that
will guarantee the assumptions made in the first paragraph.
Signed-off-by: Daniel Henrique Barboza
AppArmor shouldn't need anything.
Reviewed-by: Stefan Berger
---
src/conf/domain_audit.c | 3 +++
src/conf/domain_conf.c | 18 ++
src/conf/domain_conf.h | 2 ++
src/qemu/qemu_cgroup.c | 12 +---
src/qemu/qemu_domain.c | 9 +
src/qemu/qemu_validate.c| 12
src/security/security_dac.c | 14 ++
src/security/security_selinux.c | 11 +++
8 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 1b0abb21a0..4575f66e45 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -824,6 +824,9 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason,
bool success)
if (vm->def->tpm)
virDomainAuditTPM(vm, vm->def->tpm, "start", true);
+if (vm->def->tpmproxy)
+virDomainAuditTPM(vm, vm->def->tpmproxy, "start", true);
+
for (i = 0; i < vm->def->nshmems; i++)
virDomainAuditShmem(vm, vm->def->shmems[i], "start", true);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c201fc901d..01a32f62d1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1165,6 +1165,7 @@ VIR_ENUM_IMPL(virDomainTPMModel,
"tpm-tis",
"tpm-crb",
"tpm-spapr",
+ "spapr-tpm-proxy",
);
VIR_ENUM_IMPL(virDomainTPMBackend,
@@ -3480,6 +3481,7 @@ void virDomainDefFree(virDomainDefPtr def)
VIR_FREE(def->mems);
virDomainTPMDefFree(def->tpm);
+virDomainTPMDefFree(def->tpmproxy);
for (i = 0; i < def->npanics; i++)
virDomainPanicDefFree(def->panics[i]);
@@ -4318,6 +4320,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
if ((rc = cb(def, &device, &def->tpm->info, opaque)) != 0)
return rc;
}
+if (def->tpmproxy) {
+device.type = VIR_DOMAIN_DEVICE_TPM;
+device.data.tpm = def->tpmproxy;
+if ((rc = cb(def, &device, &def->tpmproxy->info, opaque)) != 0)
+return rc;
+}
device.type = VIR_DOMAIN_DEVICE_PANIC;
for (i = 0; i < def->npanics; i++) {
device.data.panic = def->panics[i];
@@ -24344,6 +24352,16 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src,
goto error;
}
+if (src->tpmproxy && dst->tpmproxy) {
+if (!virDomainTPMDefCheckABIStability(src->tpmproxy, dst->tpmproxy))
+goto error;
+} else if (src->tpmproxy || dst->tpmproxy) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Either both target and source domains or none of "
+ "them must have TPM Proxy device present"));
+goto error;
+}
+
if (src->nmems != dst->nmems) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Target domain memory device count %zu "
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ddc75d8de2..8f178ade34 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1293,6 +1293,7 @@ typedef enum {
VIR_DOMAIN_TPM_MODEL_TIS,
VIR_DOMAIN_TPM_MODEL_CRB,
VIR_DOMAIN_TPM_MODEL_SPAPR,
+VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY,
VIR_DOMAIN_TPM_MODEL_LAST
} virDomainTPMModel;
@@ -2628,6 +2629,7 @@ struct _virDomainDef {
virDomainMemballoonDefPtr memballoon;
virDomainNVRAMDefPtr nvram;
virDomainTPMDefPtr tpm;
+virDomainTPMDefPtr tpmproxy;
virCPUDefPtr cpu;
virSysinfoDefPtr sysinfo;
virDomainRedirFilterDefPtr redirfilter;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 2e019b64af..2ed4341655 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -333,10 +333,13 @@ qemuSetupChardevCgroupCB(virDomainDefPtr def
G_GNUC_UNUSED,
static int
-qemuSetupTPMCgroup(virDomainObjPtr vm)
+qemuSetupTPMCgroup(virDomainObjPtr vm,
+ virDomainTPMDefPtr dev)
{
int ret = 0;
-virDomainTPMDefPtr dev = vm->def->tpm;
+
+if (!dev)
+return 0;
switch (dev->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
@@ -806,7 +809,10 @@ qemuSetupDevicesCgroup(virDomainObjPtr vm)
vm) <
Re: [PATCH v3 9/9] docs/news.xml: update for the new TPM Proxy device
On 5/13/20 3:30 PM, Daniel Henrique Barboza wrote: Signed-off-by: Daniel Henrique Barboza Reviewed-by: Stefan Berger --- docs/news.xml | 17 + 1 file changed, 17 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 4cef804aac..c22a0f0a18 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,23 @@ + + + qemu: add TPM Proxy device support + + + libvirt can now create guests using a new device type called + "TPM Proxy". The TPM Proxy connects to a TPM Resource Manager + present in the host, enabling the guest to run in secure virtual + machine mode with the help of an Ultravisor. Adding a TPM Proxy to + a pSeries guest brings no security benefits unless the guest is + running on a PPC64 host that has Ultravisor and TPM Resource Manager + support. Only one TPM Proxy is allowed per guest. A guest using + a TPM Proxy device can instantiate another TPM device at the same + time. This device is supported only for pSeries guests via the new + 'spapr-tpm-proxy' model of the TPM 'passthrough' backend. + +
Re: [PATCH 0/2] qemu: Couple of memleak fixes
On Thu, May 14, 2020 at 11:37:40 +0200, Michal Privoznik wrote: > *** BLURB HERE *** > > Michal PrÃvoznÃk (2): > qemuDomainStorageSourcePrivateDispose: Free httpcookie > qemuBlockJobDataDisposeJobdata: Free data.commit.disabledBitmapsBase > > src/qemu/qemu_blockjob.c | 5 + > src/qemu/qemu_domain.c | 1 + > 2 files changed, 6 insertions(+) Reviewed-by: Peter Krempa
[PATCH 2/2] qemuBlockJobDataDisposeJobdata: Free data.commit.disabledBitmapsBase
==179663== 35 (24 direct, 11 indirect) bytes in 1 blocks are definitely lost in
loss record 205 of 461
==179663==at 0x4839EC6: calloc (vg_replace_malloc.c:762)
==179663==by 0x5791AC0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.1)
==179663==by 0x190C79: qemuDomainObjPrivateXMLParseBlockjobDataCommit
(qemu_domain.c:3295)
==179663==by 0x190DF7: qemuDomainObjPrivateXMLParseBlockjobDataSpecific
(qemu_domain.c:3331)
==179663==by 0x19157D: qemuDomainObjPrivateXMLParseBlockjobData
(qemu_domain.c:3469)
==179663==by 0x1918E8: qemuDomainObjPrivateXMLParseBlockjobs
(qemu_domain.c:3498)
==179663==by 0x193841: qemuDomainObjPrivateXMLParse (qemu_domain.c:3944)
==179663==by 0x4A1BA9D: virDomainObjParseXML (domain_conf.c:22306)
==179663==by 0x4A1BFE9: virDomainObjParseNode (domain_conf.c:22429)
==179663==by 0x4A1C0B4: virDomainObjParseFile (domain_conf.c:22443)
==179663==by 0x1431E1: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61)
==179663==by 0x177722: virTestRun (testutils.c:142)
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_blockjob.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index e51499532f..17dc08476b 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -84,6 +84,11 @@ qemuBlockJobDataDisposeJobdata(qemuBlockJobDataPtr job)
virObjectUnref(job->data.backup.store);
g_free(job->data.backup.bitmap);
}
+
+if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT ||
+job->type == QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT) {
+virStringListFree(job->data.commit.disabledBitmapsBase);
+}
}
--
2.26.2
[PATCH 0/2] qemu: Couple of memleak fixes
*** BLURB HERE *** Michal Prívozník (2): qemuDomainStorageSourcePrivateDispose: Free httpcookie qemuBlockJobDataDisposeJobdata: Free data.commit.disabledBitmapsBase src/qemu/qemu_blockjob.c | 5 + src/qemu/qemu_domain.c | 1 + 2 files changed, 6 insertions(+) -- 2.26.2
[PATCH 1/2] qemuDomainStorageSourcePrivateDispose: Free httpcookie
==156803== 58 (40 direct, 18 indirect) bytes in 1 blocks are definitely lost in loss record 306 of 463 ==156803==at 0x4839EC6: calloc (vg_replace_malloc.c:762) ==156803==by 0x5791AC0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.1) ==156803==by 0x48F60DC: virAlloc (viralloc.c:48) ==156803==by 0x18DD74: qemuStorageSourcePrivateDataAssignSecinfo (qemu_domain.c:2384) ==156803==by 0x18DFD5: qemuStorageSourcePrivateDataParse (qemu_domain.c:2433) ==156803==by 0x49EC884: virDomainStorageSourceParse (domain_conf.c:9857) ==156803==by 0x49ECBA3: virDomainDiskBackingStoreParse (domain_conf.c:9909) ==156803==by 0x49F129D: virDomainDiskDefParseXML (domain_conf.c:10785) ==156803==by 0x4A1804E: virDomainDefParseXML (domain_conf.c:21543) ==156803==by 0x4A1B60C: virDomainObjParseXML (domain_conf.c:22254) ==156803==by 0x4A1BFE9: virDomainObjParseNode (domain_conf.c:22429) ==156803==by 0x4A1C0B4: virDomainObjParseFile (domain_conf.c:22443 Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a1b250fd0b..d0528dbfe0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1229,6 +1229,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj) g_clear_pointer(&priv->secinfo, qemuDomainSecretInfoFree); g_clear_pointer(&priv->encinfo, qemuDomainSecretInfoFree); +g_clear_pointer(&priv->httpcookie, qemuDomainSecretInfoFree); } -- 2.26.2
Re: [PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390
On Wed, May 13, 2020 at 07:41:34PM +0200, Andrea Bolognani wrote: > On Wed, 2020-05-13 at 17:32 +0100, Daniel P. Berrangé wrote: > > On Tue, May 12, 2020 at 12:13:22PM +0200, Boris Fiuczynski wrote: > > > The behavior change would be > > > Current code: > > > uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid > > > uid=0 -> uid=0 fid=0 -> address gets autogenerated > > > > IIUC, in the two cases here where the address gets auto-generated, > > the resulting guest VM successfully boots & runs > > > > > With the series applied > > > uid=0 fid=0 -> uid=0 fid=0 -> address is rejected as invalid > > > uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid > > > uid=0 -> uid=0 fid=0 -> address is rejected as invalid > > > > ...so this proposed change is a functional regression for the > > user. > > > > > The documentation already specifies the uid value range correctly. > > > The fix for users hitting the two scenarios (uid=0 fid=0) and (uid=0) is > > > simple: Remove the zpci definition completely. > > > > This would be taking a users' currently working VM, intentionally > > breaking it, and then making the user pick up the pieces. This is > > an example of a behaviour regression that libvirt promises to not > > do to users. > > The bit of nuance that might be missing here is that existing guests > already have a full zPCI address stored in the domain XML, which > means the wouldn't be affected in any way; additionally, the case > where no zPCI address is provided when defining a new guest, which I > assume is the most common one, will keep working. > > The only scenarios that would no longer work are: > > * the user manually specifies uid=0 fid=0; > * the user manually specifies uid=0 and doesn't specify fid. > > In both cases the user would have gone out of their way to specify > a value for the uid attribute that is documented as being invalid: > > PCI addresses for S390 guests will have a zpci child element, with > two attributes: uid (a hex value between 0x0001 and 0x [...] > > https://libvirt.org/formatdomain.html#elementsAddress The effect of specifying zero though is that we perform allocation to assign a non-zero address, which is then valid. The same happens with regular PCI devices if you give slot="0". > As a result, they'd now get a pretty clear error message at define > time instead of confusing behavior across the board. I'm not really > sure anyone would complain about such a change. I don't see this existing behaviour as confusing. It looks like mostly being a docs ommission about auto-allocation taking place. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool
On Wed, May 13, 2020 at 07:00:49PM +0200, Andrea Bolognani wrote: > On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote: > > This series is trying to consolidate the number of config files we currently > > recognize under ~/.config/lcitool to a single global YAML config file. > > Thanks > > to this effort we can expose more seetings than we previously could which > > will > > come handy in terms of generating cloudinit images for OpenStack. > > > > Patches 1-4 (ACKed) > > > > Since RFC: > > - replaced TOML with YAML which simplified some aspects of the code, thanks > > Andrea > > - instead of hardcoding the default values, the config within the repo is > > used > > as a template and overriden with user-selected options > > > > Since v1: > > - uncommented the mandatory options in the default template YAML config so > > that > > we know about all the supported keys which is useful for validating the > > user > > config > > - removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in > > v1) > > - added checks for value types we get from the config > > - use yaml.safe_load instead of yaml.load > > - added code snippet to delete keys we don't recognize so as not to > > introduce a > > security issue, because we essentially just take the config and pass it to > > ansible, we don't want users to use to re-define some of Ansible's > > variables > > - added the last patch just to demonstrate a number of test cases I used as > > a > > proof for correctness of this revision (feel free to add more cases), but > > this is not the right series to add pytest support into lcitool > > You should have all the ACKs you need now. > > Thanks for being patient during review, and for taking care of this > in the first place :) Thanks for the review, changes are now merged. -- Erik Skultety
Re: [libvirt-ci PATCH v2 06/13] lcitool: Introduce methods to load and validate the YAML config
On Wed, May 13, 2020 at 06:48:26PM +0200, Andrea Bolognani wrote:
> On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
> > +def _validate_section(self, config, section, mandatory_keys):
> > +# remove keys we don't recognize
> > +self._remove_unknown_keys(config[section],
> > self.values[section].keys())
> > +
> > +# check that the mandatory keys are present and non-empty
> > +for key in mandatory_keys:
> > +if config.get(section).get(key, None) is None:
>
> Isn't
>
> foo.get("bar")
>
> defined as returning None if the "bar" key is not defined for the
> dictionary foo? Basically, I think you could write this as
>
> if config.get(section).get(key) is None:
Correct, I was just being explicit, it might not be clear at first glance that
None is the default return value, but I agree that what you suggest is the
Pythonic way, so I'll happily change it.
--
Erik Skultety
Re: [PATCH 3/6] qemu: check if AMD secure guest support is enabled
On Wed, May 13, 2020 at 11:26:52PM -0500, Brijesh Singh wrote:
>
> On 5/13/20 1:21 AM, Erik Skultety wrote:
> > 2) check if /dev/sev device exist (aka firmware is detected)
> This seems reasonable. Shouldn't it have been documented in
> docs/kbase/launch_security_sev.rst?
> >>> Sure, we can add a mention about this. Although, doesn't 1 imply 2? IOW
> >>> can
> >>> you have the kernel module parameter set to 1 and yet kernel doesn't
> >>> expose the
> >>> /dev/sev node?
> >>
> >> Currently, 1 does not imply 2, KVM driver does not initialize the
> >> firmware during the feature probe (i.e does not access the /dev/sev).
> >> The firmware initialization is delayed until the first guest launch. So
> >> only sane way to know whether firmware is been detected is check the
> >> existence of the /dev/sev or issue a query-sev command . The query-sev
> >> command will send the platform_status request to the firmware, if the
> >> firmware is not ready then this command will fail.
> > I see. Can query-sev fail or return that it's disabled, aka {"enabled":
> > false,...} in the SevInfo QMP response, but at the same time succeed in
> > returning the platform capabilities via query-sev-capabilities? I'm asking,
> > because libvirt only issues the latter to fill in the QEMU capabilities
> > structure.
>
> Just looked at qemu code, If /dev/sev does not exist then
> query-sev-capabilities should fail, and if SEV is not enabled in the
> guest then query-sev should returns false. So, basically what libvirt is
> doing correct, it should be using query-sev-capabilities to populate
> QEMU capabilities. It was my bad, I should have mentioned the
> query-sev-capabilities and not the query-sev check.
Perfect, now it makes much more sense to me after quite a period not closely
following this feature, thanks Brijesh.
Boris, I'll start with the review today, but for the IBM part, I'm not sure I
can get my hands on the necessary s390 HW (I'm also not familiar with s390), so
I can do code-only review here, but at least for the AMD bits I do have access
to the necessary HW and apart from scanning the filesystem (or parsing the
kernel cmdline) the outcome is the same, so I'll do some mirroring in the
review process.
Regards,
--
Erik Skultety
