[PATCH v3 3/3] conf: domain: Introduce and use virDomainObjGetMessages()

[Luke Yue]

The test driver and qemu driver share the same code in
virDomainGetMessages(), so extract it to a function.

Signed-off-by: Luke Yue 
---
 src/conf/domain_conf.c   | 53 
 src/conf/domain_conf.h   |  5 
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_driver.c   | 34 +-
 src/test/test_driver.c   | 34 +-
 5 files changed, 61 insertions(+), 66 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d78f846a52..f5b7cc8dc4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -31153,3 +31153,56 @@ virHostdevIsVFIODevice(const virDomainHostdevDef 
*hostdev)
 hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
 hostdev->source.subsys.u.pci.backend == 
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
 }
+
+
+/**
+ * virDomainObjGetMessages
+ * @vm: domain object
+ * @msgs: pointer to a variable to store messages
+ * @flags: zero or more virDomainMessageType flags
+ *
+ * Returns number of messages stored in @msgs, -1 otherwise.
+ */
+int
+virDomainObjGetMessages(virDomainObj *vm,
+char ***msgs,
+unsigned int flags)
+{
+size_t i, n;
+int nmsgs;
+int rv = -1;
+
+*msgs = NULL;
+nmsgs = 0;
+n = 0;
+
+if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
+nmsgs += __builtin_popcount(vm->taint);
+*msgs = g_renew(char *, *msgs, nmsgs+1);
+
+for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
+if (vm->taint & (1 << i)) {
+(*msgs)[n++] = g_strdup_printf(
+_("tainted: %s"),
+_(virDomainTaintMessageTypeToString(i)));
+}
+}
+}
+
+if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
+nmsgs += vm->ndeprecations;
+*msgs = g_renew(char *, *msgs, nmsgs+1);
+
+for (i = 0; i < vm->ndeprecations; i++) {
+(*msgs)[n++] = g_strdup_printf(
+_("deprecated configuration: %s"),
+vm->deprecations[i]);
+}
+}
+
+(*msgs)[nmsgs] = NULL;
+
+rv = nmsgs;
+
+return rv;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f706c498ff..71b9c69e2c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -4138,3 +4138,8 @@ virHostdevIsMdevDevice(const virDomainHostdevDef *hostdev)
 bool
 virHostdevIsVFIODevice(const virDomainHostdevDef *hostdev)
 ATTRIBUTE_NONNULL(1);
+
+int
+virDomainObjGetMessages(virDomainObj *vm,
+char ***msgs,
+unsigned int flags);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 68e4b6aab8..17d2cfbdbe 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -562,6 +562,7 @@ virDomainObjDeprecation;
 virDomainObjEndAPI;
 virDomainObjFormat;
 virDomainObjGetDefs;
+virDomainObjGetMessages;
 virDomainObjGetMetadata;
 virDomainObjGetOneDef;
 virDomainObjGetOneDefState;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 235f575901..592e1236e7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20319,8 +20319,6 @@ qemuDomainGetMessages(virDomainPtr dom,
 {
 virDomainObj *vm = NULL;
 int rv = -1;
-size_t i, n;
-int nmsgs;
 
 virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
   VIR_DOMAIN_MESSAGE_TAINTING, -1);
@@ -20331,37 +20329,7 @@ qemuDomainGetMessages(virDomainPtr dom,
 if (virDomainGetMessagesEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-*msgs = NULL;
-nmsgs = 0;
-n = 0;
-
-if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
-nmsgs += __builtin_popcount(vm->taint);
-*msgs = g_renew(char *, *msgs, nmsgs+1);
-
-for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
-if (vm->taint & (1 << i)) {
-(*msgs)[n++] = g_strdup_printf(
-_("tainted: %s"),
-_(virDomainTaintMessageTypeToString(i)));
-}
-}
-}
-
-if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
-nmsgs += vm->ndeprecations;
-*msgs = g_renew(char *, *msgs, nmsgs+1);
-
-for (i = 0; i < vm->ndeprecations; i++) {
-(*msgs)[n++] = g_strdup_printf(
-_("deprecated configuration: %s"),
-vm->deprecations[i]);
-}
-}
-
-(*msgs)[nmsgs] = NULL;
-
-rv = nmsgs;
+rv = virDomainObjGetMessages(vm, msgs, flags);
 
  cleanup:
 virDomainObjEndAPI();
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 06ba7c4cd2..b389e3412b 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -9353,8 +9353,6 @@ testDomainGetMessages(virDomainPtr dom,
 {
 virDomainObj *vm = NULL;
 int rv = -1;
-size_t i, n;
-int nmsgs;
 
 virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
   VIR_DOMAIN_MESSAGE_TAINTING, 

[PATCH v3 2/3] test_driver: Introduce testDomainObjCheckTaint

[Luke Yue]

In order to test the virDomainGetMessages for test driver, we need to
check some taints or deprecations, so introduce testDomainObjCheckTaint
for checking taints.

As we introduced testDomainObjCheckTaint for test driver, the `dominfo`
command in virshtest will now print tainting messages, so add them for
test.

Signed-off-by: Luke Yue 
---
 src/test/test_driver.c | 57 ++
 tests/virshtest.c  |  2 ++
 2 files changed, 59 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 35742fcde3..06ba7c4cd2 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -9291,6 +9291,61 @@ testDomainCheckpointDelete(virDomainCheckpointPtr 
checkpoint,
 return ret;
 }
 
+static void
+testDomainObjCheckDiskTaint(virDomainObj *obj,
+virDomainDiskDef *disk)
+{
+if (disk->rawio == VIR_TRISTATE_BOOL_YES)
+virDomainObjTaint(obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES);
+
+if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
+virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
+disk->src->path && virFileIsCDROM(disk->src->path) == 1)
+virDomainObjTaint(obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH);
+}
+
+static void
+testDomainObjCheckHostdevTaint(virDomainObj *obj,
+   virDomainHostdevDef *hostdev)
+{
+if (!virHostdevIsSCSIDevice(hostdev))
+return;
+
+if (hostdev->source.subsys.u.scsi.rawio == VIR_TRISTATE_BOOL_YES)
+virDomainObjTaint(obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES);
+}
+
+static void
+testDomainObjCheckNetTaint(virDomainObj *obj,
+   virDomainNetDef *net)
+{
+/* script is only useful for NET_TYPE_ETHERNET (qemu) and
+ * NET_TYPE_BRIDGE (xen), but could be (incorrectly) specified for
+ * any interface type. In any case, it's adding user sauce into
+ * the soup, so it should taint the domain.
+ */
+if (net->script != NULL)
+virDomainObjTaint(obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS);
+}
+
+static void
+testDomainObjCheckTaint(virDomainObj *obj)
+{
+size_t i;
+
+for (i = 0; i < obj->def->ndisks; i++)
+testDomainObjCheckDiskTaint(obj, obj->def->disks[i]);
+
+for (i = 0; i < obj->def->nhostdevs; i++)
+testDomainObjCheckHostdevTaint(obj, obj->def->hostdevs[i]);
+
+for (i = 0; i < obj->def->nnets; i++)
+testDomainObjCheckNetTaint(obj, obj->def->nets[i]);
+
+if (obj->def->os.dtb)
+virDomainObjTaint(obj, VIR_DOMAIN_TAINT_CUSTOM_DTB);
+}
+
 static int
 testDomainGetMessages(virDomainPtr dom,
   char ***msgs,
@@ -9311,6 +9366,8 @@ testDomainGetMessages(virDomainPtr dom,
 nmsgs = 0;
 n = 0;
 
+testDomainObjCheckTaint(vm);
+
 if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
 nmsgs += __builtin_popcount(vm->taint);
 *msgs = g_renew(char *, *msgs, nmsgs+1);
diff --git a/tests/virshtest.c b/tests/virshtest.c
index c1974c46cb..937448cefc 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -22,6 +22,7 @@ main(void)
 
 # define DOM_UUID "ef861801-45b9-11cb-88e3-afbfe5370493"
 # define SECURITY_LABEL "libvirt-test (enforcing)"
+# define MESSAGES "tainted: network configuration using opaque shell scripts"
 
 static const char *dominfo_fc4 = "\
 Id: 2\n\
@@ -38,6 +39,7 @@ Managed save:   no\n\
 Security model: testSecurity\n\
 Security DOI:   \n\
 Security label: " SECURITY_LABEL "\n\
+Messages:   " MESSAGES "\n\
 \n";
 static const char *domuuid_fc4 = DOM_UUID "\n\n";
 static const char *domid_fc4 = "2\n\n";
-- 
2.32.0

[PATCH v3 1/3] test_driver: Implement virDomainGetMessages

[Luke Yue]

Signed-off-by: Luke Yue 
---
 src/test/test_driver.c | 53 ++
 1 file changed, 53 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ef0ddab54d..35742fcde3 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -9291,6 +9291,58 @@ testDomainCheckpointDelete(virDomainCheckpointPtr 
checkpoint,
 return ret;
 }
 
+static int
+testDomainGetMessages(virDomainPtr dom,
+  char ***msgs,
+  unsigned int flags)
+{
+virDomainObj *vm = NULL;
+int rv = -1;
+size_t i, n;
+int nmsgs;
+
+virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
+  VIR_DOMAIN_MESSAGE_TAINTING, -1);
+
+if (!(vm = testDomObjFromDomain(dom)))
+return -1;
+
+*msgs = NULL;
+nmsgs = 0;
+n = 0;
+
+if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
+nmsgs += __builtin_popcount(vm->taint);
+*msgs = g_renew(char *, *msgs, nmsgs+1);
+
+for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
+if (vm->taint & (1 << i)) {
+(*msgs)[n++] = g_strdup_printf(
+_("tainted: %s"),
+_(virDomainTaintMessageTypeToString(i)));
+}
+}
+}
+
+if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
+nmsgs += vm->ndeprecations;
+*msgs = g_renew(char *, *msgs, nmsgs+1);
+
+for (i = 0; i < vm->ndeprecations; i++) {
+(*msgs)[n++] = g_strdup_printf(
+_("deprecated configuration: %s"),
+vm->deprecations[i]);
+}
+}
+
+(*msgs)[nmsgs] = NULL;
+
+rv = nmsgs;
+
+virDomainObjEndAPI();
+return rv;
+}
+
 /*
  * Test driver
  */
