Re: Status of iSER support.

[David Schweizer]

Hello Peter,

On 5/12/20 8:33 AM, Peter Krempa wrote:
> Well, you certainly can contribute in having them merged. Even testing
> is a contribution.
> [..]
> Well, for us that means that we don't have anybody willing to give input
> on the feature. I don't have experience nor hardware for testing it [..].

I also have only one capable host left, but no counterpart to test with.
Other hardware is either running in production environment or reserved
for emergencies.

> Depends on the status of the patches. I've reviewed them yesterday and I
> don't agree with the design. If you have more knowledge of how those
> things work, even explaining them is a contribution to possbily adding
> the feature.

I mean it is kind of tricky to implement, since iSER sits somewhat
between the iSCSI protocol and different transport methods. Maybe
implementing it as its own protocol capable of communicating over
different transport methods (RoCE, Infiniband, etc. (NOT: RDMA)) is the
way to go.

I think the author simply tried to adapt it to the implementation in
qemu as best as he could to avoid self inflicting head wounds with his
keyboard during alteration of the libvirt qemu driver.

Kind regards,

David



signature.asc
Description: OpenPGP digital signature

[libvirt PATCH v2 4/4] tests: qemuxml2argvtest: Add vmpvscsi to disk-scsi test

[Chris Jester-Young]

Signed-off-by: Chris Jester-Young 
---
 .../disk-scsi.x86_64-latest.args  | 37 +++
 tests/qemuxml2argvdata/disk-scsi.xml  |  6 +++
 .../disk-scsi.x86_64-latest.xml   | 11 +-
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args
index 06c71cbab6..489f53d80e 100644
--- a/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args
@@ -32,37 +32,44 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -device megasas,id=scsi1,bus=pci.0,addr=0x3 \
 -device mptsas1068,id=scsi2,bus=pci.0,addr=0x4 \
 -device spapr-vscsi,id=scsi3,reg=0x2000 \
+-device pvscsi,id=scsi4,bus=pci.0,addr=0x5 \
 -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1",\
+"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 ide-hd,bus=ide.0,unit=0,drive=libvirt-6-format,id=ide0-0-0,bootindex=1 
\
+-blockdev '{"driver":"file","filename":"/tmp/scsidisk.img",\
 "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 ide-hd,bus=ide.0,unit=0,drive=libvirt-5-format,id=ide0-0-0,bootindex=1 
\
--blockdev '{"driver":"file","filename":"/tmp/scsidisk.img",\
+-device scsi-hd,bus=scsi0.0,scsi-id=0,device_id=drive-scsi0-0-0,\
+drive=libvirt-5-format,id=scsi0-0-0 \
+-blockdev '{"driver":"file","filename":"/tmp/scsidisk2.img",\
 "node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}' \
 -blockdev '{"node-name":"libvirt-4-format","read-only":false,"driver":"raw",\
 "file":"libvirt-4-storage"}' \
--device scsi-hd,bus=scsi0.0,scsi-id=0,device_id=drive-scsi0-0-0,\
-drive=libvirt-4-format,id=scsi0-0-0 \
--blockdev '{"driver":"file","filename":"/tmp/scsidisk2.img",\
+-device 
scsi-hd,bus=scsi1.0,channel=0,scsi-id=0,lun=0,device_id=abcdefghijklmn,\
+drive=libvirt-4-format,id=scsi1-0-0-0,serial=abcdefghijklmn \
+-blockdev '{"driver":"file","filename":"/tmp/scsidisk3.img",\
 "node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}' \
 -blockdev '{"node-name":"libvirt-3-format","read-only":false,"driver":"raw",\
 "file":"libvirt-3-storage"}' \
--device 
scsi-hd,bus=scsi1.0,channel=0,scsi-id=0,lun=0,device_id=abcdefghijklmn,\
-drive=libvirt-3-format,id=scsi1-0-0-0,serial=abcdefghijklmn \
--blockdev '{"driver":"file","filename":"/tmp/scsidisk3.img",\
-"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
--blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
-"file":"libvirt-2-storage"}' \
 -device scsi-hd,bus=scsi2.0,channel=0,scsi-id=0,lun=0,\
-device_id=drive-scsi2-0-0-0,drive=libvirt-2-format,id=scsi2-0-0-0,\
+device_id=drive-scsi2-0-0-0,drive=libvirt-3-format,id=scsi2-0-0-0,\
 wwn=0x5000c50015ea71ac \
 -blockdev '{"driver":"file","filename":"/tmp/scsidisk4.img",\
+"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
+"file":"libvirt-2-storage"}' \
+-device scsi-hd,bus=scsi3.0,channel=0,scsi-id=0,lun=0,\
+device_id=drive-scsi3-0-0-0,drive=libvirt-2-format,id=scsi3-0-0-0 \
+-blockdev '{"driver":"file","filename":"/tmp/scsidisk5.img",\
 "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
 -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\
 "file":"libvirt-1-storage"}' \
--device scsi-hd,bus=scsi3.0,channel=0,scsi-id=0,lun=0,\
-device_id=drive-scsi3-0-0-0,drive=libvirt-1-format,id=scsi3-0-0-0 \
--device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 \
+-device scsi-hd,bus=scsi4.0,channel=0,scsi-id=0,lun=0,\
+device_id=drive-scsi4-0-0-0,drive=libvirt-1-format,id=scsi4-0-0-0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-scsi.xml 
b/tests/qemuxml2argvdata/disk-scsi.xml
index 7fbb785a37..4468a182db 100644
--- a/tests/qemuxml2argvdata/disk-scsi.xml
+++ b/tests/qemuxml2argvdata/disk-scsi.xml
@@ -41,12 +41,18 @@
   
   
 
+
+  
+  
+  
+
 
 
 
 
 
 
+
 
 
 
diff --git a/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml 
b/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
index c5fd3c0657..ff93277835 100644
--- a/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
@@ -49,6 +49,12 @@
   
   
 
+
+  
+  
+  
+  
+
 
   
 
@@ -67,11 +73,14 @@
 
   
 
+
+  
+
 

[libvirt PATCH v2 2/4] qemu: pvscsi: Add support for vmpvscsi controller model

[Chris Jester-Young]

Availability of the vmpvscsi controller model is gated by the pvscsi
capability.

Signed-off-by: Chris Jester-Young 
---
 src/qemu/qemu_command.c  | 4 +++-
 src/qemu/qemu_validate.c | 9 -
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2d8a6fb0dd..bfe70ed228 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2573,9 +2573,11 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
 virBufferAddLit(, "megasas");
 break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+virBufferAddLit(, "pvscsi");
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported controller model: %s"),

virDomainControllerModelSCSITypeToString(def->model));
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index d34151050f..fde1892d42 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2362,9 +2362,16 @@ qemuValidateCheckSCSIControllerModel(virQEMUCapsPtr 
qemuCaps,
 return false;
 }
 break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_PVSCSI)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("This QEMU doesn't support "
+ "the pvscsi (VMware paravirtual SCSI) 
controller"));
+return false;
+}
+break;
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
 case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported controller model: %s"),
virDomainControllerModelSCSITypeToString(model));
-- 
2.25.1

[libvirt PATCH v2 1/4] qemu: pvscsi: Add capability

[Chris Jester-Young]

This capability flags support for `-device pvscsi`, which provides the
VMware paravirtual SCSI controller.

Signed-off-by: Chris Jester-Young 
---
 src/qemu/qemu_capabilities.c   | 2 ++
 src/qemu/qemu_capabilities.h   | 1 +
 tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.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.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml| 1 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64.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.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.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.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 +
 40 files changed, 41 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 0e7db2643a..7e711f22f8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -581,6 +581,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "machine.pseries.cap-ibs",
   "tcg",
   "virtio-blk-pci.scsi.default.disabled",
+  "pvscsi",
 );
 
 
@@ -1302,6 +1303,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "tpm-spapr", QEMU_CAPS_DEVICE_TPM_SPAPR },
 { "vhost-user-fs-device", QEMU_CAPS_DEVICE_VHOST_USER_FS },
 { "tcg-accel", QEMU_CAPS_TCG },
+{ "pvscsi", QEMU_CAPS_SCSI_PVSCSI },
 };
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index db8bebe3df..6bfc7386e3 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -562,6 +562,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_MACHINE_PSERIES_CAP_IBS, /* -machine pseries.cap-ibs */
 QEMU_CAPS_TCG, /* QEMU does support TCG */
 QEMU_CAPS_VIRTIO_BLK_SCSI_DEFAULT_DISABLED, /* virtio-blk-pci.scsi 
disabled by default */
+QEMU_CAPS_SCSI_PVSCSI, /* -device pvscsi */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
index 2ba184cdda..a4f6c3aa09 100644
--- a/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml
@@ -86,6 +86,7 @@
   
   
   
+  
   1005003
   0
   43100245
diff --git a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
index fb160dfd4c..9e5501bed9 100644
--- a/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml
@@ -91,6 +91,7 @@
   
   
   
+  
   1006000
   0
   43100243
diff --git a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
index 732af17233..28eb61b38c 100644
--- a/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml
@@ -93,6 +93,7 @@
   
   
   
+  
   1007000
   0
   43100244
diff --git a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml
index 22b4817e4e..cfe79fef46 100644
--- a/tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml
+++ 

[libvirt PATCH v2 0/4] qemu: Add support for VMware paravirtual SCSI controller

[Chris Jester-Young]

QEMU has supported pvscsi, the VMware paravirtual SCSI controller, since
2013, but libvirt does not currently expose support for it.  Adding such
support is straightforward.

I have tested it with a Windows 10 guest, and the official VMware driver
supports it with no issues. However, booting only works with SeaBIOS
(tested) and the pre-release version of OVMF (not tested yet).

Chris Jester-Young (4):
  qemu: pvscsi: Add capability
  qemu: pvscsi: Add support for vmpvscsi controller model
  tests: qemuxml2xmltest: Convert disk-scsi to DO_TEST_CAPS_LATEST
  tests: qemuxml2argvtest: Add vmpvscsi to disk-scsi test

 src/qemu/qemu_capabilities.c  |  2 +
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_command.c   |  4 +-
 src/qemu/qemu_validate.c  |  9 -
 .../caps_1.5.3.x86_64.xml |  1 +
 .../caps_1.6.0.x86_64.xml |  1 +
 .../caps_1.7.0.x86_64.xml |  1 +
 .../caps_2.1.1.x86_64.xml |  1 +
 .../caps_2.10.0.aarch64.xml   |  1 +
 .../caps_2.10.0.ppc64.xml |  1 +
 .../caps_2.10.0.x86_64.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.x86_64.xml|  1 +
 .../caps_2.4.0.x86_64.xml |  1 +
 .../caps_2.5.0.x86_64.xml |  1 +
 .../caps_2.6.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  1 +
 .../caps_2.6.0.x86_64.xml |  1 +
 .../caps_2.7.0.x86_64.xml |  1 +
 .../caps_2.8.0.x86_64.xml |  1 +
 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  1 +
 .../caps_2.9.0.x86_64.xml |  1 +
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.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 +
 .../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 +
 .../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 +
 .../disk-scsi.x86_64-latest.args  | 37 +++
 tests/qemuxml2argvdata/disk-scsi.xml  |  6 +++
 ...k-scsi.xml => disk-scsi.x86_64-latest.xml} | 20 --
 tests/qemuxml2xmltest.c   |  3 +-
 46 files changed, 97 insertions(+), 23 deletions(-)
 rename tests/qemuxml2xmloutdata/{disk-scsi.xml => disk-scsi.x86_64-latest.xml} 
(83%)

-- 
2.25.1

[libvirt PATCH v2 3/4] tests: qemuxml2xmltest: Convert disk-scsi to DO_TEST_CAPS_LATEST

[Chris Jester-Young]

Signed-off-by: Chris Jester-Young 
---
 .../{disk-scsi.xml => disk-scsi.x86_64-latest.xml}  | 13 -
 tests/qemuxml2xmltest.c |  3 +--
 2 files changed, 9 insertions(+), 7 deletions(-)
 rename tests/qemuxml2xmloutdata/{disk-scsi.xml => disk-scsi.x86_64-latest.xml} 
(92%)

diff --git a/tests/qemuxml2xmloutdata/disk-scsi.xml 
b/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
similarity index 92%
rename from tests/qemuxml2xmloutdata/disk-scsi.xml
rename to tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
index 062a907303..c5fd3c0657 100644
--- a/tests/qemuxml2xmloutdata/disk-scsi.xml
+++ b/tests/qemuxml2xmloutdata/disk-scsi.x86_64-latest.xml
@@ -8,6 +8,9 @@
 hvm
 
   
+  
+qemu64
+  
   
   destroy
   restart
@@ -46,20 +49,20 @@
   
   
 
-
+
   
 
 
   
 
 
-  
+  
 
 
-  
+  
 
 
-  
+  
 
 
   
@@ -68,7 +71,7 @@
 
 
 
-  
+  
 
   
 
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index e7480fcf9d..033f81013e 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -339,8 +339,7 @@ mymain(void)
 DO_TEST("disk-network-vxhs", NONE);
 DO_TEST("disk-network-tlsx509", NONE);
 DO_TEST("disk-nvme", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_QCOW2_LUKS);
-DO_TEST("disk-scsi", QEMU_CAPS_SCSI_LSI, QEMU_CAPS_SCSI_MEGASAS,
-QEMU_CAPS_SCSI_MPTSAS1068, QEMU_CAPS_SCSI_DISK_WWN);
+DO_TEST_CAPS_LATEST("disk-scsi");
 DO_TEST("disk-virtio-scsi-reservations",
 QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_PR_MANAGER_HELPER);
 DO_TEST("controller-virtio-scsi", QEMU_CAPS_VIRTIO_SCSI);
-- 
2.25.1

Re: [libvirt PATCH] qemu: Support vmpvscsi controller model

[Chris Jester-Young]

On Tue, May 12, 2020 at 04:51:20PM +0200, Peter Krempa wrote:
> On Tue, May 12, 2020 at 10:26:46 -0400, Chris Jester-Young wrote:
> > As Daniel P. Berrangé mentioned, this can be configured out via Kconfig.
> > Plus there are actually some files in tests/qemucapabilitiesdata that do
> > not advertise that device (e.g., *.s390x.replies).
> 
> Yes, thus we need the capability. I'd slightly prefer if the capability
> is added in a separate commit but that's not strictly required.

Yep, it'll be the first commit in the series that I'll post shortly.
Currently just finishing up `make syntax-check`.

> We prefer if new tests use DO_TEST_CAPS_LATEST, or the version-locked
> variants of the test macro so that we are testing a "real" situation. It
> also simplifies addition of the test.

Thank you, that's a very helpful tip! I ended up editing disk-scsi.xml,
which is already DO_TEST_CAPS_LATEST for qemuxml2argvtest but not for
qemuxml2xmltest, so you'll see a commit for the latter too. I found
commit 4150f944f9f3f68077aa91e91af259755d4dc568 very helpful for seeing
how one goes about such a conversion.

> Note that you can use VIR_TEST_REGENERATE_OUTPUT=1 env variable when
> running the qemuxml2argvtest to force creation of the output file and
> then just verify that it's as expected.

Thanks, that's very helpful too, knowing I don't have to manually add
`` by hand!

Too bad, in the case of the qemuxml2argvtest data changes, the block
device node names are numbered backwards compared to the controller
bus IDs, which made the diff larger than it had to be. (I manually
renamed the node names before and after the change to verify that the
actual diff, minus the node name differences, makes sense.)

> Well, CI is already there, but the main libvirt project still works on
> the mail based workflow

Okay, then I'll email out the next patch series right after I send this
message, and not wait around.

Thanks so much for your help!
Chris.

Re: [PATCH V3 0/5] Introduce getHost support for ARM CPU driver

[Zhenyu Zheng]

Hi Jirka,

Thanks alot for the review and suggestions, I will try to fix them. As for
v4, I saw there was a "email patch" button in gitlab, so I used that, so
seems it is not correct :)

BR

On Tue, May 12, 2020 at 11:38 PM Jiri Denemark  wrote:

> On Wed, Apr 22, 2020 at 15:11:14 +0800, ZhengZhenyu wrote:
> > Introduce getHost support for ARM CPU driver. First add
> > some data about commonly used ARM CPU models, and their
> > vendors into cpu_map, then added some helper methods as
> > callbacks to load them. Read and parse vendor_id, part_id
> > and CPU flags of local CPU from corresponding registers.
> >
> > Signed-off-by: Zhenyu Zheng 
>
> The emails come from ZhengZhenyu , but the
> signed-off-by line says something else. Could you make them consistent?
>
> BTW, I'm intentionally replying to v3 as v4 was only supposed to have a
> one line diff to v3, but it was sent with both html and plain text parts
> and git am was unable to apply any of the v4 patches.
>
> Jirka
>

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[David Gibson]

On Wed, May 13, 2020 at 10:06:23AM +1000, David Gibson wrote:
> On Tue, May 12, 2020 at 01:29:43PM -0300, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 5/11/20 8:50 AM, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 5/11/20 8:28 AM, Daniel P. Berrangé wrote:
> > > > On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:
> > > > > 
> > > > > 
> > > > > On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:
> > > > > > On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:
> > > > > [...]
> > [...]
> > > > 
> > > > Currently libvirt only allows a single , but we can trivially
> > > > lift that restriction to allow multiple if desired too.
> > > 
> > > I don't believe it'll be necessary. Since it's only this TPM Proxy device 
> > > that
> > > can coexist with other TPMs, my idea is to do what I did here in this 
> > > series,
> > > but instead of creating a new device type I'll re-use the existing TPM 
> > > device
> > > in a 'tpmproxy' pointer in the domain for this case.
> > > 
> > > I'll still thinking about whether a new backend type is warranted or not. 
> > > For
> > > this PPC64 case alone it'll be simpler to just add a new 'model' called
> > > 'spapr-tpm'-proxy' for the existing TPM passthrough type. Creating a new
> > > backend type makes it easier to add other TPM Proxy devices when other 
> > > archs
> > > implement it though.
> > 
> > Update: I tried it out the new "backend" approach and didn't enjoy the 
> > results.
> > It ended up replicating a large amount of existing cgroup/dac/selinux code 
> > that
> > handles the existing "passthrough" backend and, all said and done, it 
> > didn't alleviate
> > that much the parsing/format XML logic comparing to the alternative.
> > 
> > I chose then to go to the simpler route - adding a new 'passthrough' model 
> > called
> > 'spapr-tpm-proxy'. This will not scale well if/when more TPM proxies 
> > devices are
> > added in the future, but we can cross that bridge when we come to
> > it.
> 
> TBH, I think that's unlikely to happen.  The TPM proxy exists because
> of the design of POWER's secure VM model, and because it was easy to
> add an hcall to PAPR for this, without thinking how it would integrate
> with other TPM devices or PAPR's existing / concurrently designed vTPM
> interface.  I don't think there's any general reason you'd want a
> device like this, distinct from just a vTPM.

Indeed, arguably, this would be better modelled by just some sort of
machine or firmware feature flag, rather than a "device" as such.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[David Gibson]

On Tue, May 12, 2020 at 01:29:43PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/11/20 8:50 AM, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 5/11/20 8:28 AM, Daniel P. Berrangé wrote:
> > > On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:
> > > > 
> > > > 
> > > > On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:
> > > > > On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:
> > > > [...]
> [...]
> > > 
> > > Currently libvirt only allows a single , but we can trivially
> > > lift that restriction to allow multiple if desired too.
> > 
> > I don't believe it'll be necessary. Since it's only this TPM Proxy device 
> > that
> > can coexist with other TPMs, my idea is to do what I did here in this 
> > series,
> > but instead of creating a new device type I'll re-use the existing TPM 
> > device
> > in a 'tpmproxy' pointer in the domain for this case.
> > 
> > I'll still thinking about whether a new backend type is warranted or not. 
> > For
> > this PPC64 case alone it'll be simpler to just add a new 'model' called
> > 'spapr-tpm'-proxy' for the existing TPM passthrough type. Creating a new
> > backend type makes it easier to add other TPM Proxy devices when other archs
> > implement it though.
> 
> Update: I tried it out the new "backend" approach and didn't enjoy the 
> results.
> It ended up replicating a large amount of existing cgroup/dac/selinux code 
> that
> handles the existing "passthrough" backend and, all said and done, it didn't 
> alleviate
> that much the parsing/format XML logic comparing to the alternative.
> 
> I chose then to go to the simpler route - adding a new 'passthrough' model 
> called
> 'spapr-tpm-proxy'. This will not scale well if/when more TPM proxies devices 
> are
> added in the future, but we can cross that bridge when we come to
> it.

TBH, I think that's unlikely to happen.  The TPM proxy exists because
of the design of POWER's secure VM model, and because it was easy to
add an hcall to PAPR for this, without thinking how it would integrate
with other TPM devices or PAPR's existing / concurrently designed vTPM
interface.  I don't think there's any general reason you'd want a
device like this, distinct from just a vTPM.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 3/6] qemu: check if AMD secure guest support is enabled

[Brijesh Singh]

On 5/12/20 10:32 AM, Erik Skultety wrote:
> On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
>> On 5/11/20 9:40 PM, Brijesh Singh wrote:
>>> Thanks for the patch Paulo, Few comments.
>>>
>>> On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
 From: Paulo de Rezende Pinatti 

 Implement secure guest check for AMD SEV (Secure Encrypted
 Virtualization) in order to invalidate the qemu capabilities
 cache in case the availability of the feature changed.

 For AMD SEV the verification consists of:
 - checking if /sys/module/kvm_amd/parameters/sev contains the
 value '1': meaning SEV is enabled in the host kernel;
 - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
 SME memory encryption feature on the host is enabled
>>>
>>> In addition to the kernel module parameter, we probably also need to
>>> check whether QEMU supports the feature. e.g, what if user has newer
>>> kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12.  To check the
>>> SEV feature support, we should verify the following conditions:
>>>
>>> 1) check kernel module parameter is set
>> Paulo implemented the checks following the documentation in
>> docs/kbase/launch_security_sev.rst. The check for the module parameter sev
>> is included. Is the check for the kernel cmdline parameter "mem_encrypt" not
>> necessary? Or would that be covered by your suggested check in 2)?
> Brijesh correct me here please, but IIRC having mem_encrypt set is merely
> a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW
> SEV should work without SME. If my memory serves well here, we don't need
> and also should not parse the kernel cmdline in this case.
>

Yes, that is correct.  mem_encrypt=on is not required for the SEV to
work. The mem_encrypt=on option is meant to enable the SME feature on
the host machine. In addition to the guest, if customer wants to protect
the Hypervsior memory from physical attacks then they can use SME or
TSME.  The SME can be enabled using mem_encrypt=on, whereas the TSME can
be enabled in the BIOS and does not require any kernel changes. AMD is
mostly recommending TSME to protect against the physical attacks.


>>> 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.


>>> 3) Check if Qemu supports SEV feature (maybe we can detect the existence
>>> of the query-sev QMP command or detect Qemu version >= 2.12)
> ^This is already achieved by qemuMonitorJSONGetSEVCapabilities
>
>> The idea is to check the host capabilities to detect if qemus capabilities
>> need to be reprobed. Therefore this would be a result if checks 1+2 change
>> but it would not be a cache invalidation trigger.
> I agree in the sense that the SEV support that is currently being reported in
> QEMU capabilities wasn't a good choice because it reports only platform data
> which are constant to the host and have nothing to do with QEMU. It would be
> okay to just say  in qemu capabilities and report the
> rest in host capabilities. I haven't added this info into host capabilities
> when I realized the mistake because I didn't want to duplicate the platform
> info on 2 places.  For the IBM secure guest feature, it makes total sense to
> report support within host capabilities, I'm just not sure whether we should 
> do
> the same for SEV and try really hard to "fix" the mess.
>
> Regards,

Re: [libvirt PATCH] xenconfig: Add feature gfx_passthru

[Artur Puzio]

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?
>
> Regards,
> Jim

Hi, sorry for slowish response.

Setting gfx_passthru to 1 as in this patch enables the autodetection
routines (it's equivalent to gfx_passthru=1 from xl.cfg). Currently
libxl handles only IGD. If there is no IGD specified at creation and
gfx_passthru is 1, autodetection routines will report an error
preventing domain creation. Relevant libxl code:

libxl_dm.c:1746

    if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
    enum libxl_gfx_passthru_kind gfx_passthru_kind =
    libxl__detect_gfx_passthru_kind(gc,
guest_config);
    switch (gfx_passthru_kind) {
    case LIBXL_GFX_PASSTHRU_KIND_IGD:
    machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
    break;
    case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
    LOGD(ERROR, guest_domid, "unable to detect required
gfx_passthru_kind");
    return ERROR_FAIL;
    default:
    LOGD(ERROR, guest_domid, "invalid value for
gfx_passthru_kind");
    return ERROR_INVAL;
    }
    }

libxl contains a function for checking if IGD is present. After some
testing I will send another version of this patch that will enable
gfx_passthru if there is a IGD attached.

Artur

>
>>
>> Signed-off-by: Artur Puzio 
>> ---
>>   docs/formatdomain.html.in |  7 +++
>>   docs/schemas/domaincommon.rng |  5 +
>>   src/conf/domain_conf.c    |  4 
>>   src/conf/domain_conf.h    |  1 +
>>   src/libxl/libxl_conf.c    | 13 +
>>   5 files changed, 30 insertions(+)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 91d6f6c0d3..5307844a23 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2064,6 +2064,7 @@
>>     xen
>>   e820_host state='on'/
>>   passthrough state='on' mode='share_pt'/
>> +    gfx_passthru state='on'/
>>     /xen
>>     pvspinlock state='on'/
>>     gic version='2'/
>> @@ -2270,6 +2271,12 @@
>>     on, off; mode - optional string sync_pt or share_pt
>>     6.3.0
>>   
>> +    
>> +  gfx_passthru
>> +  Enable Intel GPU specific quirks. Required and allowed
>> only when passing an IGD.
>> +  on, off
>> +  6.3.0
>> +    
>>     
>>     
>>     pmu
>> diff --git a/docs/schemas/domaincommon.rng
>> b/docs/schemas/domaincommon.rng
>> index 9d60b090f3..7d8ea879a1 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -6409,6 +6409,11 @@
>>   
>>     
>>   
>> +    
>> +  
>> +    
>> +  
>> +    
>>     
>>   
>>     
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 8a87586936..75f72ff64c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -214,6 +214,7 @@ VIR_ENUM_IMPL(virDomainXen,
>>     VIR_DOMAIN_XEN_LAST,
>>     "e820_host",
>>     "passthrough",
>> +  "gfx_passthru"
>>   );
>>     VIR_ENUM_IMPL(virDomainXenPassthroughMode,
>> @@ -19649,6 +19650,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
>>     switch ((virDomainXen) feature) {
>>   case VIR_DOMAIN_XEN_E820_HOST:
>> +    case VIR_DOMAIN_XEN_GFX_PASSTHRU:
>>   break;
>>     case VIR_DOMAIN_XEN_PASSTHROUGH:
>> @@ -23579,6 +23581,7 @@
>> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>>   }
>>   switch ((virDomainXen) i) {
>>   case VIR_DOMAIN_XEN_E820_HOST:
>> +    case VIR_DOMAIN_XEN_GFX_PASSTHRU:
>>   break;
>>     case VIR_DOMAIN_XEN_PASSTHROUGH:
>> @@ -29235,6 +29238,7 @@ virDomainDefFormatFeatures(virBufferPtr buf,
>>     switch ((virDomainXen) j) {
>>   case VIR_DOMAIN_XEN_E820_HOST:
>> +    

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel P . Berrangé]

On Tue, May 12, 2020 at 01:51:33PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/12/20 1:44 PM, Daniel P. Berrangé wrote:
> > On Tue, May 12, 2020 at 01:21:40PM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 5/12/20 12:53 PM, Daniel P. Berrangé wrote:
> > > > On Tue, May 12, 2020 at 11:21:52AM -0400, Stefan Berger wrote:
> > > > > On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:
> > > > > > On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza 
> > > > > > wrote:
> > > > > > > 
> > > > > > > On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:
> > > > > > > > On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:
> > > > > > > [...]
> > > > > > > > > It's a different guest side interface, the H_TPM_COMM 
> > > > > > > > > hypercall
> > > > > > > > > instead of the other PAPR TPM interface.  To which "why?" is 
> > > > > > > > > a very
> > > > > > > > > good question, but it's there now, so there's not much we can 
> > > > > > > > > do about
> > > > > > > > > it.
> > > > > > > > That's ok. Even though its a different guest interface, it is 
> > > > > > > > still
> > > > > > > > conceptually a TPM device at a high level, so we should be 
> > > > > > > > reusing
> > > > > > > > the existing  device type. At most we should add a new 
> > > > > > > > backend
> > > > > > > > type
> > > > > > > I think adding a new backend type is sensible. Re-using the 
> > > > > > > passthrough type
> > > > > > > and making the differentiation with 'model', for a device that 
> > > > > > > doesn't
> > > > > > > operate exactly as a regular vTPM but can coexist with other vTPM 
> > > > > > > devices,
> > > > > > > will make for a lot of IFs in the code.
> > > > > > Currently libvirt only allows a single , but we can trivially
> > > > > > lift that restriction to allow multiple if desired too.
> > > > > 
> > > > > 
> > > > > QEMU won't accept multiple TIS or CRB devices, though.
> > > > 
> > > > The commit message says you can do 2 at a time:
> > > > 
> > > > "Although redundant, there is currently no technical
> > > >  limitation for a guest to assign both a vTPM and a TPM Proxy at the
> > > >  same time."
> > > > 
> > > > is that text not accurate ?
> > > 
> > > 
> > > It is. A TPM Proxy is not considered a TIS or CRB, so it's ok to mix it up
> > > with another TPM device. The allowed combinations are:
> > > 
> > > - single vTPM device
> > > - single TPM Proxy device
> > > - a single vTPM + single TPM Proxy devices
> > 
> > So we do need multiple   support in the XML for this last case
> > then.
> 
> 
> Indeed we do. Working on it ATM. The plan is for this kind of XML format to 
> be valid:
> 
> 
> 
>   
> 
>   
> 
> 
>   
> 
>   
> 

Right, that sounds ok.


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 v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel Henrique Barboza]

On 5/12/20 1:44 PM, Daniel P. Berrangé wrote:

On Tue, May 12, 2020 at 01:21:40PM -0300, Daniel Henrique Barboza wrote:



On 5/12/20 12:53 PM, Daniel P. Berrangé wrote:

On Tue, May 12, 2020 at 11:21:52AM -0400, Stefan Berger wrote:

On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:


On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:

[...]

It's a different guest side interface, the H_TPM_COMM hypercall
instead of the other PAPR TPM interface.  To which "why?" is a very
good question, but it's there now, so there's not much we can do about
it.

That's ok. Even though its a different guest interface, it is still
conceptually a TPM device at a high level, so we should be reusing
the existing  device type. At most we should add a new backend
type

I think adding a new backend type is sensible. Re-using the passthrough type
and making the differentiation with 'model', for a device that doesn't
operate exactly as a regular vTPM but can coexist with other vTPM devices,
will make for a lot of IFs in the code.

Currently libvirt only allows a single , but we can trivially
lift that restriction to allow multiple if desired too.



QEMU won't accept multiple TIS or CRB devices, though.


The commit message says you can do 2 at a time:

"Although redundant, there is currently no technical
 limitation for a guest to assign both a vTPM and a TPM Proxy at the
 same time."

is that text not accurate ?



It is. A TPM Proxy is not considered a TIS or CRB, so it's ok to mix it up
with another TPM device. The allowed combinations are:

- single vTPM device
- single TPM Proxy device
- a single vTPM + single TPM Proxy devices


So we do need multiple   support in the XML for this last case
then.



Indeed we do. Working on it ATM. The plan is for this kind of XML format to be 
valid:



  

  


  

  




A TPM Proxy allows a second TPM device to be added, as long as it's not a 
second TPM
Proxy device.


Thanks,

DHB




Regards,
Daniel

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel P . Berrangé]

On Tue, May 12, 2020 at 01:21:40PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/12/20 12:53 PM, Daniel P. Berrangé wrote:
> > On Tue, May 12, 2020 at 11:21:52AM -0400, Stefan Berger wrote:
> > > On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:
> > > > On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:
> > > > > 
> > > > > On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:
> > > > > > On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:
> > > > > [...]
> > > > > > > It's a different guest side interface, the H_TPM_COMM hypercall
> > > > > > > instead of the other PAPR TPM interface.  To which "why?" is a 
> > > > > > > very
> > > > > > > good question, but it's there now, so there's not much we can do 
> > > > > > > about
> > > > > > > it.
> > > > > > That's ok. Even though its a different guest interface, it is still
> > > > > > conceptually a TPM device at a high level, so we should be reusing
> > > > > > the existing  device type. At most we should add a new backend
> > > > > > type
> > > > > I think adding a new backend type is sensible. Re-using the 
> > > > > passthrough type
> > > > > and making the differentiation with 'model', for a device that doesn't
> > > > > operate exactly as a regular vTPM but can coexist with other vTPM 
> > > > > devices,
> > > > > will make for a lot of IFs in the code.
> > > > Currently libvirt only allows a single , but we can trivially
> > > > lift that restriction to allow multiple if desired too.
> > > 
> > > 
> > > QEMU won't accept multiple TIS or CRB devices, though.
> > 
> > The commit message says you can do 2 at a time:
> > 
> >"Although redundant, there is currently no technical
> > limitation for a guest to assign both a vTPM and a TPM Proxy at the
> > same time."
> > 
> > is that text not accurate ?
> 
> 
> It is. A TPM Proxy is not considered a TIS or CRB, so it's ok to mix it up
> with another TPM device. The allowed combinations are:
> 
> - single vTPM device
> - single TPM Proxy device
> - a single vTPM + single TPM Proxy devices

So we do need multiple   support in the XML for this last case
then.


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 3/6] qemu: check if AMD secure guest support is enabled

[Boris Fiuczynski]

On 5/12/20 5:32 PM, Erik Skultety wrote:
I leave 1) and 2) to the AMD experts... :-)


3) Check if Qemu supports SEV feature (maybe we can detect the existence
of the query-sev QMP command or detect Qemu version >= 2.12)

^This is already achieved by qemuMonitorJSONGetSEVCapabilities


The idea is to check the host capabilities to detect if qemus capabilities
need to be reprobed. Therefore this would be a result if checks 1+2 change
but it would not be a cache invalidation trigger.

I agree in the sense that the SEV support that is currently being reported in
QEMU capabilities wasn't a good choice because it reports only platform data
which are constant to the host and have nothing to do with QEMU. It would be
okay to just say  in qemu capabilities and report the
rest in host capabilities. I haven't added this info into host capabilities
when I realized the mistake because I didn't want to duplicate the platform
info on 2 places.  For the IBM secure guest feature, it makes total sense to
report support within host capabilities, I'm just not sure whether we should do
the same for SEV and try really hard to "fix" the mess.


I am not sure I understand the "mess" correctly.
The idea is to prevent the cached qemu capabilities becoming stale which 
Daniel PB has called earlier a bug in the caching algorithm. This can 
occur if the kernel or firmware configuration change. To find out these 
situations we try to implement qemu capability cache invalidation 
triggers. The monitor method you mentioned is called during a (re-)probe 
and for IBM Secure Execution the host CPU model is also updated during a 
(re-)probe. Wouldn't that clean up the "mess"?


--
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 v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel Henrique Barboza]

On 5/11/20 8:50 AM, Daniel Henrique Barboza wrote:



On 5/11/20 8:28 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:



On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:

[...]

[...]


Currently libvirt only allows a single , but we can trivially
lift that restriction to allow multiple if desired too.


I don't believe it'll be necessary. Since it's only this TPM Proxy device that
can coexist with other TPMs, my idea is to do what I did here in this series,
but instead of creating a new device type I'll re-use the existing TPM device
in a 'tpmproxy' pointer in the domain for this case.

I'll still thinking about whether a new backend type is warranted or not. For
this PPC64 case alone it'll be simpler to just add a new 'model' called
'spapr-tpm'-proxy' for the existing TPM passthrough type. Creating a new
backend type makes it easier to add other TPM Proxy devices when other archs
implement it though.


Update: I tried it out the new "backend" approach and didn't enjoy the results.
It ended up replicating a large amount of existing cgroup/dac/selinux code that
handles the existing "passthrough" backend and, all said and done, it didn't 
alleviate
that much the parsing/format XML logic comparing to the alternative.

I chose then to go to the simpler route - adding a new 'passthrough' model 
called
'spapr-tpm-proxy'. This will not scale well if/when more TPM proxies devices are
added in the future, but we can cross that bridge when we come to it.


DHB






Thanks,

DHB




Regards,
Daniel

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel Henrique Barboza]

On 5/12/20 12:53 PM, Daniel P. Berrangé wrote:

On Tue, May 12, 2020 at 11:21:52AM -0400, Stefan Berger wrote:

On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:


On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:

[...]

It's a different guest side interface, the H_TPM_COMM hypercall
instead of the other PAPR TPM interface.  To which "why?" is a very
good question, but it's there now, so there's not much we can do about
it.

That's ok. Even though its a different guest interface, it is still
conceptually a TPM device at a high level, so we should be reusing
the existing  device type. At most we should add a new backend
type

I think adding a new backend type is sensible. Re-using the passthrough type
and making the differentiation with 'model', for a device that doesn't
operate exactly as a regular vTPM but can coexist with other vTPM devices,
will make for a lot of IFs in the code.

Currently libvirt only allows a single , but we can trivially
lift that restriction to allow multiple if desired too.



QEMU won't accept multiple TIS or CRB devices, though.


The commit message says you can do 2 at a time:

   "Although redundant, there is currently no technical
limitation for a guest to assign both a vTPM and a TPM Proxy at the
same time."

is that text not accurate ?



It is. A TPM Proxy is not considered a TIS or CRB, so it's ok to mix it up
with another TPM device. The allowed combinations are:

- single vTPM device
- single TPM Proxy device
- a single vTPM + single TPM Proxy devices

Not allowed:

- 2 or more vTPM devices
- 2 or more TPM Proxy devices



DHB



Regards,
Daniel

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Daniel P . Berrangé]

On Tue, May 12, 2020 at 11:21:52AM -0400, Stefan Berger wrote:
> On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:
> > On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:
> > > > On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:
> > > [...]
> > > > > It's a different guest side interface, the H_TPM_COMM hypercall
> > > > > instead of the other PAPR TPM interface.  To which "why?" is a very
> > > > > good question, but it's there now, so there's not much we can do about
> > > > > it.
> > > > That's ok. Even though its a different guest interface, it is still
> > > > conceptually a TPM device at a high level, so we should be reusing
> > > > the existing  device type. At most we should add a new backend
> > > > type
> > > I think adding a new backend type is sensible. Re-using the passthrough 
> > > type
> > > and making the differentiation with 'model', for a device that doesn't
> > > operate exactly as a regular vTPM but can coexist with other vTPM devices,
> > > will make for a lot of IFs in the code.
> > Currently libvirt only allows a single , but we can trivially
> > lift that restriction to allow multiple if desired too.
> 
> 
> QEMU won't accept multiple TIS or CRB devices, though.

The commit message says you can do 2 at a time:

  "Although redundant, there is currently no technical
   limitation for a guest to assign both a vTPM and a TPM Proxy at the
   same time."