@@ -9448,6 +9500,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .domainCheckpointLookupByName = testDomainCheckpointLookupByName, /* 5.6.0 
*/
 .domainCheckpointGetParent = testDomainCheckpointGetParent, /* 5.6.0 */
 .domainCheckpointDelete = testDomainCheckpointDelete, /* 5.6.0 */
+.domainGetMessages = testDomainGetMessages, /* 7.6.0 */
 };
 
 static virNetworkDriver testNetworkDriver = {
-- 
2.32.0

[PATCH v3 0/3] Implement virDomainGetMessages for test driver

[Luke Yue]

v3:
- Squash tests commit
- Extract the same code in test driver and qemu driver to a function

Luke Yue (3):
  test_driver: Implement virDomainGetMessages
  test_driver: Introduce testDomainObjCheckTaint
  conf: domain: Introduce and use virDomainObjGetMessages()

 src/conf/domain_conf.c   | 53 +++
 src/conf/domain_conf.h   |  5 +++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_driver.c   | 34 +-
 src/test/test_driver.c   | 78 
 tests/virshtest.c|  2 ++
 6 files changed, 140 insertions(+), 33 deletions(-)

-- 
2.32.0

Re: [libvirt PATCH 3/5] qemu: Default to TPM 2.0 for ARM virt guests

[liuyd.f...@fujitsu.com]

Sorry for the inconvenience.

Tested-by: Liu Yiding 

Thanks,
Liu

On 6/29/21 9:08 PM, Andrea Bolognani wrote:
> On Tue, Jun 29, 2021 at 02:54:24AM +, liuyd.f...@fujitsu.com wrote:
>> That works for me. Thanks.
>>
>> Tested-by: liuyd.f...@fujitsu.com
> Glad to hear that! Can you please provide a full Tested-by tag in the
> expected format
>
>Tested-by: FirstName LastName 
>
> so that it's suitable for inclusion in the git log? Thanks!
>
-- 
Best Regards.
Yiding Liu

[PATCH 04/10] ch_monitor: Add pty json builder function

[William Douglas]

Add function to build the the json structure to configure a PTY in
cloud-hypervisor. The configuration only supports a single serial or
console device.

Signed-off-by: William Douglas 
---
 src/ch/ch_monitor.c | 59 +
 1 file changed, 59 insertions(+)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 5e7d6e3189..d4289b75ce 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -89,6 +89,65 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef 
*vmdef)
 return -1;
 }
 
+static int
+virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef)
+{
+virJSONValue *ptyc = virJSONValueNewObject();
+virJSONValue *ptys = virJSONValueNewObject();
+
+if (vmdef->nconsoles || vmdef->nserials) {
+if ((vmdef->nconsoles &&
+ vmdef->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)
+&& (vmdef->nserials &&
+vmdef->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Only a single console or serial can be 
configured for this domain"));
+goto cleanup;
+} else if (vmdef->nconsoles > 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Only a single console can be configured for this 
domain"));
+goto cleanup;
+} else if (vmdef->nserials > 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Only a single serial can be configured for this 
domain"));
+goto cleanup;
+}
+}
+
+if (vmdef->nconsoles) {
+if (vmdef->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) {
+if (virJSONValueObjectAppendString(ptyc, "mode", "Pty") < 0)
+goto cleanup;
+if (virJSONValueObjectAppend(content, "console", ) < 0)
+goto cleanup;
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Console can only be enabled for a PTY"));
+goto cleanup;
+}
+}
+
+if (vmdef->nserials) {
+if (vmdef->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) {
+if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0)
+goto cleanup;
+if (virJSONValueObjectAppend(content, "serial", ) < 0)
+goto cleanup;
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Serial can only be enabled for a PTY"));
+goto cleanup;
+}
+}
+
+return 0;
+
+ cleanup:
+virJSONValueFree(ptyc);
+virJSONValueFree(ptys);
+return -1;
+}
+
 static int
 virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef)
 {
-- 
2.31.1

[PATCH 10/10] ch_driver: Turn on the domainOpenConsole API

[William Douglas]

With all the rest of the enablement work out of the way, add the final
call for the cloud-hypervisor driver to handle domainOpenConsole.

Signed-off-by: William Douglas 
---
 src/ch/ch_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index b0b8e19120..a11357f83a 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -990,6 +990,7 @@ static virHypervisorDriver chHypervisorDriver = {
 .domainGetXMLDesc = chDomainGetXMLDesc, /* 7.5.0 */
 .domainGetInfo = chDomainGetInfo,   /* 7.5.0 */
 .domainIsActive = chDomainIsActive, /* 7.5.0 */
+.domainOpenConsole = chDomainOpenConsole,   /* 7.6.0 */
 .nodeGetInfo = chNodeGetInfo,   /* 7.5.0 */
 };
 
-- 
2.31.1

[PATCH 09/10] ch_driver: Add handler for console API

[William Douglas]

Add the handler function to find and open the console character
device that will be used by the console API.

Signed-off-by: William Douglas 
---
 src/ch/ch_driver.c | 77 ++
 1 file changed, 77 insertions(+)

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index ac958d73a8..b0b8e19120 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -30,6 +30,7 @@
 #include "viraccessapicheck.h"
 #include "viralloc.h"
 #include "virbuffer.h"
+#include "virchrdev.h"
 #include "vircommand.h"
 #include "virerror.h"
 #include "virfile.h"
@@ -816,6 +817,82 @@ static int chDomainGetInfo(virDomainPtr dom,
 return ret;
 }
 
+static int
+chDomainOpenConsole(virDomainPtr dom,
+const char *dev_name,
+virStreamPtr st,
+unsigned int flags)
+{
+ virDomainObj *vm = NULL;
+ int ret = -1;
+ size_t i;
+ virDomainChrDef *chr = NULL;
+ virCHDomainObjPrivate *priv;
+
+ virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE | VIR_DOMAIN_CONSOLE_FORCE, -1);
+
+ if (!(vm = chDomObjFromDomain(dom)))
+  goto cleanup;
+
+ if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0)
+  goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+  goto cleanup;
+
+ priv = vm->privateData;
+
+ if (dev_name) {
+  for (i = 0; !chr && i < vm->def->nconsoles; i++) {
+   if (vm->def->consoles[i]->info.alias &&
+   STREQ(dev_name, vm->def->consoles[i]->info.alias))
+chr = vm->def->consoles[i];
+  }
+  for (i = 0; !chr && i < vm->def->nserials; i++) {
+   if (STREQ(dev_name, vm->def->serials[i]->info.alias))
+chr = vm->def->serials[i];
+  }
+  for (i = 0; !chr && i < vm->def->nparallels; i++) {
+   if (STREQ(dev_name, vm->def->parallels[i]->info.alias))
+chr = vm->def->parallels[i];
+  }
+ } else {
+  if (vm->def->nconsoles &&
+  vm->def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)
+   chr = vm->def->consoles[0];
+  else if (vm->def->nserials &&
+   vm->def->serials[0]->source->type == 
VIR_DOMAIN_CHR_TYPE_PTY)
+   chr = vm->def->serials[0];
+ }
+
+ if (!chr) {
+  virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find character 
device %s"),
+ NULLSTR(dev_name));
+  goto cleanup;
+ }
+
+ if (chr->source->type != VIR_DOMAIN_CHR_TYPE_PTY) {
+  virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("character device %s is not using a PTY"),
+ dev_name ? dev_name : NULLSTR(chr->info.alias));
+  goto cleanup;
+ }
+
+ /* handle mutually exclusive access to console devices */
+ ret = virChrdevOpen(priv->chrdevs, chr->source, st,
+ (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0);
+
+ if (ret == 1) {
+  virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Active console session exists for this domain"));
+  ret = -1;
+ }
+
+ cleanup:
+ virDomainObjEndAPI();
+ return ret;
+}
+
 static int chStateCleanup(void)
 {
 if (ch_driver == NULL)
-- 
2.31.1

[PATCH 05/10] ch_monitor: Make use of the PTY json builder

[William Douglas]

Call into the PTY json builder for configured serial and console
devices though the devices themselves still aren't allowed in
configuration.

Signed-off-by: William Douglas 
---
 src/ch/ch_monitor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index d4289b75ce..2ade6967fb 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -429,6 +429,9 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
 goto cleanup;
 }
 
+if (virCHMonitorBuildPTYJson(content, vmdef) < 0)
+goto cleanup;
+
 if (virCHMonitorBuildCPUJson(content, vmdef) < 0)
 goto cleanup;
 
-- 
2.31.1

[PATCH 06/10] ch_process: Handle enabled console devices

[William Douglas]

Add functionality to allow libvirt console to connect to the
cloud-hypervisor created PTY associated with a VM. This will need to
be run once the VM is created by cloud-hypervisor.

Signed-off-by: William Douglas 
---
 src/ch/ch_process.c | 64 +
 1 file changed, 64 insertions(+)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 93b1f7f97e..90344f14ea 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -28,6 +28,7 @@
 #include "ch_process.h"
 #include "viralloc.h"
 #include "virerror.h"
+#include "virjson.h"
 #include "virlog.h"
 
 #define VIR_FROM_THIS VIR_FROM_CH
@@ -52,6 +53,69 @@ virCHProcessConnectMonitor(virCHDriver *driver,
 return monitor;
 }
 
+static void
+virCHProcessUpdateConsoleDevice(virDomainObj *vm,
+virJSONValue *config,
+const char *device)
+{
+const char *path;
+virDomainChrDef *chr = NULL;
+virJSONValue *dev, *file;
+
+if (!config)
+return;
+
+dev = virJSONValueObjectGet(config, device);
+if (!dev)
+return;
+
+file = virJSONValueObjectGet(dev, "file");
+if (!file)
+return;
+
+path = virJSONValueGetString(file);
+if (!path)
+return;
+
+if (STREQ(device, "console")) {
+chr = vm->def->consoles[0];
+} else if (STREQ(device, "serial")) {
+chr = vm->def->serials[0];
+}
+
+if (chr && chr->source)
+chr->source->data.file.path = g_strdup(path);
+}
+
+static void
+virCHProcessUpdateConsole(virDomainObj *vm,
+  virJSONValue *info)
+{
+virJSONValue *config;
+
+config = virJSONValueObjectGet(info, "config");
+if (!config)
+return;
+
+virCHProcessUpdateConsoleDevice(vm, config, "console");
+virCHProcessUpdateConsoleDevice(vm, config, "serial");
+}
+
+static int
+virCHProcessUpdateInfo(virDomainObj *vm)
+{
+virJSONValue *info;
+virCHDomainObjPrivate *priv = vm->privateData;
+if (virCHMonitorGetInfo(priv->monitor, ) < 0)
+return -1;
+
+virCHProcessUpdateConsole(vm, info);
+
+virJSONValueFree(info);
+
+return 0;
+}
+
 /**
  * virCHProcessStart:
  * @driver: pointer to driver structure
-- 
2.31.1

[PATCH 03/10] ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API

[William Douglas]

Now that virCHMonitorGet is capable of handling data returned by the
cloud-hypervisor API, make use of this via virCHMonitorGetInfo to call
into the vm.info endpoint.

Signed-off-by: William Douglas 
---
 src/ch/ch_monitor.c | 15 +++
 src/ch/ch_monitor.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 93a5f8fdb4..5e7d6e3189 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -745,3 +745,18 @@ virCHMonitorResumeVM(virCHMonitor *mon)
 {
 return virCHMonitorPutNoContent(mon, URL_VM_RESUME);
 }
+
+/**
+ * virCHMonitorGetInfo:
+ * @mon: Pointer to the monitor
+ * @info: Get VM info
+ *
+ * Retrive the VM info and store in @info
+ *
+ * Returns 0 on success.
+ */
+int
+virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info)
+{
+return virCHMonitorGet(mon, URL_VM_INFO, info);
+}
diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
index e717e11cbc..e39b4eb8b2 100644
--- a/src/ch/ch_monitor.h
+++ b/src/ch/ch_monitor.h
@@ -23,6 +23,7 @@
 #include 
 
 #include "virobject.h"