is that text not accurate ?

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 1/5] cpu_map: Introduce ARM cpu models

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:16 +0800, ZhengZhenyu wrote:
> Introduce vendors and some commonly used models
> for ARM arch, these will be used for virConnectionGetCapabilities
> for ARM CPUs.
> 
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu_map/Makefile.inc.am   |  7 +++
>  src/cpu_map/arm_Falkor.xml| 16 
>  src/cpu_map/arm_Kunpeng-920.xml   | 24 
>  src/cpu_map/arm_ThunderX299xx.xml | 16 
>  src/cpu_map/arm_cortex-a53.xml| 16 
>  src/cpu_map/arm_cortex-a57.xml| 15 +++
>  src/cpu_map/arm_cortex-a72.xml| 15 +++
>  src/cpu_map/arm_vendors.xml   | 14 ++
>  src/cpu_map/index.xml | 15 +++
>  9 files changed, 138 insertions(+)
>  create mode 100644 src/cpu_map/arm_Falkor.xml
>  create mode 100644 src/cpu_map/arm_Kunpeng-920.xml
>  create mode 100644 src/cpu_map/arm_ThunderX299xx.xml
>  create mode 100644 src/cpu_map/arm_cortex-a53.xml
>  create mode 100644 src/cpu_map/arm_cortex-a57.xml
>  create mode 100644 src/cpu_map/arm_cortex-a72.xml
>  create mode 100644 src/cpu_map/arm_vendors.xml

This patch should be moved just before the last one to make sure libvirt
can be built after each patch.

> 
> diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
> index be64c9a0d4..93c2b19ddf 100644
> --- a/src/cpu_map/Makefile.inc.am
> +++ b/src/cpu_map/Makefile.inc.am
> @@ -2,7 +2,14 @@
>  
>  cpumapdir = $(pkgdatadir)/cpu_map
>  cpumap_DATA = \
> +cpu_map/arm_cortex-a53.xml \

This line is indented with 4 spaces while it should start with a tab
instead.

> + cpu_map/arm_cortex-a57.xml \
> + cpu_map/arm_cortex-a72.xml \
>   cpu_map/arm_features.xml \
> + cpu_map/arm_Kunpeng-920.xml \
> + cpu_map/arm_ThunderX299xx.xml \
> + cpu_map/arm_Falkor.xml \
> + cpu_map/arm_vendors.xml \
>   cpu_map/index.xml \
>   cpu_map/ppc64_vendors.xml \
>   cpu_map/ppc64_POWER7.xml \
> diff --git a/src/cpu_map/arm_Falkor.xml b/src/cpu_map/arm_Falkor.xml
> new file mode 100644
> index 00..902ed2b6ba
> --- /dev/null
> +++ b/src/cpu_map/arm_Falkor.xml
> @@ -0,0 +1,16 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

What is the purpose of the feature list here when you don't parse them
anywhere?

> +  
> +

Jirka

Re: [PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:24 +0800, ZhengZhenyu wrote:
> Introduce getHost support for ARM CPU driver, read
> CPU vendor_id, part_id and flags from registers
> directly. These codes will only be compiled on
> aarch64 hardwares.
> 
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 204 ++
>  1 file changed, 204 insertions(+)
> 
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 6e9ff9bf11..ec50a5615d 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -21,6 +21,10 @@
>   */
>  
>  #include 
> +#if defined(__aarch64__)
> +# include 
> +# include 
> +#endif
>  
>  #include "viralloc.h"
>  #include "cpu.h"
> @@ -32,6 +36,15 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_CPU
>  
> +#if defined(__aarch64__)
> +/* Shift bit mask for parsing cpu flags */
> +# define BIT_SHIFTS(n) (1UL << (n))
> +/* The current max number of cpu flags on ARM is 32 */
> +# define MAX_CPU_FLAGS 32
> +#endif
> +
> +VIR_LOG_INIT("cpu.cpu_arm");
> +
>  static const virArch archs[] = {
>  VIR_ARCH_ARMV6L,
>  VIR_ARCH_ARMV7B,
> @@ -491,12 +504,203 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
>  return 0;
>  }
>  
> +#if defined(__aarch64__)
> +/**
> + * virCPUarmCpuDataFromRegs:
> + *
> + * @data: 64-bit arm CPU specific data
> + *
> + * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU
> + * flags from AT_HWCAP. There are currently 32 valid flags  on ARM arch
> + * represented by each bit.
> + */
> +static int
> +virCPUarmCpuDataFromRegs(virCPUarmData *data)
> +{
> +/* Generate human readable flag list according to the order of */
> +/* AT_HWCAP bit map */
> +const char *flag_list[MAX_CPU_FLAGS] = {
> +"fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",
> +"crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",
> +"jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",
> +"asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",
> +"ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};

I'd move this array out of the function.

> +unsigned long cpuid, hwcaps;

One variable per line, please.

> +char **features = NULL;

VIR_AUTOSTRINGLIST features = NULL;

> +g_autofree char *cpu_feature_str = NULL;
> +int cpu_feature_index = 0;
> +size_t i;
> +
> +if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("CPUID registers unavailable"));
> +return -1;

Wrong indentation.

> +}
> +
> +/* read the cpuid data from MIDR_EL1 register */
> +asm("mrs %0, MIDR_EL1" : "=r" (cpuid));
> +VIR_DEBUG("CPUID read from register:  0x%016lx", cpuid);
> +
> +/* parse the coresponding part_id bits */
> +data->pvr = cpuid>>4&0xFFF;

Please, add spaces around the operators and consider using () for
clarity. So, I guess:

data->pvr = (cpuid >> 4) & 0xfff;

> +/* parse the coresponding vendor_id bits */
> +data->vendor_id = cpuid>>24&0xFF;

data->vendor_id = (cpuid >> 24) & 0xff;

> +
> +hwcaps = getauxval(AT_HWCAP);
> +VIR_DEBUG("CPU flags read from register:  0x%016lx", hwcaps);
> +
> +if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)
> +return -1;

The string list is supposed to be NULL-terminated so you need to
allocate space for MAX_CPU_FLAGS + 1:

features = g_new0(char *, MAX_CPU_FLAGS + 1);

> +
> +/* shift bit map mask to parse for CPU flags */
> +for (i = 0; i< MAX_CPU_FLAGS; i++) {

i < MAX_CPU_FLAGS

> +if (hwcaps & BIT_SHIFTS(i)) {
> +features[cpu_feature_index] = g_strdup(flag_list[i]);
> +cpu_feature_index++;
> +}

This could also be written as

if (hwcaps & BIT_SHIFTS(i))
features[cpu_feature_index++] = g_strdup(flag_list[i]);

it doesn't matter that much, though.

> +}
> +
> +if (cpu_feature_index > 1) {

This would not work for CPUs with exactly one feature.

> +cpu_feature_str = virStringListJoin((const char **)features, " ");
> +if (!cpu_feature_str)
> +goto error;
> +}
> +data->features = g_strdup(cpu_feature_str);

Just store the features string list in the data directly:

if (cpu_feature_index > 0)
data->features = g_steal_pointer();

> +
> +return 0;
> +
> + error:
> +virStringListFree(features);
> +return -1;

This can be dropped thanks to VIR_AUTOSTRINGLIST.

> +}
> +
> +static int
> +virCPUarmDataParseFeatures(virCPUDefPtr cpu,
> +   const virCPUarmData *cpuData)
> +{
> +int ret = -1;
> +size_t i;
> +char **features;
> +
> +if (!cpu || !cpuData)
> +return ret;
> +
> +if (!(features = virStringSplitCount(cpuData->features, " ",
> + 0, >nfeatures)))
> +return ret;

Please don't do this. The virCPUarmData can contain the feature list
directly.

> +if (cpu->nfeatures) {
> +if 

Re: [PATCH V3 4/5] cpu: Add helper funtions to parse vendor and model

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:22 +0800, ZhengZhenyu wrote:
> Add helper functions to parse vendor and model from
> xml for ARM arch, and use them as callbacks when
> load cpu maps.
> 
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 173 +-
>  1 file changed, 170 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 2009904cc9..6e9ff9bf11 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -204,6 +204,174 @@ virCPUarmMapFeatureParse(xmlXPathContextPtr ctxt 
> G_GNUC_UNUSED,
>  return 0;
>  }
>  
> +static virCPUarmVendorPtr
> +virCPUarmVendorFindByID(virCPUarmMapPtr map,
> +unsigned long vendor_id)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nvendors; i++) {
> +if (map->vendors[i]->value == vendor_id)
> +return map->vendors[i];
> +}
> +
> +return NULL;
> +}
> +
> +
> +static virCPUarmVendorPtr
> +virCPUarmVendorFindByName(virCPUarmMapPtr map,
> +  const char *name)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nvendors; i++) {
> +if (STREQ(map->vendors[i]->name, name))
> +return map->vendors[i];
> +}
> +
> +return NULL;
> +}
> +
> +
> +static int
> +virCPUarmVendorParse(xmlXPathContextPtr ctxt,
> +   const char *name,
> +   void *data)

Wrong indentation.

> +{
> +virCPUarmMapPtr map = data;
> +virCPUarmVendorPtr vendor = NULL;

This should be declared as

g_autoptr(virCPUarmVendor) vendor = NULL;

> +int ret = -1;

Remove this variable, please.

> +
> +if (VIR_ALLOC(vendor) < 0)
> +return ret;

vendor = g_new0(virCPUarmVendor, 1);

> +
> +vendor->name = g_strdup(name);
> +
> +if (virCPUarmVendorFindByName(map, vendor->name)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU vendor %s already defined"),
> +   vendor->name);
> +goto cleanup;

return -1;

> +}
> +
> +if (virXPathULongHex("string(@value)", ctxt, >value) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("Missing CPU vendor value"));
> +goto cleanup;

return -1;

> +}
> +
> +if (virCPUarmVendorFindByID(map, vendor->value)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU vendor value 0x%2lx already defined"),
> +   vendor->value);
> +goto cleanup;

return -1;

> +}
> +
> +if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
> +goto cleanup;

return -1;

> +
> +ret = 0;

return 0;

> +
> + cleanup:
> +virCPUarmVendorFree(vendor);
> +return ret;
> +

The cleanup section can be dropped.

> +}
> +
> +static virCPUarmModelPtr
> +virCPUarmModelFind(virCPUarmMapPtr map,
> +   const char *name)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nmodels; i++) {
> +if (STREQ(map->models[i]->name, name))
> +return map->models[i];
> +}
> +
> +return NULL;
> +}
> +
> +#if defined(__aarch64__)
> +static virCPUarmModelPtr
> +virCPUarmModelFindByPVR(virCPUarmMapPtr map,
> +unsigned long pvr)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nmodels; i++) {
> +if (map->models[i]->data.pvr == pvr)
> +return map->models[i];
> +}
> +
> +return NULL;
> +}
> +#endif
> +
> +static int
> +virCPUarmModelParse(xmlXPathContextPtr ctxt,
> +  const char *name,
> +  void *data)
> +{
> +virCPUarmMapPtr map = data;
> +virCPUarmModel *model;

g_autoptr(virCPUArmModel) model = NULL;

> +g_autofree xmlNodePtr *nodes = NULL;
> +g_autofree char *vendor = NULL;
> +
> +if (VIR_ALLOC(model) < 0)
> +goto error;

model = g_new0(...)

And the rest of this function should be modified in similarly to
VendorParse, i.e., s/goto error/return -1/ and drop the error section.

> +
> +model->name = g_strdup(name);
> +
> +if (virCPUarmModelFind(map, model->name)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU model %s already defined"),
> +   model->name);
> +goto error;
> +}
> +
> +if (virXPathBoolean("boolean(./vendor)", ctxt)) {
> +vendor = virXPathString("string(./vendor/@name)", ctxt);
> +if (!vendor) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Invalid vendor element in CPU model %s"),
> +   model->name);
> +goto error;
> +}
> +
> +if (!(model->vendor = virCPUarmVendorFindByName(map, vendor))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown vendor %s referenced by CPU model %s"),
> +   vendor, model->name);
> +goto error;
> +}
> +}
> +
> 

Re: [PATCH V3 3/5] cpu: Introduce ARM related structs

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:20 +0800, ZhengZhenyu wrote:
> Introduce vendor and model struct and related
> cleanup functions for ARM cpu.
> 
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 73 +++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index ee5802198f..2009904cc9 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
...
> @@ -81,12 +103,62 @@ virCPUarmMapNew(void)
>  return map;
>  }
>  
> +static void
> +virCPUarmDataClear(virCPUarmData *data)
> +{
> +if (!data)
> +return;
> +
> +VIR_FREE(data->features);

virStringListFree(data->features)

> +}
> +
> +static void
> +virCPUarmDataFree(virCPUDataPtr cpuData)
> +{
> +if (!cpuData)
> +return;
> +
> +virCPUarmDataClear(>data.arm);
> +VIR_FREE(cpuData);

g_free()

> +}

The two functions above should go in one patch with the structure
definition. Either you can move them to the previous patch or you can
squash the two patches into a single one.

> +
> +static void
> +virCPUarmModelFree(virCPUarmModelPtr model)
> +{
> +if (!model)
> +return;
> +
> +virCPUarmDataClear(>data);
> +g_free(model->name);
> +g_free(model);
> +}

Please, define the autoptr clean function for both model and vendor
structures so that you can later use g_autoptr(virCPUarm...) ... = NULL;
declarations:

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUarmModel, virCPUarmModelFree);

> +
> +static void
> +virCPUarmVendorFree(virCPUarmVendorPtr vendor)
> +{
> +if (!vendor)
> +return;
> +
> +g_free(vendor->name);
> +g_free(vendor);
> +}

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUarmVendor, virCPUarmVendorFree);

> +
>  static void
>  virCPUarmMapFree(virCPUarmMapPtr map)
>  {
>  if (!map)
>  return;
>  
> +size_t i;

We declare all variables in the beginning of each block, i.e., this
should go above the if (!map) check.

> +
> +for (i = 0; i < map->nmodels; i++)
> +virCPUarmModelFree(map->models[i]);
> +g_free(map->models);
> +
> +for (i = 0; i < map->nvendors; i++)
> +virCPUarmVendorFree(map->vendors[i]);
> +g_free(map->vendors);
> +
>  g_ptr_array_free(map->features, TRUE);
>  
>  g_free(map);
> @@ -259,6 +331,7 @@ struct cpuArchDriver cpuDriverArm = {
>  .compare = virCPUarmCompare,
>  .decode = NULL,
>  .encode = NULL,
> +.dataFree = virCPUarmDataFree,
>  .baseline = virCPUarmBaseline,
>  .update = virCPUarmUpdate,
>  .validateFeatures = virCPUarmValidateFeatures,

And the same applies for this hunk: it should go into the patch which
introduced virCPUarmData.

Jirka

Re: [PATCH V3 2/5] cpu: Introduce virCPUarmData to virCPUData

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:18 +0800, ZhengZhenyu wrote:
> Introduce virCPUarmData to virCPUData
> 
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/Makefile.inc.am |  1 +
>  src/cpu/cpu.h   |  2 ++
>  src/cpu/cpu_arm_data.h  | 31 +++
>  3 files changed, 34 insertions(+)
>  create mode 100644 src/cpu/cpu_arm_data.h

This patch needs to be the first one in the series and either the
corresponding hunks from the following patch should be squashed here or
the this and the following patch could be squashed into a single one.

...
> diff --git a/src/cpu/cpu_arm_data.h b/src/cpu/cpu_arm_data.h
> new file mode 100644
> index 00..cf12ca8c2e
> --- /dev/null
> +++ b/src/cpu/cpu_arm_data.h
> @@ -0,0 +1,31 @@
> +/*
> + * cpu_arm_data.h: 64-bit arm CPU specific data
> + *
> + * Copyright (C) 2020 Huawei Technologies Co., Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library;  If not, see
> + * .
> + *
> + */
> +
> +#pragma once
> +
> +#define VIR_CPU_ARM_DATA_INIT { 0 }
> +
> +typedef struct _virCPUarmData virCPUarmData;
> +struct _virCPUarmData {
> +unsigned long vendor_id;
> +unsigned long pvr;
> +char *features;

Make this a list of strings:

char **features;

> +};

Jirka

Re: [PATCH V3 0/5] Introduce getHost support for ARM CPU driver

[Jiri Denemark]

On Wed, Apr 22, 2020 at 15:11:14 +0800, ZhengZhenyu wrote:
> Introduce getHost support for ARM CPU driver. First add
> some data about commonly used ARM CPU models, and their
> vendors into cpu_map, then added some helper methods as
> callbacks to load them. Read and parse vendor_id, part_id
> and CPU flags of local CPU from corresponding registers.
> 
> Signed-off-by: Zhenyu Zheng 

The emails come from ZhengZhenyu , but the
signed-off-by line says something else. Could you make them consistent?

BTW, I'm intentionally replying to v3 as v4 was only supposed to have a
one line diff to v3, but it was sent with both html and plain text parts
and git am was unable to apply any of the v4 patches.

Jirka

Re: [PATCH 3/6] qemu: check if AMD secure guest support is enabled

[Erik Skultety]

On Tue, May 12, 2020 at 09:08:38AM +0200, Boris Fiuczynski wrote:
> On 5/11/20 9:40 PM, Brijesh Singh wrote:
> > Thanks for the patch Paulo, Few comments.
> > 
> > On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
> > > From: Paulo de Rezende Pinatti 
> > > 
> > > Implement secure guest check for AMD SEV (Secure Encrypted
> > > Virtualization) in order to invalidate the qemu capabilities
> > > cache in case the availability of the feature changed.
> > > 
> > > For AMD SEV the verification consists of:
> > > - checking if /sys/module/kvm_amd/parameters/sev contains the
> > > value '1': meaning SEV is enabled in the host kernel;
> > > - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
> > > SME memory encryption feature on the host is enabled
> > 
> > 
> > In addition to the kernel module parameter, we probably also need to
> > check whether QEMU supports the feature. e.g, what if user has newer
> > kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12.  To check the
> > SEV feature support, we should verify the following conditions:
> > 
> > 1) check kernel module parameter is set
> Paulo implemented the checks following the documentation in
> docs/kbase/launch_security_sev.rst. The check for the module parameter sev
> is included. Is the check for the kernel cmdline parameter "mem_encrypt" not
> necessary? Or would that be covered by your suggested check in 2)?

Brijesh correct me here please, but IIRC having mem_encrypt set is merely
a good security recommendation but is orthogonal with kvm.amd_sev feature, IOW
SEV should work without SME. If my memory serves well here, we don't need
and also should not parse the kernel cmdline in this case.

> 
> > 
> > 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?

> 
> > 
> > 3) Check if Qemu supports SEV feature (maybe we can detect the existence
> > of the query-sev QMP command or detect Qemu version >= 2.12)

^This is already achieved by qemuMonitorJSONGetSEVCapabilities

> The idea is to check the host capabilities to detect if qemus capabilities
> need to be reprobed. Therefore this would be a result if checks 1+2 change
> but it would not be a cache invalidation trigger.

I agree in the sense that the SEV support that is currently being reported in
QEMU capabilities wasn't a good choice because it reports only platform data
which are constant to the host and have nothing to do with QEMU. It would be
okay to just say  in qemu capabilities and report the
rest in host capabilities. I haven't added this info into host capabilities
when I realized the mistake because I didn't want to duplicate the platform
info on 2 places.  For the IBM secure guest feature, it makes total sense to
report support within host capabilities, I'm just not sure whether we should do
the same for SEV and try really hard to "fix" the mess.

Regards,
-- 
Erik Skultety

Re: [PATCH v1 1/8] docs: documentation and schema for the new TPM Proxy device

[Stefan Berger]

On 5/11/20 7:28 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 08:26:53AM -0300, Daniel Henrique Barboza wrote:


On 5/11/20 6:57 AM, Daniel P. Berrangé wrote:

On Mon, May 11, 2020 at 11:22:57AM +1000, David Gibson wrote:

[...]

It's a different guest side interface, the H_TPM_COMM hypercall
instead of the other PAPR TPM interface.  To which "why?" is a very
good question, but it's there now, so there's not much we can do about
it.

That's ok. Even though its a different guest interface, it is still
conceptually a TPM device at a high level, so we should be reusing
the existing  device type. At most we should add a new backend
type

I think adding a new backend type is sensible. Re-using the passthrough type
and making the differentiation with 'model', for a device that doesn't
operate exactly as a regular vTPM but can coexist with other vTPM devices,
will make for a lot of IFs in the code.