+#include "virjson.h"
 #include "domain_conf.h"
 
 #define URL_ROOT "http://localhost/api/v1;
@@ -34,6 +35,7 @@
 #define URL_VM_REBOOT "vm.reboot"
 #define URL_VM_Suspend "vm.pause"
 #define URL_VM_RESUME "vm.resume"
+#define URL_VM_INFO "vm.info"
 
 typedef struct _virCHMonitor virCHMonitor;
 
@@ -58,3 +60,4 @@ int virCHMonitorShutdownVM(virCHMonitor *mon);
 int virCHMonitorRebootVM(virCHMonitor *mon);
 int virCHMonitorSuspendVM(virCHMonitor *mon);
 int virCHMonitorResumeVM(virCHMonitor *mon);
+int virCHMonitorGetInfo(virCHMonitor *mon, virJSONValue **info);
-- 
2.31.1

[PATCH 02/10] ch_monitor: Update virCHMonitorGet to handle accept a response

[William Douglas]

The virCHMonitorGet function needed to be able to return data from the
hypervisor. This functionality is needed in order for the driver to
support PTY enablement and getting details about the VM state.

Signed-off-by: William Douglas 
---
 src/ch/ch_monitor.c | 37 +++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index f76bc2b423..93a5f8fdb4 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -612,12 +612,33 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char 
*endpoint)
 return ret;
 }
 
+struct curl_data {
+char *content;
+size_t size;
+};
+
+static size_t
+curl_callback(void *contents, size_t size, size_t nmemb, void *userp)
+{
+size_t content_size = size * nmemb;
+struct curl_data *data = (struct curl_data *)userp;
+
+data->content = g_malloc0(content_size + 1);
+memcpy(data->content, contents, content_size);
+data->content[content_size] = 0;
+data->size = content_size;
+
+return content_size;
+}
+
 static int
-virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
+virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue 
**response)
 {
 g_autofree char *url = NULL;
 int responseCode = 0;
 int ret = -1;
+struct curl_slist *headers = NULL;
+struct curl_data data;
 
 url = g_strdup_printf("%s/%s", URL_ROOT, endpoint);
 
@@ -629,12 +650,24 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
 curl_easy_setopt(mon->handle, CURLOPT_UNIX_SOCKET_PATH, mon->socketpath);
 curl_easy_setopt(mon->handle, CURLOPT_URL, url);
 
+if (response) {
+headers = curl_slist_append(headers, "Accept: application/json");
+headers = curl_slist_append(headers, "Content-Type: application/json");
+curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers);
+curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback);
+curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *));
+}
+
 responseCode = virCHMonitorCurlPerform(mon->handle);
 
 virObjectUnlock(mon);
 
-if (responseCode == 200 || responseCode == 204)
+if (responseCode == 200 || responseCode == 204) {
 ret = 0;
+if (response) {
+*response = virJSONValueFromString(data.content);
+}
+}
 
 return ret;
 }
-- 
2.31.1

[PATCH 07/10] ch_process: Update the domain with console path information

[William Douglas]

Add call to update the domain with console path information from
cloud-hypervisor as part of the last stages of initializing the
domain.

Signed-off-by: William Douglas 
---
 src/ch/ch_process.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 90344f14ea..f6ae1677fd 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -156,6 +156,9 @@ int virCHProcessStart(virCHDriver *driver,
 
 vm->pid = priv->monitor->pid;
 vm->def->id = vm->pid;
+
+virCHProcessUpdateInfo(vm);
+
 virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
 
 return 0;
-- 
2.31.1

[PATCH 08/10] ch_domain: Allow controller and chr devices

[William Douglas]

With the console and serial device handling fully functional, allow
the required device types to be specified in the domain
configuration.

Signed-off-by: William Douglas 
---
 src/ch/ch_domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index a6b87e28e5..89a8ad8f45 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -215,6 +215,8 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_NET:
 case VIR_DOMAIN_DEVICE_MEMORY:
 case VIR_DOMAIN_DEVICE_VSOCK:
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+case VIR_DOMAIN_DEVICE_CHR:
 break;
 
 case VIR_DOMAIN_DEVICE_LEASE:
@@ -224,12 +226,10 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_HOSTDEV:
 case VIR_DOMAIN_DEVICE_WATCHDOG:
-case VIR_DOMAIN_DEVICE_CONTROLLER:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
 case VIR_DOMAIN_DEVICE_HUB:
 case VIR_DOMAIN_DEVICE_REDIRDEV:
 case VIR_DOMAIN_DEVICE_SMARTCARD:
-case VIR_DOMAIN_DEVICE_CHR:
 case VIR_DOMAIN_DEVICE_MEMBALLOON:
 case VIR_DOMAIN_DEVICE_NVRAM:
 case VIR_DOMAIN_DEVICE_RNG:
-- 
2.31.1

[PATCH 00/10] ch: Add Console support

[William Douglas]

This series enables console support in the cloud-hypervisor driver.

Cloud-hypervisor only supports a single console or serial device at a
time, hence the checks to ensure the domain configuration is only
passing one or the other.

I wasn't sure if the controller device should have some additonal
validation handling so I just did the minimal to get the console open
functioning.

William Douglas (10):
  ch_monitor: Make unused function static
  ch_monitor: Update virCHMonitorGet to handle accept a response
  ch_monitor: Use virCHMonitorGet to access cloud-hypervisor API
  ch_monitor: Add pty json builder function
  ch_monitor: Make use of the PTY json builder
  ch_process: Handle enabled console devices
  ch_process: Update the domain with console path information
  ch_domain: Allow controller and chr devices
  ch_driver: Add handler for console API
  ch_driver: Turn on the domainOpenConsole API

 src/ch/ch_domain.c  |   4 +-
 src/ch/ch_driver.c  |  78 +
 src/ch/ch_monitor.c | 117 ++--
 src/ch/ch_monitor.h |   3 ++
 src/ch/ch_process.c |  67 +
 5 files changed, 263 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH 01/10] ch_monitor: Make unused function static

[William Douglas]

The virCHMonitorGet function wasn't in use and was declared as
non-static (though not in a header file). This function isn't going to
be used outside of the monitor though so remove the initial
declaration and define the function to be static.

Future work should adjust this function to allow reading VM state
needed for PTY enablement.

Signed-off-by: William Douglas 
---
 src/ch/ch_monitor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 1648d05017..f76bc2b423 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -54,7 +54,6 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor);
 
 int virCHMonitorShutdownVMM(virCHMonitor *mon);
 int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint);
-int virCHMonitorGet(virCHMonitor *mon, const char *endpoint);
 
 static int
 virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef)
@@ -613,7 +612,7 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char 
*endpoint)
 return ret;
 }
 
-int
+static int
 virCHMonitorGet(virCHMonitor *mon, const char *endpoint)
 {
 g_autofree char *url = NULL;
-- 
2.31.1

Re: [PATCH] docs: formatdomain: Document disk serial truncation status quo

[Tomáš Golembiovský]

On Tue, Jun 29, 2021 at 03:33:26PM +0200, Peter Krempa wrote:
> On Mon, Jun 28, 2021 at 16:55:34 +0200, Tomáš Golembiovský wrote:
> > Hi,
> > 
> > I have a few questions regarding this to get better understanding on how
> > this should be handled by management apps.
> > 
> > On Fri, Jun 04, 2021 at 02:08:40PM +0200, Peter Krempa wrote:
> > > Disk serials are truncated arbitrarily and silently by qemu depending on
> > > the device type and how they are configured. Since changing the current
> > > state would lead to more regressions than we have now, document that the
> > > truncation is arbitrary.
> > > 
> > > Signed-off-by: Peter Krempa 
> > > ---
> > >  docs/formatdomain.rst | 10 ++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > index aa7bb8da14..3ee537da14 100644
> > > --- a/docs/formatdomain.rst
> > > +++ b/docs/formatdomain.rst
> > > @@ -3146,6 +3146,16 @@ paravirtualized driver is specified via the 
> > > ``disk`` element.
> > > may look like ``WD-WMAP9A966149``. Not supported for
> > > scsi-block devices, that is those using disk ``type`` 'block' using
> > > ``device`` 'lun' on ``bus`` 'scsi'. :since:`Since 0.7.1`
> > > +
> > > +   Note that depending on hypervisor and device type the serial number 
> > > may be
> > > +   truncated silently. IDE/SATA devices are commonly limited to 20 
> > > characters.
> > > +   SCSI devices depending on hypervisor version are limited to 20, 36 or 
> > > 247
> > 
> > Is this meant to say "hypervisor" or is it really "hypervisor version"?
> > This can mean a huge difference. See below.
> 
> In this case, hypervisor + version.
> 
> > > +   characters.
> > > +
> > > +   Hypervisors may also start rejecting overly long serials instead of
> > > +   truncating them in the future so it's advised to avoid the implicit
> > > +   truncation by testing the desired serial length range with the 
> > > desired device
> > > +   and hypervisor combination.
> > 
> > If hypervisor start rejecting long serial numbers than this will become
> > tricky.
> 
> It indeed will be tricky.
> 
> > Does the above mean libvirt can report the length limit? Or does
> > that mean one should first try running some VMs to test the limit, take
> > a note of the length and hardcode that? If it is the later then
> 
> For now, libvirt can't do that because qemu isn't exposing this data in
> any way, but in case it would make oVirt's life easier I think we can
> ask QEMU to add the length limit in an introspectable fashion.
> 

Ok, there's probably no need for that if we can safely assume the limit
will not become smaller in the future. I was concerned about a situation
where all VMs would suddnely start failing after QEMU upgrade.

Thanks for the info,

Tomas


> > what are the chances that the limit in hypervisor will become smaller?
> 
> Generally I'd assume it's close to 0. Decreasing the length can be
> generally considered as regression in behaviour and more importantly
> there usually aren't technical reasons to do that once it's proven to
> work ad a higher limit.
> 
> > Or is it safe to assume that the limit will only grow in future versions
> > of the hypervisor (notably QEMU).
> 
> For qemu I think it's safe to assume that it will only grow in cases
> where the technical limit of the emulated device's serial passing
> approach is higher than currently considered.
> 

-- 
Tomáš Golembiovský 

Re: [PATCH] docs: formatdomain: Document disk serial truncation status quo

[Peter Krempa]

On Mon, Jun 28, 2021 at 16:55:34 +0200, Tomáš Golembiovský wrote:
> Hi,
> 
> I have a few questions regarding this to get better understanding on how
> this should be handled by management apps.
> 
> On Fri, Jun 04, 2021 at 02:08:40PM +0200, Peter Krempa wrote:
> > Disk serials are truncated arbitrarily and silently by qemu depending on
> > the device type and how they are configured. Since changing the current
> > state would lead to more regressions than we have now, document that the
> > truncation is arbitrary.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  docs/formatdomain.rst | 10 ++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index aa7bb8da14..3ee537da14 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -3146,6 +3146,16 @@ paravirtualized driver is specified via the ``disk`` 
> > element.
> > may look like ``WD-WMAP9A966149``. Not supported for
> > scsi-block devices, that is those using disk ``type`` 'block' using
> > ``device`` 'lun' on ``bus`` 'scsi'. :since:`Since 0.7.1`
> > +
> > +   Note that depending on hypervisor and device type the serial number may 
> > be
> > +   truncated silently. IDE/SATA devices are commonly limited to 20 
> > characters.
> > +   SCSI devices depending on hypervisor version are limited to 20, 36 or 
> > 247
> 
> Is this meant to say "hypervisor" or is it really "hypervisor version"?
> This can mean a huge difference. See below.

In this case, hypervisor + version.

> > +   characters.
> > +
> > +   Hypervisors may also start rejecting overly long serials instead of
> > +   truncating them in the future so it's advised to avoid the implicit
> > +   truncation by testing the desired serial length range with the desired 
> > device
> > +   and hypervisor combination.
> 
> If hypervisor start rejecting long serial numbers than this will become
> tricky.

It indeed will be tricky.

> Does the above mean libvirt can report the length limit? Or does
> that mean one should first try running some VMs to test the limit, take
> a note of the length and hardcode that? If it is the later then

For now, libvirt can't do that because qemu isn't exposing this data in
any way, but in case it would make oVirt's life easier I think we can
ask QEMU to add the length limit in an introspectable fashion.

> what are the chances that the limit in hypervisor will become smaller?

Generally I'd assume it's close to 0. Decreasing the length can be
generally considered as regression in behaviour and more importantly
there usually aren't technical reasons to do that once it's proven to
work ad a higher limit.

> Or is it safe to assume that the limit will only grow in future versions
> of the hypervisor (notably QEMU).

For qemu I think it's safe to assume that it will only grow in cases
where the technical limit of the emulated device's serial passing
approach is higher than currently considered.

Re: [libvirt PATCH 3/5] qemu: Default to TPM 2.0 for ARM virt guests

[Andrea Bolognani]

On Tue, Jun 29, 2021 at 02:54:24AM +, liuyd.f...@fujitsu.com wrote:
> That works for me. Thanks.
>
> Tested-by: liuyd.f...@fujitsu.com

Glad to hear that! Can you please provide a full Tested-by tag in the
expected format

  Tested-by: FirstName LastName 

so that it's suitable for inclusion in the git log? Thanks!

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/4] qemu: interface: add qemuDomainDefIsOvsport