Currently libvirt only allows a single , but we can trivially
lift that restriction to allow multiple if desired too.



QEMU won't accept multiple TIS or CRB devices, though.




Regards,
Daniel

[PATCH 0/2] qemu: Switch to new -numa memdev=

[Michal Privoznik]

In a way, libvirt already uses -numa memdev= in a few cases. In fact, in
as few cases as possible - only configurations which can not be
configured with -numa mem=, because these two ways are incompatible when
it comes to migration.

My approach to solve this is to have a privateData flag which tells
which directs libvirt to generate old or new cmd line. And then this
flag is saved into migration cookie so that the destination is directed
the same way.

Problem with this approach is that, while migration 6.3.0 -> 6.4.0 ->
6.3.0 works, migration where the machine is started on newer libvirt
6.4.0 and then migrated back to 6.3.0 won't work (in fact is explicitly
denied by 2/2) even though there is nothing visible that should prevent
the migration.

I am not sure whether we have a good move here, because even if we
waited until QEMU removes the old way, it won't help us really. We
would be just leaving the problem for future us.

Michal Prívozník (2):
  qemu: Switch to new -numa memdev=
  qemu: Disallow migration to older -numa if newer is used

 src/qemu/qemu_command.c   | 20 ++---
 src/qemu/qemu_domain.c| 38 +-
 src/qemu/qemu_domain.h|  3 +
 src/qemu/qemu_migration.c | 18 -
 src/qemu/qemu_migration_cookie.c  | 73 +++
 src/qemu/qemu_migration_cookie.h  | 12 +++
 src/qemu/qemu_process.c   | 25 ++-
 .../qemustatusxml2xmldata/backup-pull-in.xml  |  1 +
 .../blockjob-blockdev-in.xml  |  1 +
 .../blockjob-mirror-in.xml|  1 +
 .../disk-secinfo-upgrade-in.xml   |  1 +
 .../disk-secinfo-upgrade-out.xml  |  1 +
 .../migration-in-params-in.xml|  1 +
 .../migration-out-nbd-in.xml  |  1 +
 .../migration-out-nbd-out.xml |  1 +
 .../migration-out-nbd-tls-in.xml  |  1 +
 .../migration-out-nbd-tls-out.xml |  1 +
 .../migration-out-params-in.xml   |  1 +
 tests/qemustatusxml2xmldata/modern-in.xml |  1 +
 .../qemustatusxml2xmldata/vcpus-multi-in.xml  |  1 +
 .../hugepages-numa-default-2M.args| 10 ++-
 .../hugepages-numa-default-dimm.args  |  8 +-
 .../hugepages-numa-default.args   |  6 +-
 .../memory-hotplug-dimm-addr.args |  3 +-
 .../qemuxml2argvdata/memory-hotplug-dimm.args |  3 +-
 ...y-hotplug-nvdimm-access.x86_64-latest.args |  3 +-
 ...ry-hotplug-nvdimm-align.x86_64-latest.args |  3 +-
 ...ry-hotplug-nvdimm-label.x86_64-latest.args |  3 +-
 ...ory-hotplug-nvdimm-pmem.x86_64-latest.args |  3 +-
 ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args |  3 +-
 ...hotplug-nvdimm-readonly.x86_64-latest.args |  3 +-
 .../memory-hotplug-nvdimm.x86_64-latest.args  |  3 +-
 .../numatune-auto-prefer.args |  4 +-
 .../qemuxml2argvdata/pages-dimm-discard.args  |  3 +-
 .../pages-discard-hugepages.args  | 11 ++-
 tests/qemuxml2argvdata/pages-discard.args | 12 ++-
 36 files changed, 236 insertions(+), 47 deletions(-)

-- 
2.26.2

[PATCH 2/2] qemu: Disallow migration to older -numa if newer is used

[Michal Privoznik]

As advertised in the previous commit, migration from -numa
memdev= to -numa mem= is not supported and results in error.
Fortunately, we can check whether the destination has used the
style we told it to, or if the corresponding flag in the
migration cookie is missing then we know we are talking to older
daemon which would have used the old approach. Anyway, let's deny
migration then.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_migration.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 423713e00b..dde491b720 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2424,7 +2424,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 cookieFlags = 0;
 } else {
 cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS |
-  QEMU_MIGRATION_COOKIE_CAPS;
+  QEMU_MIGRATION_COOKIE_CAPS |
+  QEMU_MIGRATION_COOKIE_PRIVATE;
 }
 
 if (flags & VIR_MIGRATE_POSTCOPY &&
@@ -3549,10 +3550,18 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
  cookiein, cookieinlen,
  cookieFlags |
  QEMU_MIGRATION_COOKIE_GRAPHICS |
- QEMU_MIGRATION_COOKIE_CAPS);
+ QEMU_MIGRATION_COOKIE_CAPS |
+ QEMU_MIGRATION_COOKIE_PRIVATE);
 if (!mig)
 goto error;
 
+if (priv->forceNewNuma == VIR_TRISTATE_BOOL_YES &&
+!(mig->priv && mig->priv->forceNewNuma == VIR_TRISTATE_BOOL_YES)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("Migration to older numa unsupported"));
+goto error;
+}
+
 if (qemuMigrationSrcGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
 VIR_WARN("unable to provide data for graphics client relocation");
 
-- 
2.26.2

[PATCH 1/2] qemu: Switch to new -numa memdev=

[Michal Privoznik]

In v4.1.0-rc0~9^2~25 QEMU deprecated -numa mem= in favor of -numa
memdev= + -object memory-backend-*. However, the problem is these
two are incompatible. A domain started with older cmd line can't
be migrated to the newer one and vice versa. That is why libvirt
hasn't switched to the new cmd line, until now.

Starting with this commit, the new cmd line is used and this fact
is then recorded in domain private data object under
priv->forceNewNuma. This means, that the status XML and migration
cookie have it too and thus can instruct the other daemon which
cmd line to generate.

Unfortunately, migration from newer to older style will fail.
I'm saving the explicit check for a separate commit.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1783355

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c   | 20 ++---
 src/qemu/qemu_domain.c| 38 +-
 src/qemu/qemu_domain.h|  3 +
 src/qemu/qemu_migration.c |  5 +-
 src/qemu/qemu_migration_cookie.c  | 73 +++
 src/qemu/qemu_migration_cookie.h  | 12 +++
 src/qemu/qemu_process.c   | 25 ++-
 .../qemustatusxml2xmldata/backup-pull-in.xml  |  1 +
 .../blockjob-blockdev-in.xml  |  1 +
 .../blockjob-mirror-in.xml|  1 +
 .../disk-secinfo-upgrade-in.xml   |  1 +
 .../disk-secinfo-upgrade-out.xml  |  1 +
 .../migration-in-params-in.xml|  1 +
 .../migration-out-nbd-in.xml  |  1 +
 .../migration-out-nbd-out.xml |  1 +
 .../migration-out-nbd-tls-in.xml  |  1 +
 .../migration-out-nbd-tls-out.xml |  1 +
 .../migration-out-params-in.xml   |  1 +
 tests/qemustatusxml2xmldata/modern-in.xml |  1 +
 .../qemustatusxml2xmldata/vcpus-multi-in.xml  |  1 +
 .../hugepages-numa-default-2M.args| 10 ++-
 .../hugepages-numa-default-dimm.args  |  8 +-
 .../hugepages-numa-default.args   |  6 +-
 .../memory-hotplug-dimm-addr.args |  3 +-
 .../qemuxml2argvdata/memory-hotplug-dimm.args |  3 +-
 ...y-hotplug-nvdimm-access.x86_64-latest.args |  3 +-
 ...ry-hotplug-nvdimm-align.x86_64-latest.args |  3 +-
 ...ry-hotplug-nvdimm-label.x86_64-latest.args |  3 +-
 ...ory-hotplug-nvdimm-pmem.x86_64-latest.args |  3 +-
 ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args |  3 +-
 ...hotplug-nvdimm-readonly.x86_64-latest.args |  3 +-
 .../memory-hotplug-nvdimm.x86_64-latest.args  |  3 +-
 .../numatune-auto-prefer.args |  4 +-
 .../qemuxml2argvdata/pages-dimm-discard.args  |  3 +-
 .../pages-discard-hugepages.args  | 11 ++-
 tests/qemuxml2argvdata/pages-discard.args | 12 ++-
 36 files changed, 225 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2d8a6fb0dd..3316aae419 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7028,8 +7028,6 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 char *tmpmask = NULL;
 char *next = NULL;
 virBufferPtr nodeBackends = NULL;
-bool needBackend = false;
-int rc;
 int ret = -1;
 size_t ncells = virDomainNumaGetNodeCount(def->numa);
 
@@ -7039,23 +7037,21 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 if (VIR_ALLOC_N(nodeBackends, ncells) < 0)
 goto cleanup;
 
-/* using of -numa memdev= cannot be combined with -numa mem=, thus we
- * need to check which approach to use */
+/* Using of -numa memdev= cannot be combined with -numa mem=.
+ * However, as of QEMU 4.1.0 using the latter is deprecated
+ * and the former is preferred. */
 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 ((rc = qemuBuildMemoryCellBackendStr(def, cfg, i, priv,
-[i])) < 0)
+if (qemuBuildMemoryCellBackendStr(def, cfg, i, priv,
+  [i]) < 0)
 goto cleanup;
-
-if (rc == 0)
-needBackend = true;
 }
 }
 
-if (!needBackend &&
+if (priv->forceNewNuma == VIR_TRISTATE_BOOL_NO &&
 qemuBuildMemPathStr(def, cmd, priv) < 0)
 goto cleanup;
 
@@ -7064,7 +7060,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, 
i
 goto cleanup;
 
-if (needBackend) {
+if (priv->forceNewNuma == VIR_TRISTATE_BOOL_YES) {
 virCommandAddArg(cmd, "-object");
 virCommandAddArgBuffer(cmd, [i]);
 }
@@ -7079,7 +7075,7 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 virBufferAdd(, 

Re: [libvirt PATCH] qemu: Support vmpvscsi controller model

[Peter Krempa]

On Tue, May 12, 2020 at 10:26:46 -0400, Chris Jester-Young wrote:
> On Mon, May 11, 2020 at 09:13:42AM +0200, Peter Krempa wrote:
> > It would be great if you could describe that 'pvscsi' indeed is the
> > vmware paravirtual scsi controller even in qemu, because I had to dig
> > through the code to figure it out.
> 
> Sure, just in the commit message? Or elsewhere too?

Commit message is probably enough.

> > Can the 'pvscsi' device be compiled out using any upstream way? If not
> > we don't really need a capability for it since qemu-1.5.3 is the oldest
> > supported qemu and all other versions support it as well.
> 
> As Daniel P. Berrangé mentioned, this can be configured out via Kconfig.
> Plus there are actually some files in tests/qemucapabilitiesdata that do
> not advertise that device (e.g., *.s390x.replies).

Yes, thus we need the capability. I'd slightly prefer if the capability
is added in a separate commit but that's not strictly required.

> 
> > The remainder of the patch looks okay to me, but it's missing a
> > qemuxml2argvtest case for the new controller. It looks like the
> > controller is using PCI addressing so the rest should be okay.
> 
> Sure thing, I'll be happy to add a test. Do I just take one of the
> existing tests and modify the XML and expected resultant command?

Both are actually fine. If there is an appropriate file where the data
would fit, just adding it to an existing test case is appropriate.

Otherwise you can copy a minimal test case and add the controller.

We prefer if new tests use DO_TEST_CAPS_LATEST, or the version-locked
variants of the test macro so that we are testing a "real" situation. It
also simplifies addition of the test.

Note that you can use VIR_TEST_REGENERATE_OUTPUT=1 env variable when
running the qemuxml2argvtest to force creation of the output file and
then just verify that it's as expected.

> > If you figure out that the capability is required, I'd prefer if the
> > addition of the capability is in a separate patch.
> 
> Can do. Given the (hopefully, given the patches from earlier today)
> impending arrival of merge requests, and the fact that I already have
> to redo the patch (since new capabilities have since been added), my
> current plan is to just post the new commits in a merge request.
> 
> (Don't worry, I won't actually file the MR until the CI system is in
> place.)

Well, CI is already there, but the main libvirt project still works on
the mail based workflow

[libvirt-ci PATCH v2 11/13] config: Move the virt-install settings from install.yml to the config

[Erik Skultety]

Looking into the future where we're able to generate cloudinit images,
we'll need to configure some of the install options which is currently
not possible without editing the install.yml group vars file within the
repository. That is suboptimal, so let's move the install options to
the global config under the 'install' section so that further tweaking
is possible (but explicitly discouraged at the same time).

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/config.yaml| 17 +
 guests/group_vars/all/install.yml | 11 ---
 guests/lcitool| 21 +++--
 3 files changed, 28 insertions(+), 21 deletions(-)
 delete mode 100644 guests/group_vars/all/install.yml

diff --git a/guests/config.yaml b/guests/config.yaml
index 05f1e8a..0b0b79c 100644
--- a/guests/config.yaml
+++ b/guests/config.yaml
@@ -15,6 +15,23 @@ install:
   # cases, SSH key authentication will be used instead. (Mandatory)
   root_password:
 
+  # Settings mapping to the virt-install options - see virt-install(1).
+  # It is strongly recommended that you keep the following at their default
+  # values to produce machines which conform to the upstream libvirt standard,
+  # unless you have a reason to do otherwise.
+  #
+  # Sizes are expressed in GiB.
+  #
+  virt_type: kvm
+  arch: x86_64
+  machine: pc
+  cpu_model: host-passthrough
+  vcpus: 2
+  memory_size: 2
+  disk_size: 15
+  storage_pool: default
+  network: default
+
 # GitLab-related options (only apply to the 'gitlab' flavor)
 gitlab:
 
diff --git a/guests/group_vars/all/install.yml 
b/guests/group_vars/all/install.yml
deleted file mode 100644
index 94b752f..000
--- a/guests/group_vars/all/install.yml
+++ /dev/null
@@ -1,11 +0,0 @@

-# Sizes are in GiB
-install_virt_type: kvm
-install_arch: x86_64
-install_machine: pc
-install_cpu_model: host-passthrough
-install_vcpus: 2
-install_memory_size: 2
-install_disk_size: 15
-install_storage_pool: default
-install_network: default
diff --git a/guests/lcitool b/guests/lcitool
index a93b56e..378c937 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -562,6 +562,7 @@ class Application:
 
 def _action_install(self, args):
 base = Util.get_base()
+config = self._config
 
 for host in self._inventory.expand_pattern(args.hosts):
 facts = self._inventory.get_facts(host)
@@ -569,16 +570,16 @@ class Application:
 # Both memory size and disk size are stored as GiB in the
 # inventory, but virt-install expects the disk size in GiB
 # and the memory size in *MiB*, so perform conversion here
-memory_arg = str(int(facts["install_memory_size"]) * 1024)
+memory_arg = str(config.values["install"]["memory_size"] * 1024)
 
-vcpus_arg = str(facts["install_vcpus"])
+vcpus_arg = str(config.values["install"]["vcpus"])
 
 disk_arg = "size={},pool={},bus=virtio".format(
-facts["install_disk_size"],
-facts["install_storage_pool"],
+config.values["install"]["disk_size"],
+config.values["install"]["storage_pool"],
 )
 network_arg = "network={},model=virtio".format(
-facts["install_network"],
+config.values["install"]["network"],
 )
 
 # Different operating systems require different configuration
@@ -638,10 +639,10 @@ class Application:
 virt_install,
 "--name", host,
 "--location", facts["install_url"],
-"--virt-type", facts["install_virt_type"],
-"--arch", facts["install_arch"],
-"--machine", facts["install_machine"],
-"--cpu", facts["install_cpu_model"],
+"--virt-type", config.values["install"]["virt_type"],
+"--arch", config.values["install"]["arch"],
+"--machine", config.values["install"]["machine"],
+"--cpu", config.values["install"]["cpu_model"],
 "--vcpus", vcpus_arg,
 "--memory", memory_arg,
 "--disk", disk_arg,
@@ -658,7 +659,7 @@ class Application:
 cmd.append("--noautoconsole")
 
 # Only configure autostart for the guest for the jenkins flavor
-if self._config.values["install"]["flavor"] == "jenkins":
+if config.values["install"]["flavor"] == "jenkins":
 cmd += ["--autostart"]
 
 try:
-- 
2.25.3

[libvirt-ci PATCH v2 12/13] guests: README: Document the existence and usage of config.yaml

[Erik Skultety]

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/README.markdown | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/guests/README.markdown b/guests/README.markdown
index 7dbefed..0e70f5f 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -64,13 +64,15 @@ branches out of those with
 Host setup
 --
 
-Ansible and `virt-install` need to be available on the host.
+`ansible` and `virt-install` need to be available on the host, the former can
+be either installed system-wide using your package manager or using `pip`
+(see the provided requirements.txt file). The latter can only be installed with
+your package manager as `virt-install` is not distributed via PyPI.
 
-Before you can start bringing up guests, you'll have to store your
-site-specific root password in the `~/.config/lcitool/root-password` file.
-This password will only be necessary for serial console access in case
-something goes horribly wrong; for day to day operations, SSH key
-authentication will be used instead.
+Before you can start bringing up guests, you need to create
+~/.config/lcitool/config.yaml, ideally by copying the `config.yaml` template,
+and set at least the options marked as "(mandatory)" depending on the flavor
+(`test`, `jenkins`, `gitlab`) you wish to use with your machines.
 
 Ansible expects to be able to connect to the guests by name: installing and
 enabling the [libvirt NSS plugin](https://wiki.libvirt.org/page/NSS_module)
@@ -123,8 +125,8 @@ Jenkins CI use
 --
 
 You'll need to configure `lcitool` to use the `jenkins` flavor for
-guests: to do so, just write `jenkins` in the `~/.config/lcitool/flavor`
-file.
+guests. To do so, simply set the `install.flavor` to `jenkins` in
+`~/.config/lcitool/config.yaml`.
 
 Once a guest has been prepared, you'll be able to log in as root either
 via SSH (your public key will have been authorized) or on the serial
-- 
2.25.3

[libvirt-ci PATCH v2 10/13] lcitool: Drop the gitlab-related getter methods

[Erik Skultety]

We can now access the values directly in the config dictionary.

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 40 
 1 file changed, 40 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 9dd4aea..a93b56e 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -252,44 +252,6 @@ class Config:
 
 return vault_pass_file
 
-def get_gitlab_runner_token_file(self):
-if self.values["install"]["flavor"] != "gitlab":
-return None
-
-gitlab_runner_token_file = self._get_config_file("gitlab-runner-token")
-
-try:
-with open(gitlab_runner_token_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid GitLab runner token file ({}): {}".format(
-gitlab_runner_token_file, ex
-)
-)
-
-return gitlab_runner_token_file
-
-def get_gitlab_url_file(self):
-if self.values["install"]["flavor"] != "gitlab":
-return None
-
-gitlab_url_file = self._get_config_file("gitlab-url")
-
-try:
-with open(gitlab_url_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid GitLab url file ({}): {}".format(
-gitlab_url_file, ex
-)
-)
-
-return gitlab_url_file
-
 
 class Inventory:
 
@@ -527,8 +489,6 @@ class Application:
 base = Util.get_base()
 
 vault_pass_file = self._config.get_vault_password_file()
-gitlab_url_file = self._config.get_gitlab_url_file()
-gitlab_runner_token_file = self._config.get_gitlab_runner_token_file()
 
 ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
 selected_projects = self._projects.expand_pattern(projects)
-- 
2.25.3

[libvirt-ci PATCH v2 13/13] DO NOT MERGE: Demonstrate functionality with pytest unit tests

[Erik Skultety]

This patch exercises the functionality added to the Config class by
defining a couple of pytest unit tests. The pytest script imports
lcitool as a local module (which is far from ideal), but in order to do
that, lcitool has to adopt the .py extension otherwise python refuses
to import it, but then again, this patch is just to showcase the
functionality.
---
 guests/{lcitool => lcitool.py} |   0
 guests/requirements.txt|   2 +
 guests/test_config.py  | 165 +
 3 files changed, 167 insertions(+)
 rename guests/{lcitool => lcitool.py} (100%)
 create mode 100644 guests/test_config.py

diff --git a/guests/lcitool b/guests/lcitool.py
similarity index 100%
rename from guests/lcitool
rename to guests/lcitool.py
diff --git a/guests/requirements.txt b/guests/requirements.txt
index 900cfd6..c2b8f2c 100644
--- a/guests/requirements.txt
+++ b/guests/requirements.txt
@@ -1,2 +1,4 @@
 ansible
 jenkins-job-builder
+pytest
+mock
diff --git a/guests/test_config.py b/guests/test_config.py
new file mode 100644
index 000..09343cc
--- /dev/null
+++ b/guests/test_config.py
@@ -0,0 +1,165 @@
+import os
+import pytest
+import yaml
+from mock import patch
+import lcitool
+
+empty_config = ""
+
+missing_mandatory_key = \
+"""
+install:
+flavor: test
+"""
+
+empty_mandatory_key = \
+"""
+install:
+flavor: test
+root_password:
+"""
+
+empty_optional_key = \
+"""
+install:
+flavor:
+root_password: foo
+"""
+
+extra_key = \
+"""
+install:
+root_password: foo
+bar: baz
+"""
+
+empty_flavor = \
+"""
+install:
+root_password: foo
+"""
+
+invalid_flavor = \
+"""
+install:
+flavor: bar
+root_password: foo
+"""
+
+invalid_value_type = \
+"""
+install:
+flavor: bar
+root_password:
+- let
+- me
+- in
+"""
+
+
+def Config_init_mock(self):
+# load only the default config template here
+base = lcitool.Util.get_base()
+with open(os.path.join(base, "config.yaml"), "r") as fp:
+self.values = yaml.safe_load(fp)
+
+
+@pytest.fixture
+def config():
+with patch.object(config, "__init__", Config_init_mock):
+return lcitool.Config()
+
+
+@pytest.mark.parametrize("yamlstr,_pass", [
+(empty_config, False),
+(missing_mandatory_key, False),
+(empty_mandatory_key, False),
+(empty_optional_key, False),
+(invalid_flavor, False),
+(invalid_value_type, False),
+(extra_key, True),
+(empty_flavor, True),
+])
+def test_config_validate(config, yamlstr, _pass):
+user_config = yaml.safe_load(yamlstr)
+
+if _pass:
+config._validate(user_config)
+else:
+with pytest.raises(Exception):
+config._validate(user_config)
+
+
+yaml_in1 = \
+"""
+install:
+root_password: foo
+vcpus: 4
+"""
+
+dict_out1 = {
+"install": {
+"flavor": "test",
+"root_password": "foo",
+"virt_type": "kvm",
+"arch": "x86_64",
+"machine": "pc",
+"cpu_model": "host-passthrough",
+"vcpus": 4,
+"memory_size": 2,
+"disk_size": 15,
+"storage_pool": "default",
+"network": "default"},
+"gitlab": {
+"url": "https://gitlab.com;,
+"runner_secret": None}
+}
+
+yaml_in2 = \
+"""
+install:
+flavor: gitlab
+root_password: foo
+virt_type: qemu
+arch: aarch64
+machine: q35
+cpu_model: host-passthrough
+vcpus: 4
+memory_size: 4
+disk_size: 8
+storage_pool: foo
+network: bar
+gitlab:
+url: https://example.com
+runner_secret: foobar
+"""
+
+dict_out2 = {
+"install": {
+"flavor": "gitlab",
+"root_password": "foo",
+"virt_type": "qemu",
+"arch": "aarch64",
+"machine": "q35",
+"cpu_model": "host-passthrough",
+"vcpus": 4,
+"memory_size": 4,
+"disk_size": 8,
+"storage_pool": "foo",
+"network": "bar"},
+"gitlab": {
+"url": "https://example.com;,
+"runner_secret": "foobar"}
+}
+
+
+@pytest.mark.parametrize("yaml_in, dict_out", [
+(yaml_in1, dict_out1),
+(yaml_in2, dict_out2)
+])
+def test_compare_config_contents(config, yaml_in, dict_out):
+parsed = yaml.safe_load(yaml_in)
+
+# fill in the default values
+config._update(parsed)
+assert config.values == dict_out
-- 
2.25.3

[libvirt-ci PATCH v2 09/13] lcitool: Drop the get_root_password_file() method

[Erik Skultety]

We can now access this value directly in the config dictionary.

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 17 -
 1 file changed, 17 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index fa60218..9dd4aea 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -252,22 +252,6 @@ class Config:
 
 return vault_pass_file
 
-def get_root_password_file(self):
-root_pass_file = self._get_config_file("root-password")
-
-try:
-with open(root_pass_file, "r") as infile:
-if not infile.readline().strip():
-raise ValueError
-except Exception as ex:
-raise Exception(
-"Missing or invalid root password file ({}): {}".format(
-root_pass_file, ex
-)
-)
-
-return root_pass_file
-
 def get_gitlab_runner_token_file(self):
 if self.values["install"]["flavor"] != "gitlab":
 return None
@@ -543,7 +527,6 @@ class Application:
 base = Util.get_base()
 
 vault_pass_file = self._config.get_vault_password_file()
-root_pass_file = self._config.get_root_password_file()
 gitlab_url_file = self._config.get_gitlab_url_file()
 gitlab_runner_token_file = self._config.get_gitlab_runner_token_file()
 
-- 
2.25.3

[libvirt-ci PATCH v2 07/13] lcitool: Update the config values with internal playbook settings

[Erik Skultety]

So, the idea is to pass our YAML config to the Ansible playbooks as
extra vars. However, not all variables we need to pass on to Ansible
are exposed in the config (and they shouldn't be). Update the config
values dictionary with these variables before passing on to Ansible.

Signed-off-by: Erik Skultety 
---
 guests/lcitool| 10 ++
 guests/playbooks/build/main.yml   |  2 +-
 guests/playbooks/update/main.yml  |  6 ++--
 guests/playbooks/update/tasks/gitlab.yml  |  4 +--
 guests/playbooks/update/tasks/kludges.yml |  2 +-
 guests/playbooks/update/tasks/users.yml   | 42 +++
 6 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 577e9d2..3270fca 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -595,20 +595,16 @@ class Application:
 playbook_path = os.path.join(playbook_base, "main.yml")
 extra_vars_path = os.path.join(tempdir.name, "extra_vars.json")
 
-extra_vars = {
+self._config.values.update({
 "base": base,
 "playbook_base": playbook_base,
-"root_password_file": root_pass_file,
-"flavor": flavor,
 "selected_projects": selected_projects,
 "git_remote": git_remote,
 "git_branch": git_branch,
-"gitlab_url_file": gitlab_url_file,
-"gitlab_runner_token_file": gitlab_runner_token_file,
-}
+})
 
 with open(extra_vars_path, "w") as fp:
-json.dump(extra_vars, fp)
+json.dump(self._config.values, fp)
 
 ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
 if ansible_playbook is None:
diff --git a/guests/playbooks/build/main.yml b/guests/playbooks/build/main.yml
index 8abda67..462764b 100644
--- a/guests/playbooks/build/main.yml
+++ b/guests/playbooks/build/main.yml
@@ -1,6 +1,6 @@
 ---
 - hosts: all
-  remote_user: '{{ flavor }}'
+  remote_user: '{{ install.flavor }}'
 
   vars_files:
 - '{{ playbook_base }}/jobs/defaults.yml'
diff --git a/guests/playbooks/update/main.yml b/guests/playbooks/update/main.yml
index 371e53d..1b97027 100644
--- a/guests/playbooks/update/main.yml
+++ b/guests/playbooks/update/main.yml
@@ -45,7 +45,7 @@
   vars:
 project: jenkins
   when:
-- flavor == "jenkins"
+- install.flavor == "jenkins"
 
 # Configure environment. Needs to happen after installing packages
 - include: '{{ playbook_base }}/tasks/kludges.yml'
@@ -57,9 +57,9 @@
 # Configure the Jenkins agent
 - include: '{{ playbook_base }}/tasks/jenkins.yml'
   when:
-- flavor == 'jenkins'
+- install.flavor == 'jenkins'
 
 # Install the Gitlab runner agent
 - include: '{{ playbook_base }}/tasks/gitlab.yml'
   when:
-- flavor == 'gitlab'
+- install.flavor == 'gitlab'
diff --git a/guests/playbooks/update/tasks/gitlab.yml 
b/guests/playbooks/update/tasks/gitlab.yml
index f07279c..07a376c 100644
--- a/guests/playbooks/update/tasks/gitlab.yml
+++ b/guests/playbooks/update/tasks/gitlab.yml
@@ -1,8 +1,6 @@
 ---
 - name: Define gitlab-related facts
   set_fact:
-gitlab_url: '{{ lookup("file", gitlab_url_file) }}'
-gitlab_runner_secret: '{{ lookup("file", gitlab_runner_token_file) }}'
 gitlab_runner_download_url: 
https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-{{
 ansible_system|lower }}-amd64
 gitlab_runner_config_dir: '/etc/gitlab-runner'
 
@@ -14,7 +12,7 @@
 force: yes
 
 - name: Register the gitlab-runner agent
-  shell: 'gitlab-runner register --non-interactive --config "{{ 
gitlab_runner_config_dir }}/config.toml" --registration-token "{{ 
gitlab_runner_secret }}" --url "{{ gitlab_url }}" --executor shell --tag-list 
"{{ os.name|lower }}-{{ os.version }}"'
+  shell: 'gitlab-runner register --non-interactive --config "{{ 
gitlab_runner_config_dir }}/config.toml" --registration-token "{{ 
gitlab.runner_secret }}" --url "{{ gitlab.url }}" --executor shell --tag-list 
"{{ os.name|lower }}-{{ os.version }}"'
   args:
 creates: '{{ gitlab_runner_config_dir }}/config.toml'
 