[Michal Prívozník]

On 6/28/21 11:18 AM, zhangjl02 wrote:
> From: zhangjl02 
> 
> Tell whether a port definition is an ovs managed virtual port
> ---
>  src/qemu/qemu_domain.c | 13 +
>  src/qemu/qemu_domain.h |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index fc60e15eea..da5a226fc2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11575,3 +11575,16 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfig *cfg,
>  
>  return 0;
>  }
> +
> +/*
> + * Check whether the port is an ovs managed port
> + */
> +bool qemuDomainDefIsOvsport(virDomainNetDef *net,
> + int actualType) {
> +const virNetDevVPortProfile *vport = 
> virDomainNetGetActualVirtPortProfile(net);
> +if ((actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) && vport  &&
> +vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
> +return true;
> +}
> +return false;
> +}
> \ No newline at end of file
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index acf6ca5ab6..81a9bf0cfb 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1063,3 +1063,6 @@ int
>  qemuDomainNamePathsCleanup(virQEMUDriverConfig *cfg,
> const char *name,
> bool bestEffort);
> +
> +bool qemuDomainDefIsOvsport(virDomainNetDef *net,
> +int actualType);
> \ No newline at end of file
> 

There's nothing QEMU specific in this function. I think it can go into
src/conf/domain_conf.c (somewhere near
virDomainNetGetActualVirtPortProfile() perhaps) and that check from the
function - we have a few places like it, those can be replaced with the
function call.

Michal

Re: [PATCH 3/4] qemu: interface: remove setting noqueue for ovs port

[Michal Prívozník]

On 6/28/21 11:18 AM, zhangjl02 wrote:
> From: zhangjl02 
> 
> Return 0 directly if the port is ovs managed. When the ovs port is set
> noqueue, qos config on this port will not work.
> ---
>  src/qemu/qemu_domain.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index da5a226fc2..2e8236ce7c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11526,6 +11526,8 @@ qemuDomainInterfaceSetDefaultQDisc(virQEMUDriver 
> *driver,
>  actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>  actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>  actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +if (qemuDomainDefIsOvsport(net, actualType))
> +return 0;
>  if (virNetDevBandwidthSetRootQDisc(net->ifname, "noqueue") < 0)
>  return -1;
>  }
> 

How about:

if (!qemuDomainDefIsOvsport() &&
virNetDevBandwidthSetRootQDisc() < 0)
return -1;

Michal

Re: [PATCH 4/4] qemu: interface: check and use ovs command to set qos of ovs managed port

[Michal Prívozník]