diff --git a/guests/playbooks/update/tasks/kludges.yml 
b/guests/playbooks/update/tasks/kludges.yml
index 96fe1a5..33c6532 100644
--- a/guests/playbooks/update/tasks/kludges.yml
+++ b/guests/playbooks/update/tasks/kludges.yml
@@ -12,7 +12,7 @@
 group: wheel
   when:
 - os.name == 'FreeBSD'
-- flavor == "jenkins"
+- install.flavor == "jenkins"
 
 # FreeBSD compiles bash without defining SSH_SOURCE_BASHRC, which means
 # it won't try to detect when it's spawned by ssh and source ~/.bashrc
diff --git a/guests/playbooks/update/tasks/users.yml 
b/guests/playbooks/update/tasks/users.yml
index 5c6ce8f..bc3cc11 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -2,7 +2,7 @@
 - name: 'root: Set password'
   user:
 name: root
-  

[libvirt-ci PATCH v2 02/13] lcitool: Decrease the indent when creating a tempdir for initrd injection

[Erik Skultety]

The 'with' statement doesn't define a new code block [1], thus no local
namespace is created. Therefore, we can still access the @content
variable outside of the 'with' block. So, there's really no need to
hold the @initrd_template file open longer than necessary (not that it
would be a big deal anyway).

[1] https://docs.python.org/3.7/reference/executionmodel.html

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 88eb248..dcc54ae 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -610,11 +610,11 @@ class Application:
 facts[option]
 )
 
-tempdir = tempfile.mkdtemp()
-initrd_inject = os.path.join(tempdir, install_config)
+tempdir = tempfile.mkdtemp()
+initrd_inject = os.path.join(tempdir, install_config)
 
-with open(initrd_inject, "w") as inject:
-inject.write(content)
+with open(initrd_inject, "w") as inject:
+inject.write(content)
 
 # preseed files must use a well-known name to be picked up by
 # d-i; for kickstart files, we can use whatever name we please
-- 
2.25.3

[libvirt-ci PATCH v2 06/13] lcitool: Introduce methods to load and validate the YAML config

[Erik Skultety]

This patch introduce a set of class Config helper methods in order to
parse and validate the new global YAML config.
Currently, only 'install' and 'gitlab' sections are recognized with
the flavor setting defaulting to "test" (backwards compatibility) and
gitlab runner registration url defaulting to "https://gitlab.com;; the
rest of the options are currently mandatory.

Signed-off-by: Erik Skultety 
---
 guests/lcitool | 82 ++
 1 file changed, 82 insertions(+)

diff --git a/guests/lcitool b/guests/lcitool
index 5b44582..577e9d2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -128,6 +128,30 @@ class Util:
 
 class Config:
 
+def __init__(self):
+
+# Load the template config containing the defaults first, this must
+# always succeed.
+# NOTE: we should load this from /usr/share once we start packaging
+# lcitool
+base = Util.get_base()
+with open(os.path.join(base, "config.yaml"), "r") as fp:
+self.values = yaml.safe_load(fp)
+
+try:
+with open(self._get_config_file("config.yaml"), "r") as fp:
+user_config = yaml.safe_load(fp)
+except Exception as e:
+raise Exception("Missing or invalid config.yaml file: 
{}".format(e))
+
+if user_config is None:
+raise Exception("Missing or invalid config.yaml file")
+
+# Validate the user provided config and use it to override the default
+# settings
+self._validate(user_config)
+self._update(user_config)
+
 @staticmethod
 def _get_config_file(name):
 try:
@@ -149,6 +173,64 @@ class Config:
 
 return os.path.join(config_dir, name)
 
+@staticmethod
+def _remove_unknown_keys(_dict, known_keys):
+keys = list(_dict.keys())
+
+for k in keys:
+if k not in known_keys:
+del _dict[k]
+
+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:
+raise Exception(("Missing or empty value for mandatory key"
+ "'{}.{}'").format(section, key))
+
+# check that all keys have values assigned and of the right type
+for key in config[section].keys():
+
+# mandatory keys were already checked, so this covers optional keys
+if config[section][key] is None:
+raise Exception(
+"Missing value for '{}.{}'".format(section, key)
+)
+
+if not isinstance(config[section][key], (str, int)):
+raise Exception(
+"Invalid type for key '{}.{}'".format(section, key)
+)
+
+def _validate(self, config):
+# delete sections we don't recognize
+self._remove_unknown_keys(config, self.values.keys())
+
+if "install" not in config:
+raise Exception("Missing mandatory section 'install'")
+
+self._validate_section(config, "install", ["root_password"])
+
+# we only need this for the gitlab check below, if 'flavor' is missing
+# that's okay, we'll provide a default later
+flavor = config["install"].get("flavor", None)
+if flavor is not None and flavor not in ["test", "jenkins", "gitlab"]:
+raise Exception(
+"Invalid value '{}' for 'install.flavor'".format(flavor)
+)
+
+if flavor == "gitlab":
+self._validate_section(config, "gitlab", ["runner_secret"])
+
+def _update(self, values):
+self.values["install"].update(values["install"])
+
+if values.get("gitlab", None):
+self.values["gitlab"].update(values["gitlab"])
+
 def get_flavor(self):
 flavor_file = self._get_config_file("flavor")
 
-- 
2.25.3

[libvirt-ci PATCH v2 08/13] lcitool: Drop the get_flavor() method

[Erik Skultety]

We can now access this value directly in the config dictionary.

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 36 
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 3270fca..fa60218 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -231,37 +231,12 @@ class Config:
 if values.get("gitlab", None):
 self.values["gitlab"].update(values["gitlab"])
 
-def get_flavor(self):
-flavor_file = self._get_config_file("flavor")
-
-try:
-with open(flavor_file, "r") as infile:
-flavor = infile.readline().strip()
-except Exception:
-# If the flavor has not been configured, we choose the default
-# and store it on disk to ensure consistent behavior from now on
-flavor = "test"
-try:
-with open(flavor_file, "w") as infile:
-infile.write("{}\n".format(flavor))
-except Exception as ex:
-raise Exception(
-"Can't write flavor file ({}): {}".format(
-flavor_file, ex
-)
-)
-
-if flavor not in ["test", "jenkins", "gitlab"]:
-raise Exception("Invalid flavor '{}'".format(flavor))
-
-return flavor
-
 def get_vault_password_file(self):
 vault_pass_file = None
 
 # The vault password is only needed for the jenkins flavor, but in
 # that case we want to make sure there's *something* in there
-if self.get_flavor() == "jenkins":
+if self.values["install"]["flavor"] == "jenkins":
 vault_pass_file = self._get_config_file("vault-password")
 
 try:
@@ -294,7 +269,7 @@ class Config:
 return root_pass_file
 
 def get_gitlab_runner_token_file(self):
-if self.get_flavor() != "gitlab":
+if self.values["install"]["flavor"] != "gitlab":
 return None
 
 gitlab_runner_token_file = self._get_config_file("gitlab-runner-token")
@@ -313,7 +288,7 @@ class Config:
 return gitlab_runner_token_file
 
 def get_gitlab_url_file(self):
-if self.get_flavor() != "gitlab":
+if self.values["install"]["flavor"] != "gitlab":
 return None
 
 gitlab_url_file = self._get_config_file("gitlab-url")
@@ -567,7 +542,6 @@ class Application:
 def _execute_playbook(self, playbook, hosts, projects, git_revision):
 base = Util.get_base()
 
-flavor = self._config.get_flavor()
 vault_pass_file = self._config.get_vault_password_file()
 root_pass_file = self._config.get_root_password_file()
 gitlab_url_file = self._config.get_gitlab_url_file()
@@ -646,8 +620,6 @@ class Application:
 def _action_install(self, args):
 base = Util.get_base()
 
-flavor = self._config.get_flavor()
-
 for host in self._inventory.expand_pattern(args.hosts):
 facts = self._inventory.get_facts(host)
 
@@ -743,7 +715,7 @@ class Application:
 cmd.append("--noautoconsole")
 
 # Only configure autostart for the guest for the jenkins flavor
-if flavor == "jenkins":
+if self._config.values["install"]["flavor"] == "jenkins":
 cmd += ["--autostart"]
 
 try:
-- 
2.25.3

[libvirt-ci PATCH v2 05/13] config: Introduce a new global config.yaml configuration file

[Erik Skultety]

Rather than having the configuration options split across multiple files
(root-password, flavor, gitlab-url, gitlab-runner-token, ...), let's
consolidate these settings into a global config file.

The YAML format has been chosen simply because it's the native data
format in Ansible and thus plays very nicely when accessing variables
within Ansible playbooks.

Signed-off-by: Erik Skultety 
---
 guests/config.yaml | 25 +
 1 file changed, 25 insertions(+)
 create mode 100644 guests/config.yaml

diff --git a/guests/config.yaml b/guests/config.yaml
new file mode 100644
index 000..05f1e8a
--- /dev/null
+++ b/guests/config.yaml
@@ -0,0 +1,25 @@
+---
+# Configuration file for lcitool
+
+install:
+
+  # Installation flavor determining the target environment for the VM:
+  #
+  # test - VMs suitable for local testing (the 'test' user has sudo privileges)
+  # jenkins - VMs ready to be plugged into a Jenkins environment
+  # gitlab - VMs ready to be plugged into a GitLab environment
+  flavor: test
+
+  # Root password for the VM. This password will only be necessary for serial
+  # console access in case something goes horribly wrong, for all other use
+  # cases, SSH key authentication will be used instead. (Mandatory)
+  root_password:
+
+# GitLab-related options (only apply to the 'gitlab' flavor)
+gitlab:
+
+  # GitLab connection URL
+  url: https://gitlab.com
+
+  # GitLab runner registration token. (Mandatory)
+  runner_secret:
-- 
2.25.3

[libvirt-ci PATCH v2 03/13] lcitool: Prefer tempfile's native wrappers over low level primitives

[Erik Skultety]

Rather than requiring shutil module to get rid of the temporary
directory we're creating for virt-install, let's use the
TemporaryDirectory method instead which returns a file-like object which
can be used to clean up the standard Python way.
Although the internal exit handlers will take care of closing the
temporary directories (and thus removing their contents) automatically,
let's be explicit anyway and use the 'finally' clause to clean these up
on both success and failure.

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index dcc54ae..56eb543 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -24,7 +24,6 @@ import json
 import os
 import platform
 import random
-import shutil
 import string
 import subprocess
 import sys
@@ -610,8 +609,8 @@ class Application:
 facts[option]
 )
 
-tempdir = tempfile.mkdtemp()
-initrd_inject = os.path.join(tempdir, install_config)
+tempdir = tempfile.TemporaryDirectory(prefix="lcitool")
+initrd_inject = os.path.join(tempdir.name, install_config)
 
 with open(initrd_inject, "w") as inject:
 inject.write(content)
@@ -665,8 +664,8 @@ class Application:
 subprocess.check_call(cmd)
 except Exception as ex:
 raise Exception("Failed to install '{}': {}".format(host, ex))
-
-shutil.rmtree(tempdir, ignore_errors=True)
+finally:
+tempdir.cleanup()
 
 def _action_update(self, args):
 self._execute_playbook("update", args.hosts, args.projects,
-- 
2.25.3

[libvirt-ci PATCH v2 04/13] lcitool: Use a temporary JSON file to pass extra variables

[Erik Skultety]

This patch is a pre-requisite config file consolidation. Currently we've
got a number of files which serve as a configuration either to the
lcitool itself or to the ansible playbooks (majority).  Once we replace
these with a single global lcitool config, we'd end up passing tokens
(potentially some passwords) as ansible extra variables bare naked on
the cmdline. In order to prevent this security flaw use temporary JSON
file holding all these extra variables and pass it as follows:

$ ansible-playbook --extra-vars @extra_vars.json playbook.yml

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/lcitool | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 56eb543..5b44582 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -506,11 +506,14 @@ class Application:
 git_remote = "default"
 git_branch = "master"
 
+tempdir = tempfile.TemporaryDirectory(prefix="lcitool")
+
 ansible_cfg_path = os.path.join(base, "ansible.cfg")
 playbook_base = os.path.join(base, "playbooks", playbook)
 playbook_path = os.path.join(playbook_base, "main.yml")
+extra_vars_path = os.path.join(tempdir.name, "extra_vars.json")
 
-extra_vars = json.dumps({
+extra_vars = {
 "base": base,
 "playbook_base": playbook_base,
 "root_password_file": root_pass_file,
@@ -520,7 +523,10 @@ class Application:
 "git_branch": git_branch,
 "gitlab_url_file": gitlab_url_file,
 "gitlab_runner_token_file": gitlab_runner_token_file,
-})
+}
+
+with open(extra_vars_path, "w") as fp:
+json.dump(extra_vars, fp)
 
 ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
 if ansible_playbook is None:
@@ -529,7 +535,7 @@ class Application:
 cmd = [
 ansible_playbook,
 "--limit", ansible_hosts,
-"--extra-vars", extra_vars,
+"--extra-vars", "@" + extra_vars_path,
 ]
 
 # Provide the vault password if available
@@ -548,6 +554,8 @@ class Application:
 except Exception as ex:
 raise Exception(
 "Failed to run {} on '{}': {}".format(playbook, hosts, ex))
+finally:
+tempdir.cleanup()
 
 def _action_hosts(self, args):
 for host in self._inventory.expand_pattern("all"):
-- 
2.25.3

[libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool

[Erik Skultety]

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

Erik Skultety (13):
  requirements: Introduce a requirements.txt file
  lcitool: Decrease the indent when creating a tempdir for initrd
injection
  lcitool: Prefer tempfile's native wrappers over low level primitives
  lcitool: Use a temporary JSON file to pass extra variables
  config: Introduce a new global config.yaml configuration file
  lcitool: Introduce methods to load and validate the YAML config
  lcitool: Update the config values with internal playbook settings
  lcitool: Drop the get_flavor() method
  lcitool: Drop the get_root_password_file() method
  lcitool: Drop the gitlab-related getter methods
  config: Move the virt-install settings from install.yml to the config
  guests: README: Document the existence and usage of config.yaml
  DO NOT MERGE: Demonstrate functionality with pytest unit tests

 guests/README.markdown|  18 +-
 guests/config.yaml|  42 +
 guests/group_vars/all/install.yml |  11 --
 guests/{lcitool => lcitool.py}| 209 +++---
 guests/playbooks/build/main.yml   |   2 +-
 guests/playbooks/update/main.yml  |   6 +-
 guests/playbooks/update/tasks/gitlab.yml  |   4 +-
 guests/playbooks/update/tasks/kludges.yml |   2 +-
 guests/playbooks/update/tasks/users.yml   |  42 ++---
 guests/requirements.txt   |   4 +
 guests/test_config.py | 165 +
 11 files changed, 353 insertions(+), 152 deletions(-)
 create mode 100644 guests/config.yaml
 delete mode 100644 guests/group_vars/all/install.yml
 rename guests/{lcitool => lcitool.py} (88%)
 create mode 100644 guests/requirements.txt
 create mode 100644 guests/test_config.py

-- 
2.25.3

[libvirt-ci PATCH v2 01/13] requirements: Introduce a requirements.txt file

[Erik Skultety]

Right now, lcitool's users have 2 options to satisfy the tool's
dependencies: install them through the system package manager
system-wide or install through pip manually.
Since pip gives us the option to automate this process a tiny bit, let's
ship a requirements file. This targets users who are used to work
with Python virtual environments and install whatever it is necessary
inside.

Signed-off-by: Erik Skultety 
Reviewed-by: Andrea Bolognani 
---
 guests/requirements.txt | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 guests/requirements.txt

diff --git a/guests/requirements.txt b/guests/requirements.txt
new file mode 100644
index 000..900cfd6
--- /dev/null
+++ b/guests/requirements.txt
@@ -0,0 +1,2 @@
+ansible
+jenkins-job-builder
-- 
2.25.3

Re: [libvirt PATCH] qemu: Support vmpvscsi controller model

[Chris Jester-Young]

On Mon, May 11, 2020 at 09:13:42AM +0200, Peter Krempa wrote:
> It would be great if you could describe that 'pvscsi' indeed is the
> vmware paravirtual scsi controller even in qemu, because I had to dig
> through the code to figure it out.

Sure, just in the commit message? Or elsewhere too?

> Can the 'pvscsi' device be compiled out using any upstream way? If not
> we don't really need a capability for it since qemu-1.5.3 is the oldest
> supported qemu and all other versions support it as well.

As Daniel P. Berrangé mentioned, this can be configured out via Kconfig.
Plus there are actually some files in tests/qemucapabilitiesdata that do
not advertise that device (e.g., *.s390x.replies).

> The remainder of the patch looks okay to me, but it's missing a
> qemuxml2argvtest case for the new controller. It looks like the
> controller is using PCI addressing so the rest should be okay.

Sure thing, I'll be happy to add a test. Do I just take one of the
existing tests and modify the XML and expected resultant command?

> If you figure out that the capability is required, I'd prefer if the
> addition of the capability is in a separate patch.

Can do. Given the (hopefully, given the patches from earlier today)
impending arrival of merge requests, and the fact that I already have
to redo the patch (since new capabilities have since been added), my
current plan is to just post the new commits in a merge request.

(Don't worry, I won't actually file the MR until the CI system is in
place.)

Cheers,
Chris.

Re: [libvirt PATCH] qemu: only stop external devices after the domain

[Daniel Henrique Barboza]

On 5/12/20 8:23 AM, Ján Tomko wrote:

A failure in qemuProcessLaunch would lead to qemuExtStopDevices
being called twice - once in the cleanup section and then again
in qemuProcessStop.

However, the first one is called while the QEMU process is
still running, which is too soon for the swtpm process, because
the swtmp_ioctl command can lock up::

https://bugzilla.redhat.com/show_bug.cgi?id=1822523

Remove the first call and only leave the one in qemuProcessStop,
which is called after the QEMU process is killed.

Signed-off-by: Ján Tomko 
---



Reviewed-by: Daniel Henrique Barboza 



  src/qemu/qemu_process.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dee3f3fb63..f7f6793113 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
  ret = 0;
  
   cleanup:

-if (ret < 0)
-qemuExtDevicesStop(driver, vm);
  qemuDomainSecretDestroy(vm);
  return ret;
  }

Re: [libvirt PATCH] qemu: only stop external devices after the domain

[Daniel Henrique Barboza]

On 5/12/20 10:48 AM, Daniel Henrique Barboza wrote:



On 5/12/20 8:23 AM, Ján Tomko wrote:

A failure in qemuProcessLaunch would lead to qemuExtStopDevices
being called twice - once in the cleanup section and then again
in qemuProcessStop.

However, the first one is called while the QEMU process is
still running, which is too soon for the swtpm process, because
the swtmp_ioctl command can lock up::

    https://bugzilla.redhat.com/show_bug.cgi?id=1822523

Remove the first call and only leave the one in qemuProcessStop,
which is called after the QEMU process is killed.

Signed-off-by: Ján Tomko 
---
  src/qemu/qemu_process.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dee3f3fb63..f7f6793113 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
  ret = 0;
   cleanup:
-    if (ret < 0)
-    qemuExtDevicesStop(driver, vm);



This change makes this code obsolete:


+++ b/src/qemu/qemu_process.c
@@ -6866,11 +6866,6 @@ qemuProcessLaunch(virConnectPtr conn,
  incoming != NULL) < 0)
  goto cleanup;

-    /* Security manager labeled all devices, therefore
- * if any operation from now on fails, we need to ask the caller to
- * restore labels.
- */
-    ret = -2;

  if (incoming && incoming->fd != -1) {


You can remove the 'ret' variable altogether because no one in 
qemuProcessLaunch() is
doing anything with it.



Nevermind, I misread what the code was doing. We need to differentiate the -1 
and -2
return codes on error.







Thanks,


DHB



  qemuDomainSecretDestroy(vm);
  return ret;
  }

Re: [libvirt PATCH] qemu: only stop external devices after the domain

[Daniel Henrique Barboza]

On 5/12/20 8:23 AM, Ján Tomko wrote:

A failure in qemuProcessLaunch would lead to qemuExtStopDevices
being called twice - once in the cleanup section and then again
in qemuProcessStop.

However, the first one is called while the QEMU process is
still running, which is too soon for the swtpm process, because
the swtmp_ioctl command can lock up::

https://bugzilla.redhat.com/show_bug.cgi?id=1822523

Remove the first call and only leave the one in qemuProcessStop,
which is called after the QEMU process is killed.

Signed-off-by: Ján Tomko 
---
  src/qemu/qemu_process.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dee3f3fb63..f7f6793113 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
  ret = 0;
  
   cleanup:

-if (ret < 0)
-qemuExtDevicesStop(driver, vm);



This change makes this code obsolete:


+++ b/src/qemu/qemu_process.c
@@ -6866,11 +6866,6 @@ qemuProcessLaunch(virConnectPtr conn,
 incoming != NULL) < 0)
 goto cleanup;
 
-/* Security manager labeled all devices, therefore

- * if any operation from now on fails, we need to ask the caller to
- * restore labels.
- */
-ret = -2;
 
 if (incoming && incoming->fd != -1) {



You can remove the 'ret' variable altogether because no one in 
qemuProcessLaunch() is
doing anything with it.



Thanks,


DHB



  qemuDomainSecretDestroy(vm);
  return ret;
  }

Re: [PATCH] security: do not log password

[Peter Krempa]

On Tue, May 12, 2020 at 21:06:14 +0800, Zhang Bo wrote:
> It's insecure to log password, nomatter the password is encrypted or
> not. And do not log it even in debug mode, in the consideration of
> resilience, surposing that the log mode has been modified by the
> attacker.

That is true ...