On 6/28/21 11:18 AM, zhangjl02 wrote:
> From: zhangjl02 
> 
> When qos is set or delete, we have to check if the port is an ovs managed
> port. If true, call the virNetDevOpenvswitchInterfaceSetQos function when qos
> is set, and call the virNetDevOpenvswitchInterfaceClearQos function when
> the interface is to be destroyed.
> ---
>  src/qemu/qemu_command.c | 10 --
>  src/qemu/qemu_driver.c  | 21 -
>  src/qemu/qemu_hotplug.c | 39 ---
>  src/qemu/qemu_process.c | 11 +--
>  4 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ea513693f7..35085f293c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8610,8 +8610,14 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver,
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
>  if (virNetDevSupportsBandwidth(actualType)) {
> -if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
> -  !virDomainNetTypeSharesHostView(net)) 
> < 0)
> +if (qemuDomainDefIsOvsport(net, actualType)) {
> +if (virNetDevOpenvswitchInterfaceSetQos(net->ifname, 
> actualBandwidth,
> +def->uuid,
> +
> !virDomainNetTypeSharesHostView(net)) < 0)
> +goto cleanup;
> +}
> +else if (virNetDevBandwidthSet(net->ifname, actualBandwidth, 
> false,
> +   
> !virDomainNetTypeSharesHostView(net)) < 0)
>  goto cleanup;
>  } else {
>  VIR_WARN("setting bandwidth on interfaces of "
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 235f575901..eefb67b404 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10231,6 +10231,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>  bool inboundSpecified = false, outboundSpecified = false;
>  int actualType;
>  bool qosSupported = true;
> +bool ovsType = false;
>  
>  virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>VIR_DOMAIN_AFFECT_CONFIG, -1);
> @@ -10277,6 +10278,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>  if (net) {
>  actualType = virDomainNetGetActualType(net);
>  qosSupported = virNetDevSupportsBandwidth(actualType);
> +ovsType = qemuDomainDefIsOvsport(net, actualType);
>  }
>  
>  if (qosSupported && persistentNet) {
> @@ -10366,7 +10368,24 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>  }
>  }
>  
> -if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
> +if (ovsType){
> +if (virNetDevOpenvswitchInterfaceSetQos(net->ifname, 
> newBandwidth,
> +vm->def->uuid,
> +
> !virDomainNetTypeSharesHostView(net)) < 0){
> +virErrorPtr orig_err;
> +virErrorPreserveLast(_err);

Please separate variable declaration block and code block with an empty
line.

> +
> ignore_value(virNetDevOpenvswitchInterfaceSetQos(net->ifname, newBandwidth,
> + 
> vm->def->uuid,
> + 
> !virDomainNetTypeSharesHostView(net)));
> +if (net->bandwidth) {
> +ignore_value(virDomainNetBandwidthUpdate(net,
> + 
> net->bandwidth));
> +}
> +virErrorRestore(_err);
> +goto endjob;
> +}
> +}
> +else if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
>!virDomainNetTypeSharesHostView(net)) < 0) 
> {
>  virErrorPtr orig_err;
>  
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d2a354d026..4cf74072bc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1409,9 +1409,15 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
>  if (virNetDevSupportsBandwidth(actualType)) {
> -if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
> -  !virDomainNetTypeSharesHostView(net)) 
> < 0)
> -goto cleanup;
> +if (qemuDomainDefIsOvsport(net, actualType)) {
> +if (virNetDevOpenvswitchInterfaceSetQos(net->ifname, 
> actualBandwidth,
> +vm->def->uuid,
> +
> 

Re: [PATCH 2/4] qemu: interface: add virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos

[Michal Prívozník]

On 6/28/21 11:18 AM, zhangjl02 wrote:
> From: zhangjl02 
> 
> Introduce qos setting and cleaning method. Use ovs command to set qos
> parameters on specific interface of qemu virtual machine.
> 
> When an ovs port is created, we add 'ifname' to external-ids. When setting
> qos on an ovs port, query its qos and queue. If found, change qos on queried
> queue and qos, otherwise create new queue and qos. When cleaning qos, query
> and clean queues and qos in ovs table record by 'ifname' and 'vmid'.
> ---
>  src/libvirt_private.syms|   2 +
>  src/util/virnetdevopenvswitch.c | 269 
>  src/util/virnetdevopenvswitch.h |  11 ++
>  3 files changed, 282 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..775f6706b3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2805,8 +2805,10 @@ virNetDevMidonetUnbindPort;
>  virNetDevOpenvswitchAddPort;
>  virNetDevOpenvswitchGetMigrateData;
>  virNetDevOpenvswitchGetVhostuserIfname;
> +virNetDevOpenvswitchInterfaceClearQos;
>  virNetDevOpenvswitchInterfaceGetMaster;
>  virNetDevOpenvswitchInterfaceParseStats;
> +virNetDevOpenvswitchInterfaceSetQos;
>  virNetDevOpenvswitchInterfaceStats;
>  virNetDevOpenvswitchMaybeUnescapeReply;
>  virNetDevOpenvswitchRemovePort;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index eac68d9556..f27b74f35f 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -30,6 +30,7 @@
>  #include "virlog.h"
>  #include "virjson.h"
>  #include "virfile.h"
> +#include "virutil.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
> @@ -140,6 +141,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
> char *ifname,
>  g_autofree char *ifaceid_ex_id = NULL;
>  g_autofree char *profile_ex_id = NULL;
>  g_autofree char *vmid_ex_id = NULL;
> +g_autofree char *ifname_ex_id = NULL;
>  
>  virMacAddrFormat(macaddr, macaddrstr);
>  virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -149,6 +151,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
> char *ifname,
>  macaddrstr);
>  ifaceid_ex_id = g_strdup_printf("external-ids:iface-id=\"%s\"", 
> ifuuidstr);
>  vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr);
> +ifname_ex_id = g_strdup_printf("external-ids:ifname=\"%s\"", ifname);
>  if (ovsport->profileID[0] != '\0') {
>  profile_ex_id = g_strdup_printf("external-ids:port-profile=\"%s\"",
>  ovsport->profileID);
> @@ -174,6 +177,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
> char *ifname,
>   "--", "set", "Interface", ifname, ifaceid_ex_id,
>   "--", "set", "Interface", ifname, vmid_ex_id,
>   "--", "set", "Interface", ifname, profile_ex_id,
> + "--", "set", "Interface", ifname, ifname_ex_id,
>   "--", "set", "Interface", ifname,
>   "external-ids:iface-status=active",
>   NULL);
> @@ -614,3 +618,268 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname,
>  
>  return 0;
>  }
> +
> +
> +/**
> + * virNetDevOpenvswitchInterfaceSetQos:
> + * @ifname: on which interface
> + * @bandwidth: rates to set (may be NULL)
> + * @swapped: true if IN/OUT should be set contrariwise
> + *
> + * Update qos configuration of an OVS port.
> + *
> + * If @swapped is set, the IN part of @bandwidth is set on
> + * @ifname's TX, and vice versa. If it is not set, IN is set on
> + * RX and OUT on TX. This is because for some types of interfaces
> + * domain and the host live on the same side of the interface (so
> + * domain's RX/TX is host's RX/TX), and for some it's swapped
> + * (domain's RX/TX is hosts's TX/RX).
> + *
> + * Return 0 on success, -1 otherwise.
> + */
> +int virNetDevOpenvswitchInterfaceSetQos(const char *ifname,
> +const virNetDevBandwidth *bandwidth,
> +const unsigned char *vmid,
> +bool swapped)
> +{
> +int ret = -1;
> +char vmuuidstr[VIR_UUID_STRING_BUFLEN];
> +virNetDevBandwidthRate *rx = NULL; /* From domain POV */
> +virNetDevBandwidthRate *tx = NULL; /* From domain POV */
> +virCommand *cmd = NULL;
> +char *vmid_ex_id = NULL;
> +char *ifname_ex_id = NULL;
> +char *average = NULL;
> +char *peak = NULL;
> +char *burst = NULL;
> +char *qos_uuid = NULL;
> +char *queue_uuid = NULL;
> +char **lines = NULL;
> +
> +if (!bandwidth) {
> +/* nothing to be enabled */
> +ret = 0;
> +goto cleanup;
> +}
> +
> +if (geteuid() != 0) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   

Re: [PATCH v3 6/6] docs: add s390-pv documentation

[Pavel Hrdina]

On Tue, Jun 29, 2021 at 10:05:17AM +0200, Erik Skultety wrote:
> ...
> 
> > > +Example guest definition without launchSecurity
> > > +===
> > > +
> > > +Minimal domain XML for a protected virtualization guest using the
> > > +``iommu='on'`` setting for each virtio device.
> > 
> > I don't know how s390-pv works but for example with AMD SEV it is
> > required to use `iommu='on'` otherwise the device is not visible inside
> > the VM so I would like to make sure there is no misunderstanding and
> > it is correct.
> 
> Can you elaborate on how is the device not visible in the VM? IIRC 'iommu=on'
> makes sure that the guest virtio driver is able to negotiate the
> VIRTIO_F_IOMMU_PLATFORM feature which in connection with the correct IOMMU 
> model
> setting makes SEV work with virtio and IOMMU
> (AFAIR OVMF has a dedicated SEV iommu driver).
> 
> Therefore, that flag should have nothing to do with device visibility, in fact
> in x86_64's case it will be a PCI device, so you'll always be able to list
> those.

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

We had a discussion about this BZ that someone tried to hot-plug device
into VM with AMD-SEV enabled and the device was not visible (or possibly
was visible but did not work) in the VM because iommu was not set.

Here is a QEMU commit message that enables iommu_platform if
confidential guest support is used:

commit 9f88a7a3df11a5aaa6212ea535d40d5f92561683
Author: David Gibson 
Date:   Thu Jun 4 14:20:24 2020 +1000

confidential guest support: Alter virtio default properties for protected 
guests

The default behaviour for virtio devices is not to use the platforms normal
DMA paths, but instead to use the fact that it's running in a hypervisor
to directly access guest memory.  That doesn't work if the guest's memory
is protected from hypervisor access, such as with AMD's SEV or POWER's PEF.

So, if a confidential guest mechanism is enabled, then apply the
iommu_platform=on option so it will go through normal DMA mechanisms.
Those will presumably have some way of marking memory as shared with
the hypervisor or hardware so that DMA will work.

Pavel


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] test_driver: Implement virDomainGetMessages

[Martin Kletzander]

On Mon, Jun 28, 2021 at 11:36:45PM +0800, Luke Yue wrote:

And I have another question, as it's freeze for 7.5.0, should I change
the version number in comments to 7.6.0 in the new patches? Thanks!



Yep, it won't make 7.5.0 now.  Thanks.


signature.asc
Description: PGP signature

Re: [PATCH v3 6/6] docs: add s390-pv documentation

[Boris Fiuczynski]

On 6/25/21 12:11 PM, Pavel Hrdina wrote:

@@ -158,8 +163,42 @@ allocated 2K entries. A commonly used value for swiotlb is 
262144.
  Example guest definition
  
  
-Minimal domain XML for a protected virtualization guest, essentially