> 
> Signed-off-by: Zhang Bo 
> ---
>  src/libvirt-domain.c| 3 +--
>  src/qemu/qemu_monitor.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index a12809c2d5..e2a57c178b 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11340,8 +11340,7 @@ virDomainSetUserPassword(virDomainPtr dom,
>   const char *password,
>   unsigned int flags)
>  {
> -VIR_DOMAIN_DEBUG(dom, "user=%s, password=%s, flags=0x%x",
> - NULLSTR(user), NULLSTR(password), flags);

Yes, this is wrong.

> +VIR_DOMAIN_DEBUG(dom, "user=%s, flags=0x%x", NULLSTR(user), flags);
>  
>  virResetLastError();
>  
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 9c853ccb93..9bfaf53b65 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2241,8 +2241,7 @@ qemuMonitorSetPassword(qemuMonitorPtr mon,
>  if (!protocol)
>  return -1;
>  
> -VIR_DEBUG("protocol=%s, password=%p, action_if_connected=%s",
> -  protocol, password, action_if_connected);

Here we just log the pointer, so this is okay. There's now way to get
the password back from this. One could argue it's pointless to log the
pointer though.

Unfortunatley in debug mode we still log the full monitor command sent
to qemu, where you'll get the password.

I don't think there's a reasonable way to avoid it unless we use the
qcryptosecret interface of qemu which we use for storage auth and
encryption keys.

[PATCH] security: do not log password

[Zhang Bo]

It's insecure to log password, nomatter the password is encrypted or
not. And do not log it even in debug mode, in the consideration of
resilience, surposing that the log mode has been modified by the
attacker.

Signed-off-by: Zhang Bo 
---
 src/libvirt-domain.c| 3 +--
 src/qemu/qemu_monitor.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index a12809c2d5..e2a57c178b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11340,8 +11340,7 @@ virDomainSetUserPassword(virDomainPtr dom,
  const char *password,
  unsigned int flags)
 {
-VIR_DOMAIN_DEBUG(dom, "user=%s, password=%s, flags=0x%x",
- NULLSTR(user), NULLSTR(password), flags);
+VIR_DOMAIN_DEBUG(dom, "user=%s, flags=0x%x", NULLSTR(user), flags);
 
 virResetLastError();
 
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 9c853ccb93..9bfaf53b65 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2241,8 +2241,7 @@ qemuMonitorSetPassword(qemuMonitorPtr mon,
 if (!protocol)
 return -1;
 
-VIR_DEBUG("protocol=%s, password=%p, action_if_connected=%s",
-  protocol, password, action_if_connected);
+VIR_DEBUG("protocol=%s, action_if_connected=%s", protocol, 
action_if_connected);
 
 QEMU_CHECK_MONITOR(mon);
 
-- 
2.23.0.windows.1

[libvirt PATCH] qemu: only stop external devices after the domain

[Ján Tomko]

A failure in qemuProcessLaunch would lead to qemuExtStopDevices
being called twice - once in the cleanup section and then again
in qemuProcessStop.

However, the first one is called while the QEMU process is
still running, which is too soon for the swtpm process, because
the swtmp_ioctl command can lock up::

   https://bugzilla.redhat.com/show_bug.cgi?id=1822523

Remove the first call and only leave the one in qemuProcessStop,
which is called after the QEMU process is killed.

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_process.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dee3f3fb63..f7f6793113 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
 ret = 0;
 
  cleanup:
-if (ret < 0)
-qemuExtDevicesStop(driver, vm);
 qemuDomainSecretDestroy(vm);
 return ret;
 }
-- 
2.25.4

[libvirt-appdev-guide-python 2/2] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

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

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index 1b77b6f..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = libvirt-appdev-guide-python PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..c9b8afd
--- /dev/null
+++ b/CONTRIBUTING.rst
@@ -0,0 +1,28 @@
+==
+Contributing to libvirt-appdev-guide-python
+==
+
+The libvirt Python application developer guide accepts code contributions
+via merge requests on the GitLab project:
+
+https://gitlab.com/libvirt/libvirt-appdev-guide-python/-/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/libvirt-appdev-guide-python/pipelines
+
+CI pipeline results for merge requests will be visible via the contributors'
+own private repository fork:
+
+https://gitlab.com/yourusername/libvirt-appdev-guide-python/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.
-- 
2.26.2

[libvirt-appdev-guide-python 0/2] Enable use of GitLab CI and merge requests

[Daniel P . Berrangé]

This introduces support for GitLab CI, and then recommends use of merge
requests for contribution.

Daniel P. Berrangé (2):
  gitlab: introduce CI jobs for building content
  gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

 .gitlab-ci.yml   | 125 +++
 .gitpublish  |   4 -
 CONTRIBUTING.rst |  28 ++
 Makefile |   7 +-
 ci/libvirt-centos-7.Dockerfile   |  83 ++
 ci/libvirt-debian-10.Dockerfile  |  53 
 ci/libvirt-debian-9.Dockerfile   |  56 
 ci/libvirt-debian-sid.Dockerfile |  53 
 ci/libvirt-fedora-31.Dockerfile  |  50 +++
 ci/libvirt-fedora-32.Dockerfile  |  50 +++
 ci/libvirt-fedora-rawhide.Dockerfile |  51 +++
 ci/libvirt-ubuntu-1804.Dockerfile|  56 
 ci/libvirt-ubuntu-2004.Dockerfile|  53 
 ci/refresh   |  22 +
 14 files changed, 684 insertions(+), 7 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

-- 
2.26.2

[libvirt-appdev-guide-python 1/2] gitlab: introduce CI jobs for building content

[Daniel P . Berrangé]

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
publish the generated HTML docs, which then become browsable via
the GitLab Pages service.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml   | 125 +++
 Makefile |   7 +-
 ci/libvirt-centos-7.Dockerfile   |  83 ++
 ci/libvirt-debian-10.Dockerfile  |  53 
 ci/libvirt-debian-9.Dockerfile   |  56 
 ci/libvirt-debian-sid.Dockerfile |  53 
 ci/libvirt-fedora-31.Dockerfile  |  50 +++
 ci/libvirt-fedora-32.Dockerfile  |  50 +++
 ci/libvirt-fedora-rawhide.Dockerfile |  51 +++
 ci/libvirt-ubuntu-1804.Dockerfile|  56 
 ci/libvirt-ubuntu-2004.Dockerfile|  53 
 ci/refresh   |  22 +
 12 files changed, 656 insertions(+), 3 deletions(-)
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..5b7f947 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,35 @@
 
 stages:
   - prebuild
+  - containers
+  - docs
+
+.container_job_template: _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/libvirt-appdev-guide-python/ci-$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.docs_job_template: _job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: docs
+  before_script:
+- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+  script:
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt-publican.git brand
+- $MAKE branddir=$PWD/brand
+- mv tmp/en-US/html public
 
 # Check that all commits are signed-off for the DCO.
 # Skip on "libvirt" namespace, since we only need to run
@@ -14,3 +43,99 @@ check-dco:
   except:
 variables:
   - $CI_PROJECT_NAMESPACE == 'libvirt'
+
+centos-7-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-7
+
+debian-9-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-9
+
+debian-10-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-10
+
+debian-sid-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-sid
+
+fedora-31-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-rawhide
+
+ubuntu-1804-container:
+  <<: *container_job_definition
+  variables:
+NAME: ubuntu-1804
+
+ubuntu-2004-container:
+  <<: *container_job_definition
+  variables:
+NAME: ubuntu-2004
+
+
+# centos-7-docs is special as it is the one
+# we publish from
+pages:
+  <<: *docs_job_definition
+  variables:
+NAME: centos-7
+  artifacts:
+paths:
+  - public
+
+debian-9-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: debian-9
+
+debian-10-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: debian-10
+
+debian-sid-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: debian-sid
+
+fedora-31-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: fedora-rawhide
+
+ubuntu-1804-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: ubuntu-1804
+
+ubuntu-2004-docs:
+  <<: *docs_job_definition
+  variables:
+NAME: ubuntu-2004
diff --git a/Makefile b/Makefile
index 85935a1..237961a 100644
--- a/Makefile
+++ b/Makefile
@@ -3,17 +3,18 @@ prefix=/usr
 datadir=$(prefix)/share
 pkgdatadir=$(datadir)/publican
 contentdir=$(pkgdatadir)/Common_Content
+branddir=$(contentdir)
 
 all: html pdf
 
 html:
-   publican build --langs=en-US --formats=html 

[libvirt-csharp 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

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

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index 7c4687f..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = libvirt-csharp PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..4cbdb57
--- /dev/null
+++ b/CONTRIBUTING.rst
@@ -0,0 +1,28 @@
+==
+Contributing to libvirt-csharp
+==
+
+The libvirt Csharp API binding accepts code contributions via merge requests
+on the GitLab project:
+
+https://gitlab.com/libvirt/libvirt-csharp/-/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/libvirt-csharp/pipelines
+
+CI pipeline results for merge requests will be visible via the contributors'
+own private repository fork:
+
+https://gitlab.com/yourusername/libvirt-csharp/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.
-- 
2.26.2

[libvirt-csharp 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

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

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..9047fc3 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,55 @@
 
 stages:
   - prebuild
+  - containers
+  - builds
+
+.container_job_template: _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/libvirt-csharp/ci-$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.git_build_job_template: _build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  before_script:
+- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+- export SCRATCH_DIR="/tmp/scratch"
+- export VROOT="$SCRATCH_DIR/vroot"
+- export LD_LIBRARY_PATH="$VROOT/lib"
+- export PATH="$VROOT/bin:$PATH"
+- export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig"
+  script:
+- pushd "$PWD"
+- mkdir -p "$SCRATCH_DIR"
+- cd "$SCRATCH_DIR"
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git
+- mkdir libvirt/build
+- cd libvirt/build
+- ../autogen.sh --prefix="$VROOT" --without-libvirtd
+- $MAKE install
+- popd
+- mdtool build projects/MonoDevelop/LibvirtBindings.csproj
+
+.dist_build_job_template: _build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  before_script:
+- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+  script:
+- mdtool build projects/MonoDevelop/LibvirtBindings.csproj
 
 # Check that all commits are signed-off for the DCO.
 # Skip on "libvirt" namespace, since we only need to run
@@ -14,3 +63,50 @@ check-dco:
   except:
 variables:
   - $CI_PROJECT_NAMESPACE == 'libvirt'
+
+debian-9-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-9
+
+fedora-31-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-rawhide
+
+
+fedora-32-git-build:
+  <<: *git_build_job_definition
+  variables:
+NAME: fedora-32
+
+
+debian-9-dist-build:
+  <<: *dist_build_job_definition
+  variables:
+NAME: debian-9
+
+fedora-31-dist-build:
+  <<: *dist_build_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-dist-build:
+  <<: *dist_build_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-dist-build:
+  <<: *dist_build_job_definition
+  variables:
+NAME: fedora-rawhide
diff --git a/ci/README.rst b/ci/README.rst
new file mode 100644
index 000..530897e
--- /dev/null
+++ b/ci/README.rst
@@ -0,0 +1,14 @@
+CI job assets
+=
+
+This directory contains assets used in the automated CI jobs, most
+notably the Dockerfiles used to build container images in which the
+CI jobs then run.
+
+The ``refresh`` script is used to re-create the Dockerfiles using the
+``lcitool`` command that is provided by repo
+https://gitlab.com/libvirt/libvirt-ci
+
+The containers are built during the CI process and cached in the GitLab
+container registry of the project doing the build. The cached containers
+can be deleted at any time and will be correctly rebuilt.
diff --git a/ci/libvirt-debian-9.Dockerfile b/ci/libvirt-debian-9.Dockerfile

[libvirt-csharp 1/3] Update to target newer 4.0 .NET framework version

[Daniel P . Berrangé]

The 3.5 .NET version is no longer supported by the mono / monodevelop
toolchain.

Signed-off-by: Daniel P. Berrangé 
---
 .../virConnectOpen/virConnectOpen.csproj  |  4 +-
 .../virConnectOpenAuth.csproj |  4 +-
 .../virConnectSetErrorFunc.csproj |  4 +-
 .../virDomainStats/virDomainStats.csproj  | 57 ++-
 .../virEventRegisterImpl.csproj   |  4 +-
 projects/MonoDevelop/LibvirtBindings.csproj   |  6 +-
 projects/MonoDevelop/LibvirtBindings.sln  |  6 +-
 7 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/examples/MonoDevelop/virConnectOpen/virConnectOpen.csproj 
b/examples/MonoDevelop/virConnectOpen/virConnectOpen.csproj
index e0bbb1b..131bbfc 100644
--- a/examples/MonoDevelop/virConnectOpen/virConnectOpen.csproj
+++ b/examples/MonoDevelop/virConnectOpen/virConnectOpen.csproj
@@ -1,10 +1,8 @@
 
-http://schemas.microsoft.com/developer/msbuild/2003;>
+http://schemas.microsoft.com/developer/msbuild/2003;>
   
 Debug
 AnyCPU
-9.0.21022
-2.0
 {FFCD939E-7F9A-44D5-AEBC-84F40942E8B5}
 WinExe
 virConnectOpen
diff --git a/examples/MonoDevelop/virConnectOpenAuth/virConnectOpenAuth.csproj 
b/examples/MonoDevelop/virConnectOpenAuth/virConnectOpenAuth.csproj
index 32b20d4..4e05439 100644
--- a/examples/MonoDevelop/virConnectOpenAuth/virConnectOpenAuth.csproj
+++ b/examples/MonoDevelop/virConnectOpenAuth/virConnectOpenAuth.csproj
@@ -1,10 +1,8 @@
 
-http://schemas.microsoft.com/developer/msbuild/2003;>
+http://schemas.microsoft.com/developer/msbuild/2003;>
   
 Debug
 AnyCPU
-9.0.21022
-2.0
 {3259AE36-B12F-435E-9124-F6CAA781AD5C}
 WinExe
 virConnectOpenAuth
diff --git 
a/examples/MonoDevelop/virConnectSetErrorFunc/virConnectSetErrorFunc.csproj 
b/examples/MonoDevelop/virConnectSetErrorFunc/virConnectSetErrorFunc.csproj
index 2a77393..f5e6254 100644
--- a/examples/MonoDevelop/virConnectSetErrorFunc/virConnectSetErrorFunc.csproj
+++ b/examples/MonoDevelop/virConnectSetErrorFunc/virConnectSetErrorFunc.csproj
@@ -1,10 +1,8 @@
 
-http://schemas.microsoft.com/developer/msbuild/2003;>
+http://schemas.microsoft.com/developer/msbuild/2003;>
   
 Debug
 AnyCPU
-9.0.21022
-2.0
 {243DD685-9AB3-4CD0-93D5-92034C1D97D8}
 WinExe
 virConnectSetErrorFunc
diff --git a/examples/MonoDevelop/virDomainStats/virDomainStats.csproj 
b/examples/MonoDevelop/virDomainStats/virDomainStats.csproj
index 355d8ef..05b2ecb 100644
--- a/examples/MonoDevelop/virDomainStats/virDomainStats.csproj
+++ b/examples/MonoDevelop/virDomainStats/virDomainStats.csproj
@@ -1 +1,56 @@
-
http://schemas.microsoft.com/developer/msbuild/2003;>
  
Debug
AnyCPU
9.0.21022
2.0
{767373FC-96BE-420A-8219-97146D33B2CB}
WinExe
virDomainStats
virDomainStats
v3.5
  
  
true
full
false
bin\Debug
DEBUG
prompt
4
false
  
  
none
false
bin\Release
prompt
4
false
  
  











  
  

  gui.stetic

  
  





  
  

  {C51C70EB-9040-4F8E-9A18-DF2A77D04A37}
  LibvirtBindings

  
  

\ No newline at end of file
+
+http://schemas.microsoft.com/developer/msbuild/2003;>
+  
+Debug
+AnyCPU
+{767373FC-96BE-420A-8219-97146D33B2CB}
+WinExe
+virDomainStats
+virDomainStats
+v3.5
+  
+  
+true
+full
+false
+bin\Debug
+DEBUG
+prompt
+4
+false
+  
+  
+none
+false
+bin\Release
+prompt
+4
+false
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+  
+  
+
+  gui.stetic
+
+  
+  
+
+
+
+
+
+  
+  
+
\ No newline at end of file
diff --git 
a/examples/MonoDevelop/virEventRegisterImpl/virEventRegisterImpl.csproj 
b/examples/MonoDevelop/virEventRegisterImpl/virEventRegisterImpl.csproj
index 5165c88..858b89c 100644
--- a/examples/MonoDevelop/virEventRegisterImpl/virEventRegisterImpl.csproj
+++ b/examples/MonoDevelop/virEventRegisterImpl/virEventRegisterImpl.csproj
@@ -1,10 +1,8 @@
 
-http://schemas.microsoft.com/developer/msbuild/2003;>
+http://schemas.microsoft.com/developer/msbuild/2003;>
   
 Debug
 AnyCPU
-9.0.21022
-2.0
 {4576BB61-F143-4BC8-BD1D-D50F710CEA10}
 WinExe
 virEventRegisterImpl
diff --git a/projects/MonoDevelop/LibvirtBindings.csproj 
b/projects/MonoDevelop/LibvirtBindings.csproj
index 2473625..3a35541 100644
--- a/projects/MonoDevelop/LibvirtBindings.csproj
+++ b/projects/MonoDevelop/LibvirtBindings.csproj
@@ -1,15 +1,13 @@
 
-http://schemas.microsoft.com/developer/msbuild/2003;>
+http://schemas.microsoft.com/developer/msbuild/2003;>
   
 Debug
 AnyCPU
-9.0.21022
-2.0
 {C51C70EB-9040-4F8E-9A18-DF2A77D04A37}
 Library
 LibvirtBindings
 LibvirtBindings
-v3.5
+v4.5
   
   
 true
diff --git a/projects/MonoDevelop/LibvirtBindings.sln 

[libvirt-csharp 0/3] Enable use of GitLab CI and merge requests

[Daniel P . Berrangé]

This introduces support for GitLab CI, and then recommends use of merge
requests for contribution.

Daniel P. Berrangé (3):
  Update to target newer 4.0 .NET framework version
  gitlab: introduce CI jobs testing git master & distro libvirt
  gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

 .gitlab-ci.yml| 96 +++
 .gitpublish   |  4 -
 CONTRIBUTING.rst  | 28 ++
 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 ++
 .../virConnectOpen/virConnectOpen.csproj  |  4 +-
 .../virConnectOpenAuth.csproj |  4 +-
 .../virConnectSetErrorFunc.csproj |  4 +-
 .../virDomainStats/virDomainStats.csproj  | 57 ++-
 .../virEventRegisterImpl.csproj   |  4 +-
 projects/MonoDevelop/LibvirtBindings.csproj   |  6 +-
 projects/MonoDevelop/LibvirtBindings.sln  |  6 +-
 16 files changed, 454 insertions(+), 24 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst
 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

-- 
2.26.2

[ruby-libvirt 1/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

The ruby build needs to validate two axis

 - A variety of libvirt versions
 - A variety of ruby 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 CentOS 8 is picked as a stable long life
base.

For unknown reasons  "rake package" fails on OpenSUSE Leap 15, but it
appears to be a bug in the old version of rake, so we skip that part
of the build.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml   | 168 +++
 ci/README.rst|  14 +++
 ci/libvirt-centos-7.Dockerfile   |  86 ++
 ci/libvirt-centos-8.Dockerfile   |  64 ++
 ci/libvirt-debian-10.Dockerfile  |  56 +
 ci/libvirt-debian-9.Dockerfile   |  59 ++
 ci/libvirt-debian-sid.Dockerfile |  56 +
 ci/libvirt-fedora-31.Dockerfile  |  53 +
 ci/libvirt-fedora-32.Dockerfile  |  53 +
 ci/libvirt-fedora-rawhide.Dockerfile |  54 +
 ci/libvirt-opensuse-151.Dockerfile   |  55 +
 ci/libvirt-ubuntu-1804.Dockerfile|  59 ++
 ci/libvirt-ubuntu-2004.Dockerfile|  56 +
 ci/refresh   |  27 +
 14 files changed, 860 insertions(+)
 create mode 100644 ci/README.rst
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-centos-8.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..0ca4bb9 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,57 @@
 
 stages:
   - prebuild
+  - containers
+  - builds
+
+.container_job_template: _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"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.git_build_job_template: _build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  before_script:
+- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+- export SCRATCH_DIR="/tmp/scratch"
+- export VROOT="$SCRATCH_DIR/vroot"
+- export LD_LIBRARY_PATH="$VROOT/lib"
+- export PATH="$VROOT/bin:$PATH"
+- export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig"
+  script:
+- pushd "$PWD"
+- mkdir -p "$SCRATCH_DIR"
+- cd "$SCRATCH_DIR"
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git
+- mkdir libvirt/build
+- cd libvirt/build
+- ../autogen.sh --prefix="$VROOT" --without-libvirtd
+- $MAKE install
+- popd
+- rake build
+- if test "$NAME" != "opensuse-151" ; then rake package; fi
+
+.dist_build_job_template: _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
 
 # Check that all commits are signed-off for the DCO.
 # Skip on "libvirt" namespace, since we only need to run
@@ -14,3 +65,120 @@ check-dco:
   except:
 variables:
   - $CI_PROJECT_NAMESPACE == 'libvirt'
+
+centos-7-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-7
+
+centos-8-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-8
+
+debian-9-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-9
+
+debian-10-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-10
+
+debian-sid-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-sid
+
+fedora-31-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-rawhide
+
+opensuse-151-container:
+  <<: *container_job_definition
+  variables:
+NAME: opensuse-151
+

[ruby-libvirt 2/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

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

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index 8e6d748..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = ruby PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..926f59e
--- /dev/null
+++ 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.
-- 
2.26.2

[ruby-libvirt 3/3] Remove obsolete mercurial ignore file

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 .hgignore | 7 ---
 1 file changed, 7 deletions(-)
 delete mode 100644 .hgignore

diff --git a/.hgignore b/.hgignore
deleted file mode 100644
index 8158c7f..000
--- a/.hgignore
+++ /dev/null
@@ -1,7 +0,0 @@
-~$
-.o$
-.so$
-Makefile$
-^pkg/
-^ext/libvirt/mkmf.log$
-^doc/site/api
-- 
2.26.2

[ruby-libvirt 0/3] Add support for GitLab CI and merge requests

[Daniel P . Berrangé]

This introduces support for GitLab CI, and then recommends use of merge
requests for contribution.

Daniel P. Berrangé (3):
  gitlab: introduce CI jobs testing git master & distro libvirt
  gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
  Remove obsolete mercurial ignore file

 .gitlab-ci.yml   | 168 +++
 .gitpublish  |   4 -
 .hgignore|   7 --
 CONTRIBUTING.rst |  28 +
 ci/README.rst|  14 +++
 ci/libvirt-centos-7.Dockerfile   |  86 ++
 ci/libvirt-centos-8.Dockerfile   |  64 ++
 ci/libvirt-debian-10.Dockerfile  |  56 +
 ci/libvirt-debian-9.Dockerfile   |  59 ++
 ci/libvirt-debian-sid.Dockerfile |  56 +
 ci/libvirt-fedora-31.Dockerfile  |  53 +
 ci/libvirt-fedora-32.Dockerfile  |  53 +
 ci/libvirt-fedora-rawhide.Dockerfile |  54 +
 ci/libvirt-opensuse-151.Dockerfile   |  55 +
 ci/libvirt-ubuntu-1804.Dockerfile|  59 ++
 ci/libvirt-ubuntu-2004.Dockerfile|  56 +
 ci/refresh   |  27 +
 17 files changed, 888 insertions(+), 11 deletions(-)
 delete mode 100644 .gitpublish
 delete mode 100644 .hgignore
 create mode 100644 CONTRIBUTING.rst
 create mode 100644 ci/README.rst
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-centos-8.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

-- 
2.26.2

Re: [PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390

[Boris Fiuczynski]

On 5/7/20 5:51 PM, Laine Stump wrote:

On 5/6/20 1:48 PM, Andrea Bolognani wrote:

On Mon, 2020-04-20 at 21:55 +0200, Boris Fiuczynski wrote:

On 4/10/20 2:06 PM, Andrea Bolognani wrote:

On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:

The ZPCI address validation or autogeneration does not work as
expected in the following scenarios
1. uid = 0 and fid = 0
2. uid = 0 and fid > 0
3. uid = 0 and fid not specified
4. uid not specified and fid > 0
5. 2 ZPCI devices with uid > 0 and fid not specified.

This is because of the following reasons
1. If uid = 0 or fid = 0 the code assumes that user has not specified
 the corresponding address
2. If either uid or fid is provided, the code assumes that both uid
 and fid addresses are specified by the user.


I'd have to dig up the old threads, but based on what I remember the
behaviors you describe are entirely intentional.

For PCI addresses, setting all parts of the address to zero or not
setting it at all is equivalent, and we wanted to be consistent with
that behavior for ZPCI; additionally, zero is not a valid value for
uid so of course neither is the address uid=0 fid=0, which means that
we're not preventing the user from specifying a valid address by
conflating the all-zero address with the unspecified address.

For partially-specified addresses, the behavior is also the same as
PCI: any part you don't specify is considered to be zero, which
results in

    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
  fid=x -> uid=0 fid=x -> address is rejected as invalid
    uid=x   -> uid=x fid=0 -> address is accepted

So, just like for PCI addresses, you have basically two reasonable
options: either don't specify any zPCI address and leave allocation
entirely up to libvirt, or specify all of the addresses completely:
anything in between will likely not work as you'd expect or want.

Again, this is based purely on my recollection of design discussions
that happened one and a half years ago, so I might have gotten some
of the details wrong - in which case by all means call me out on
that O:-)


Hi Andrea,
sorry for the delayed answer. I (and some others as well) lost some
emails on my IMAP account and I just found your answer today.


No apologies needed: I also took a long time to reply to your
message, and in my case there's no mail server malfunction that I
can assign the blame to O:-)


I can remember that you had a discussion with the original author of the
zpci code. There are a few issues with the currently implemented "rules"
which partially are not even working as you outlined above in more
complex scenarios.


I disagree with this assessment - they work exactly as designed and
as described above. Whether we *want* them to behave that way... Now
that's a different topic :)

I think the disconnect lies in what the user's expectations are and
what libvirt actually implements. Basically the user expects that

   * if either one of uid and fid is explictly assigned a value by
 the user, then the guest will use that value - unless such value
 is invalid, in which case libvirt will report an error;

   * if either one of uid and fid is absent from the user-provided
 configuration, then libvirt will automatically pick a valid value
 for the attribute.

This is not how the current zPCI implementation works, or how PCI
address assignment works in libvirt; on the other hand, I think these
expectations are in fact completely reasonable, as the examples you
provide illustrate quite well.

I think you successfully convinced me that the current approach is
not good for users and we should fix it; my only doubt at this point
is whether can we safely do that without breaking libvirt's backwards
compatibility guarantees.

Dan, Laine, what's your take?


It sounds like the same semantics were applied to the zPCI address 
element as are applied to  (if I understand 
correctly, while an PCI address is considered "valid and complete" if 
any of its attributes is non-0, for the zPCI extensions, each attribute 
is taken on its own. Is that correct?


Yes, that is correct.
uid and fid can be set independently from each other within their ranges.
uid (a hex value between 0x0001 and 0x, inclusive)
fid (a hex value between 0x and 0x, inclusive)
Both must be unique within a domain.



In any case, it sounds like current behavior for zPCI is broken for some 
use cases, and imo this is new enough and usage is narrow enough that if 
the maintainers (who I would guess represent the actual users) think 
fixing the bug is more important than 100% backward compatibility in a 
corner case, then they should be able to change it.


I would like to get this fixed such that the behavior is architecture 
compliant.

The behavior change would be
Current code:
 uid=0 fid=0 -> uid=0 fid=0 -> 

[libvirt-java PATCH 3/6] Remove reference to test.java file that was deleted

[Daniel P . Berrangé]

This was removed in

  commit 2cdc96c2a6ce83ea0605ad81b2e542f0daafa805
  Author: Claudio Bley 
  Date:   Wed Jan 22 16:45:14 2014 +0100

tests: remove obsolete test driver

JUnit is used for quite some time now, which supercedes the tests
defined in the old "test" class.

Signed-off-by: Daniel P. Berrangé 
---
 README.in| 16 ++--
 libvirt-java.spec.in |  2 --
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/README.in b/README.in
index eb27c3b..34dca65 100644
--- a/README.in
+++ b/README.in
@@ -13,19 +13,7 @@ By default the installs it to /usr/share/java/@version@.jar
 
 4. You can run the unit tests with ant test.
 
-5. There is a rudimentary functional test program that the libvirt-java-devel
-installs put it into /usr/share/doc/libvirt-java-devel-@version@/test.java
+5. There is full javadoc for the API in 
/usr/share/javadoc/libvirt-java-@version@/
 
-To run it, first copy the test.java file to writeable directory
-cp /usr/share/doc/libvirt-java-devel-@version@/test.java ~
-
-Compile the java file to a class:
-javac -classpath /usr/share/java/libvirt-@version@.jar test.java
-
-Then run the program:
-java -classpath 
.:/usr/share/java/jna.jar:/usr/share/java/libvirt-@version@.jar test
-
-6. There is full javadoc for the API in 
/usr/share/javadoc/libvirt-java-@version@/
-
-7. The full API can be found at 
http://www.libvirt.org/html/libvirt-libvirt.html
+6. The full API can be found at 
http://www.libvirt.org/html/libvirt-libvirt.html
 
diff --git a/libvirt-java.spec.in b/libvirt-java.spec.in
index d07b998..034d511 100644
--- a/libvirt-java.spec.in
+++ b/libvirt-java.spec.in
@@ -80,8 +80,6 @@ rm -rf %{buildroot}
 
 %files devel
 %defattr(-,root,root)
-%doc src/test/java/test.java
-
 
 %files javadoc
 %defattr(-,root,root)
-- 
2.26.2

[libvirt-java PATCH 4/6] rpm: skip junit tests in RHEL-8 build

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 libvirt-java.spec.in | 4 
 1 file changed, 4 insertions(+)

diff --git a/libvirt-java.spec.in b/libvirt-java.spec.in
index 034d511..1d7fa43 100644
--- a/libvirt-java.spec.in
+++ b/libvirt-java.spec.in
@@ -20,7 +20,9 @@ Requires:   java >= @java.required@
 Requires:   jpackage-utils
 BuildRequires:  ant
 BuildRequires:  jna
+%if 0%{?rhel} != 8
 BuildRequires:  ant-junit
+%endif
 BuildRequires:  java-devel >= @java.required@
 BuildRequires:  jpackage-utils
 
@@ -68,7 +70,9 @@ cp -r target/javadoc/* 
%{buildroot}%{_javadocdir}/%{name}-%{version}
 %{__ln_s} %{_javadocdir}/%{name}-%{version} %{buildroot}%{_javadocdir}/%{name}
 
 %check
+%if 0%{?rhel} != 8
 ant test
+%endif
 
 %clean
 rm -rf %{buildroot}
-- 
2.26.2

[libvirt-java PATCH 1/6] Fix test driver connection type

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/test/java/org/libvirt/TestJavaBindings.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/java/org/libvirt/TestJavaBindings.java 
b/src/test/java/org/libvirt/TestJavaBindings.java
index 88ad1ba..4b55a13 100644
--- a/src/test/java/org/libvirt/TestJavaBindings.java
+++ b/src/test/java/org/libvirt/TestJavaBindings.java
@@ -51,7 +51,7 @@ public final class TestJavaBindings extends TestCase {
 }
 
 public void testConnection() throws Exception {
-assertEquals("conn.getType()", "Test", conn.getType());
+assertEquals("conn.getType()", "TEST", conn.getType());
 assertEquals("conn.getURI()", "test:///default", conn.getURI());
 assertEquals("conn.getMaxVcpus(xen)", 32, conn.getMaxVcpus("xen"));
 assertNotNull("conn.getHostName()", conn.getHostName());
-- 
2.26.2

[libvirt-java PATCH 5/6] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

The java build needs to validate two axis

 - A variety of libvirt versions
 - A variety of java 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. CentOS 8 is unable to
run tests, since it lacks ant-junit packages.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml   | 170 +++
 ci/README.rst|  14 +++
 ci/libvirt-centos-7.Dockerfile   |  88 ++
 ci/libvirt-centos-8.Dockerfile   |  55 +
 ci/libvirt-debian-10.Dockerfile  |  58 +
 ci/libvirt-debian-9.Dockerfile   |  61 ++
 ci/libvirt-debian-sid.Dockerfile |  58 +
 ci/libvirt-fedora-31.Dockerfile  |  55 +
 ci/libvirt-fedora-32.Dockerfile  |  64 ++
 ci/libvirt-fedora-rawhide.Dockerfile |  56 +
 ci/libvirt-opensuse-151.Dockerfile   |  57 +
 ci/libvirt-ubuntu-1804.Dockerfile|  61 ++
 ci/libvirt-ubuntu-2004.Dockerfile|  58 +
 ci/refresh   |  27 +
 14 files changed, 882 insertions(+)
 create mode 100644 ci/README.rst
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-centos-8.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..cb133bc 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,59 @@
 
 stages:
   - prebuild
+  - containers
+  - builds
+
+.container_job_template: _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/libvirt-java/ci-$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.git_build_job_template: _build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  before_script:
+- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+- export SCRATCH_DIR="/tmp/scratch"
+- export VROOT="$SCRATCH_DIR/vroot"
+- export LD_LIBRARY_PATH="$VROOT/lib"
+- export PATH="$VROOT/bin:$PATH"
+- export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig"
+  script:
+- pushd "$PWD"
+- mkdir -p "$SCRATCH_DIR"
+- cd "$SCRATCH_DIR"
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git
+- mkdir libvirt/build
+- cd libvirt/build
+- ../autogen.sh --prefix="$VROOT" --without-libvirtd
+- $MAKE install
+- popd
+- ant build jar docs
+- if test "$NAME" != "centos-8" ; then ant test ; fi
+- ant src
+- if test -x /usr/bin/rpmbuild ; then ant spec && rpmbuild -ba --define 
"_sourcedir `pwd`/target" target/libvirt-java.spec ; fi
+
+.dist_build_job_template: _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
+- ant src
+- if test -x /usr/bin/rpmbuild ; then ant spec && rpmbuild -ba --define 
"_sourcedir `pwd`/target" target/libvirt-java.spec ; fi
 
 # Check that all commits are signed-off for the DCO.
 # Skip on "libvirt" namespace, since we only need to run
@@ -14,3 +67,120 @@ check-dco:
   except:
 variables:
   - $CI_PROJECT_NAMESPACE == 'libvirt'
+
+centos-7-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-7
+
+centos-8-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-8
+
+debian-9-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-9
+
+debian-10-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-10
+
+debian-sid-container:
+  <<: *container_job_definition
+  variables:
+NAME: debian-sid
+
+fedora-31-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-31
+
+fedora-32-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-32
+
+fedora-rawhide-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-rawhide
+

[libvirt-java PATCH 6/6] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

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

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index 1920230..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = libvirt-java PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..4ef23bd
--- /dev/null
+++ b/CONTRIBUTING.rst
@@ -0,0 +1,28 @@
+
+Contributing to libvirt-java
+
+
+The libvirt Java API binding accepts code contributions via merge requests
+on the GitLab project:
+
+https://gitlab.com/libvirt/libvirt-java/-/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/libvirt-java/pipelines
+
+CI pipeline results for merge requests will be visible via the contributors'
+own private repository fork:
+
+https://gitlab.com/yourusername/libvirt-java/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.
-- 
2.26.2

[libvirt-java PATCH 0/6] Enable GitLab CI and merge requests

[Daniel P . Berrangé]

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
  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

 .gitlab-ci.yml| 170 ++
 .gitpublish   |   4 -
 CONTRIBUTING.rst  |  28 +++
 README.in |  16 +-
 ci/README.rst |  14 ++
 ci/libvirt-centos-7.Dockerfile|  88 +
 ci/libvirt-centos-8.Dockerfile|  55 ++
 ci/libvirt-debian-10.Dockerfile   |  58 ++
 ci/libvirt-debian-9.Dockerfile|  61 +++
 ci/libvirt-debian-sid.Dockerfile  |  58 ++
 ci/libvirt-fedora-31.Dockerfile   |  55 ++
 ci/libvirt-fedora-32.Dockerfile   |  64 +++
 ci/libvirt-fedora-rawhide.Dockerfile  |  56 ++
 ci/libvirt-opensuse-151.Dockerfile|  57 ++
 ci/libvirt-ubuntu-1804.Dockerfile |  61 +++
 ci/libvirt-ubuntu-2004.Dockerfile |  58 ++
 ci/refresh|  27 +++
 libvirt-java.spec.in  |   6 +-
 .../java/org/libvirt/TestJavaBindings.java|  13 +-
 19 files changed, 927 insertions(+), 22 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst
 create mode 100644 ci/README.rst
 create mode 100644 ci/libvirt-centos-7.Dockerfile
 create mode 100644 ci/libvirt-centos-8.Dockerfile
 create mode 100644 ci/libvirt-debian-10.Dockerfile
 create mode 100644 ci/libvirt-debian-9.Dockerfile
 create mode 100644 ci/libvirt-debian-sid.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 100644 ci/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/refresh

-- 
2.26.2

[libvirt-java PATCH 2/6] Add workaround for broken screenshot API on Ubuntu 18.04 vintage

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/test/java/org/libvirt/TestJavaBindings.java | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/test/java/org/libvirt/TestJavaBindings.java 
b/src/test/java/org/libvirt/TestJavaBindings.java
index 4b55a13..0a3bca0 100644
--- a/src/test/java/org/libvirt/TestJavaBindings.java
+++ b/src/test/java/org/libvirt/TestJavaBindings.java
@@ -325,7 +325,16 @@ public final class TestJavaBindings extends TestCase {
 
 assertFalse("Domain \"test\" not found", dom == null);
 
-String mimetype = dom.screenshot(str, 0);
+String mimetype;
+try {
+mimetype = dom.screenshot(str, 0);
+} catch (LibvirtException ex) {
+if (ex.getMessage().contains("test-screenshot.png': No such file 
or directory")) {
+System.err.format("testDomainScreenshot skipped (missing png 
file)");
+return;
+}
+throw ex;
+}
 
 ByteBuffer bb = ByteBuffer.allocateDirect(8192);
 
-- 
2.26.2

[libvirt PATCH] tools: mention srve-update-tls supports virtproxyd

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 docs/manpages/virt-admin.rst | 3 ++-
 tools/virt-admin.c   | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/manpages/virt-admin.rst b/docs/manpages/virt-admin.rst
index 3e0d127790..d5cb23a1c5 100644
--- a/docs/manpages/virt-admin.rst
+++ b/docs/manpages/virt-admin.rst
@@ -455,7 +455,8 @@ Update tls context on *server*.
 
 - *server*
 
-  Available servers on a daemon. Currently only supports 'libvirtd'.
+  Available servers on a daemon. Currently only supports 'libvirtd' or
+  'virtproxyd'.
 
 
 CLIENT COMMANDS
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index a8e5e0a5af..6ff9729f66 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -978,7 +978,7 @@ static const vshCmdOptDef opts_srv_update_tls_file[] = {
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
  .help = N_("Available servers on a daemon. "
-"Currently only supports 'libvirtd'.")
+"Currently only supports 'libvirtd' or 'virtproxyd'.")
 },
 {.name = NULL}
 };
-- 
2.26.2

Re: [PATCH 3/6] qemu: check if AMD secure guest support is enabled

[Brijesh Singh]

Thanks for the patch Paulo, Few comments.

On 5/11/20 11:41 AM, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti 
>
> Implement secure guest check for AMD SEV (Secure Encrypted
> Virtualization) in order to invalidate the qemu capabilities
> cache in case the availability of the feature changed.
>
> For AMD SEV the verification consists of:
> - checking if /sys/module/kvm_amd/parameters/sev contains the
> value '1': meaning SEV is enabled in the host kernel;
> - checking if the kernel cmdline contains 'mem_encrypt=on': meaning
> SME memory encryption feature on the host is enabled


In addition to the kernel module parameter, we probably also need to
check whether QEMU supports the feature. e.g, what if user has newer
kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12.  To check the
SEV feature support, we should verify the following conditions:

1) check kernel module parameter is set

2) check if /dev/sev device exist (aka firmware is detected)

3) Check if Qemu supports SEV feature (maybe we can detect the existence
of the query-sev QMP command or detect Qemu version >= 2.12)

thanks

> Signed-off-by: Paulo de Rezende Pinatti 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  docs/kbase/launch_security_sev.rst |  2 +-
>  src/qemu/qemu_capabilities.c   | 27 +++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/docs/kbase/launch_security_sev.rst 
> b/docs/kbase/launch_security_sev.rst
> index 65f258587d..fa602c7432 100644
> --- a/docs/kbase/launch_security_sev.rst
> +++ b/docs/kbase/launch_security_sev.rst
> @@ -109,7 +109,7 @@ following:
>   
> 
>  
> -Note that if libvirt was already installed and libvirtd running before
> +Note that if libvirt (<6.4.0) was already installed and libvirtd running 
> before
>  enabling SEV in the kernel followed by the host reboot you need to force
>  libvirtd to re-probe both the host and QEMU capabilities. First stop
>  libvirtd:
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2874bb1e7c..6cf926d52d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
>  }
>  
>  
> +/*
> + * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
> + */
> +static bool
> +virQEMUCapsKVMSupportsSecureGuestAMD(void)
> +{
> +g_autofree char *cmdline = NULL;
> +g_autofree char *modValue = NULL;
> +static const char *kValues[] = {"on"};
> +
> +if (virFileReadValueString(, 
> "/sys/module/kvm_amd/parameters/sev") < 0)
> +return false;
> +if (modValue[0] != '1')
> +return false;
> +if (virFileReadValueString(, "/proc/cmdline") < 0)
> +return false;
> +if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
> +   G_N_ELEMENTS(kValues),
> +   VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
> +   VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
> +return true;
> +return false;
> +}
> +
> +
>  /*
>   * Check whether the secure guest functionality is enabled.
>   * See the specific architecture function for details on the verifications 
> made.
> @@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
>  
>  if (ARCH_IS_S390(arch))
>  return virQEMUCapsKVMSupportsSecureGuestS390();
> +if (ARCH_IS_X86(arch))
> +return virQEMUCapsKVMSupportsSecureGuestAMD();
>  return false;
>  }
>  

Re: [PATCH 3/6] qemu: check if AMD secure guest support is enabled

[Boris Fiuczynski]

On 5/11/20 9:40 PM, Brijesh Singh wrote:

Thanks for the patch Paulo, Few comments.

On 5/11/20 11:41 AM, Boris Fiuczynski wrote:

From: Paulo de Rezende Pinatti 

Implement secure guest check for AMD SEV (Secure Encrypted
Virtualization) in order to invalidate the qemu capabilities
cache in case the availability of the feature changed.

For AMD SEV the verification consists of:
- checking if /sys/module/kvm_amd/parameters/sev contains the
value '1': meaning SEV is enabled in the host kernel;
- checking if the kernel cmdline contains 'mem_encrypt=on': meaning
SME memory encryption feature on the host is enabled



In addition to the kernel module parameter, we probably also need to
check whether QEMU supports the feature. e.g, what if user has newer
kernel but older qemu. e.g kernel > 4.18 but Qemu < 2.12.  To check the
SEV feature support, we should verify the following conditions:

1) check kernel module parameter is set
Paulo implemented the checks following the documentation in 
docs/kbase/launch_security_sev.rst. The check for the module parameter 
sev is included. Is the check for the kernel cmdline parameter 
"mem_encrypt" not necessary? Or would that be covered by your suggested 
check in 2)?




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?




3) Check if Qemu supports SEV feature (maybe we can detect the existence
of the query-sev QMP command or detect Qemu version >= 2.12)
The idea is to check the host capabilities to detect if qemus 
capabilities need to be reprobed. Therefore this would be a result if 
checks 1+2 change but it would not be a cache invalidation trigger.




thanks


Signed-off-by: Paulo de Rezende Pinatti 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
  docs/kbase/launch_security_sev.rst |  2 +-
  src/qemu/qemu_capabilities.c   | 27 +++
  2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/docs/kbase/launch_security_sev.rst 
b/docs/kbase/launch_security_sev.rst
index 65f258587d..fa602c7432 100644
--- a/docs/kbase/launch_security_sev.rst
+++ b/docs/kbase/launch_security_sev.rst
@@ -109,7 +109,7 @@ following:
   
 
  
-Note that if libvirt was already installed and libvirtd running before

+Note that if libvirt (<6.4.0) was already installed and libvirtd running before
  enabling SEV in the kernel followed by the host reboot you need to force
  libvirtd to re-probe both the host and QEMU capabilities. First stop
  libvirtd:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2874bb1e7c..6cf926d52d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4604,6 +4604,31 @@ virQEMUCapsKVMSupportsSecureGuestS390(void)
  }
  
  
+/*

+ * Check whether AMD Secure Encrypted Virtualization (x86) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestAMD(void)
+{
+g_autofree char *cmdline = NULL;
+g_autofree char *modValue = NULL;
+static const char *kValues[] = {"on"};
+
+if (virFileReadValueString(, "/sys/module/kvm_amd/parameters/sev") 
< 0)
+return false;
+if (modValue[0] != '1')
+return false;
+if (virFileReadValueString(, "/proc/cmdline") < 0)
+return false;
+if (virKernelCmdlineMatchParam(cmdline, "mem_encrypt", kValues,
+   G_N_ELEMENTS(kValues),
+   VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST |
+   VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ))
+return true;
+return false;
+}
+
+
  /*
   * Check whether the secure guest functionality is enabled.
   * See the specific architecture function for details on the verifications 
made.
@@ -4615,6 +4640,8 @@ virQEMUCapsKVMSupportsSecureGuest(void)
  
  if (ARCH_IS_S390(arch))

  return virQEMUCapsKVMSupportsSecureGuestS390();
+if (ARCH_IS_X86(arch))
+return virQEMUCapsKVMSupportsSecureGuestAMD();
  return false;
  }
  



--
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: Status of iSER support.

[Peter Krempa]

On Tue, May 12, 2020 at 08:08:28 +0200, David Schweizer wrote:
> Hello Peter,
> 
> Thank you for your answer.
> 
> The problem is that i would like to use the feature in a production
> environment and can not rely on out of tree patches.

Well, you certainly can contribute in having them merged. Even testing
is a contribution.

> 
> In regard to my question i conclude the following:
> - There is currently no support for iSER storage objects in libvirt.

No.

> - There will not be support for iSER storage objects in the next few
> libvirt releases.

Depends on the status of the patches. I've reviewed them yesterday and I
don't agree with the design. If you have more knowledge of how those
things work, even explaining them is a contribution to possbily adding
the feature.

> Which for me, at least at the moment, means dropping iSER/IB, using
> iSCSI/TCP/IPoIB and loosing some performance or using a commercially
> available hypervisor.

Well, for us that means that we don't have anybody willing to give input
on the feature. I don't have experience nor hardware for testing it and
don't have knowledge on how e.g. addressing works.

Re: Status of iSER support.

[David Schweizer]

Hello Peter,

Thank you for your answer.

The problem is that i would like to use the feature in a production
environment and can not rely on out of tree patches.

In regard to my question i conclude the following:
- There is currently no support for iSER storage objects in libvirt.
- There will not be support for iSER storage objects in the next few
libvirt releases.

Which for me, at least at the moment, means dropping iSER/IB, using
iSCSI/TCP/IPoIB and loosing some performance or using a commercially
available hypervisor.

Kind regards,

David

On 5/11/20 9:03 AM, Peter Krempa wrote:
> The following patchset attempts to implement iser transport for disk
> source specification:
> 
> https://www.redhat.com/archives/libvir-list/2020-April/msg01247.html
> 
> It's fairly recent so you can give it a try and ideally provide a
> 'Tested-by:'. I don't have a RDMA setup handy so I can't test it and
> thus can provide a code-only review.
> 
> In case of iSCSI storage pools I don't think that anybody is working on
> iser support.
> 



signature.asc
Description: OpenPGP digital signature