-it's mostly about the ``iommu`` property
+Minimal domain XML for a protected virtualization guest with
+the ``launchSecurity`` element of type ``s390-pv``
+
+::
+
+   
+ protected
+ 2048000
+ 2048000
+ 1
+ 
+   hvm
+ 
+ 
+ 
+   
+ 
+ 
+ 
+   
+   
+ 
+ 
+   
+   
+   
+ 
+ 
+   
+
+
+Example guest definition without launchSecurity
+===
+
+Minimal domain XML for a protected virtualization guest using the
+``iommu='on'`` setting for each virtio device.

I don't know how s390-pv works but for example with AMD SEV it is
required to use `iommu='on'` otherwise the device is not visible inside
the VM so I would like to make sure there is no misunderstanding and
it is correct.

Pavel



Using IBM Secure Execution you have to use `iommu='on'` on each virtio 
device. If you do not do so the devices will be available in the guest 
but it is very likely that once some tries to use these devices the 
guest very likely is going to crash.
BUT when specifying launchSecurity with type 's390-pv' one does not have 
to use `iommu='on'` on each virtio device any longer!


I tried to cover that with this change in the docs:
+Since libvirt 7.5.0 the
+` `__
+element with type ``s390-pv`` should be used on protected 
virtualization guests.

+Without ``launchSecurity`` you must enable all virtio devices to use shared
+buffers by configuring them with platform_iommu enabled.

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v3 3/6] conf: refactor launch security to allow more types

[Boris Fiuczynski]

On 6/25/21 10:51 AM, Pavel Hrdina wrote:

On Tue, Jun 22, 2021 at 03:10:46PM +0200, Boris Fiuczynski wrote:

Adding virDomainSecDef for general launch security data
and moving virDomainSEVDef as an element for SEV data.

Signed-off-by: Boris Fiuczynski 
---
  src/conf/domain_conf.c  | 127 +++-
  src/conf/domain_conf.h  |  11 +++-
  src/conf/virconftypes.h |   2 +
  src/qemu/qemu_cgroup.c  |   4 +-
  src/qemu/qemu_command.c |  44 +++--
  src/qemu/qemu_driver.c  |   3 +-
  src/qemu/qemu_firmware.c|  33 ++
  src/qemu/qemu_namespace.c   |  20 --
  src/qemu/qemu_process.c |  33 --
  src/qemu/qemu_validate.c|  22 +--
  src/security/security_dac.c |   6 +-
  11 files changed, 218 insertions(+), 87 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 93ec50ff5d..2bd5210a16 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3502,6 +3502,19 @@ virDomainSEVDefFree(virDomainSEVDef *def)
  g_free(def);
  }
  
+

+void
+virDomainSecDefFree(virDomainSecDef *def)
+{
+if (!def)
+return;
+
+virDomainSEVDefFree(def->sev);
+
+g_free(def);
+}
+
+
  static void
  virDomainOSDefClear(virDomainOSDef *os)
  {
@@ -3703,7 +3716,7 @@ void virDomainDefFree(virDomainDef *def)
  if (def->namespaceData && def->ns.free)
  (def->ns.free)(def->namespaceData);
  
-virDomainSEVDefFree(def->sev);

+virDomainSecDefFree(def->sec);
  
  xmlFreeNode(def->metadata);
  
@@ -14720,57 +14733,72 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,

  {
  VIR_XPATH_NODE_AUTORESTORE(ctxt)
  unsigned long policy;
-g_autofree char *type = NULL;
  int rc = -1;
  
  g_autoptr(virDomainSEVDef) def = g_new0(virDomainSEVDef, 1);
  
  ctxt->node = sevNode;
  
-if (!(type = virXMLPropString(sevNode, "type"))) {

+if (virXPathULongHex("string(./policy)", ctxt, ) < 0) {
  virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing launch security type"));
+   _("failed to get launch security policy for "
+ "launch security type SEV"));
  return NULL;
  }
  
-def->sectype = virDomainLaunchSecurityTypeFromString(type);

-switch ((virDomainLaunchSecurity) def->sectype) {
-case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-if (virXPathULongHex("string(./policy)", ctxt, ) < 0) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("failed to get launch security policy for "
- "launch security type SEV"));
-return NULL;
-}
+/* the following attributes are platform dependent and if missing,
+ * we can autofill them from domain capabilities later
+ */
+rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
+if (rc == 0) {
+def->haveCbitpos = true;
+} else if (rc == -2) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Invalid format for launch security cbitpos"));
+return NULL;
+}
  
-/* the following attributes are platform dependent and if missing,

- * we can autofill them from domain capabilities later
- */
-rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
-if (rc == 0) {
-def->haveCbitpos = true;
-} else if (rc == -2) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("Invalid format for launch security cbitpos"));
-return NULL;
-}
+rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
+  >reduced_phys_bits);
+if (rc == 0) {
+def->haveReducedPhysBits = true;
+} else if (rc == -2) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Invalid format for launch security "
+ "reduced-phys-bits"));
+return NULL;
+}
  
-rc = virXPathUInt("string(./reducedPhysBits)", ctxt,

-  >reduced_phys_bits);
-if (rc == 0) {
-def->haveReducedPhysBits = true;
-} else if (rc == -2) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("Invalid format for launch security "
- "reduced-phys-bits"));
-return NULL;
-}
+def->policy = policy;
+def->dh_cert = virXPathString("string(./dhCert)", ctxt);
+def->session = virXPathString("string(./session)", ctxt);
+
+return g_steal_pointer();
+}
+
+
+static virDomainSecDef *
+virDomainSecDefParseXML(xmlNodePtr lsecNode,
+xmlXPathContextPtr ctxt)
+{
+g_autoptr(virDomainSecDef) sec = g_new0(virDomainSecDef, 1);
+g_autofree char *type = NULL;
  
-def->policy = policy;

-def->dh_cert = virXPathString("string(./dhCert)", ctxt);
-def->session = virXPathString("string(./session)", 

Re: [PATCH v3 6/6] docs: add s390-pv documentation

[Erik Skultety]

...

> > +Example guest definition without launchSecurity
> > +===
> > +
> > +Minimal domain XML for a protected virtualization guest using the
> > +``iommu='on'`` setting for each virtio device.
> 
> I don't know how s390-pv works but for example with AMD SEV it is
> required to use `iommu='on'` otherwise the device is not visible inside
> the VM so I would like to make sure there is no misunderstanding and
> it is correct.

Can you elaborate on how is the device not visible in the VM? IIRC 'iommu=on'
makes sure that the guest virtio driver is able to negotiate the
VIRTIO_F_IOMMU_PLATFORM feature which in connection with the correct IOMMU model
setting makes SEV work with virtio and IOMMU
(AFAIR OVMF has a dedicated SEV iommu driver).

Therefore, that flag should have nothing to do with device visibility, in fact
in x86_64's case it will be a PCI device, so you'll always be able to list
those.

Regards,
Erik

libvirt-7.5.0 release candidate 2

[Jiri Denemark]

I have just tagged v7.5.0-rc2 in the repository and pushed signed
tarballs and source RPMs to https://libvirt.org/sources/

Please give the release candidate some testing and in case you find a
serious issue which should have a fix in the upcoming release, feel
free to reply to this thread to make sure the issue is more visible.

If you have not done so yet, please update NEWS.rst to document any
significant change you made since the last release.

Thanks,

Jirka