[libvirt] [PATCH] libxl: fix leaking logfile fds
Per-domain log files were introduced in commit a30b08b7179. The FILE
objects associated with these log files are stored in a hash table
using domid as a key. When a domain is shutdown, destroyed, or
otherwise powered-off, the FILE object is removed from the hash table,
where the free function will close the FILE.
Unfortunately the call to remove the FILE from the hash table occurs
after setting domid=-1 in the libxlDomainCleanup() function. The
object is never removed from the hash table, the free function is
never called, and the underlying fd is leaked. Fix by removing the
FILE object from the hash table before setting domid=-1.
Signed-off-by: Jim Fehlig
---
src/libxl/libxl_domain.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index d4859d6707..d12b1b1b4b 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -769,6 +769,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
VIR_WARN("Unable to release lease on %s", vm->def->name);
VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState));
+libxlLoggerCloseFile(cfg->logger, vm->def->id);
vm->def->id = -1;
if (priv->deathW) {
@@ -822,8 +823,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
VIR_FREE(xml);
}
-libxlLoggerCloseFile(cfg->logger, vm->def->id);
-
virDomainObjRemoveTransientDef(vm);
virObjectUnref(cfg);
}
--
2.16.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Question about using cpu mode "host-model" while providing a cpu model name
On 05/17/2018 03:28 PM, Jiri Denemark wrote: > On Wed, May 09, 2018 at 13:36:55 +0200, Halil Pasic wrote: >> >> >> On 05/09/2018 12:41 PM, Jiri Denemark wrote: >>> On Tue, May 08, 2018 at 10:44:22 -0400, Collin Walling wrote: Hi I have noticed something that may be misconstrued regarding the libvirt domain xml format for defining a cpu model. There seems to be a misalignment where the libvirt documentation states something that is not supported, but libvirt itself gives no clear indication of such. This is regarding the cpu mode "host-model" and providing a cpu model name between the tags. >From the libvirt docs under header "CPU model and topology" paragraph >"cpu" subparagraph "host-model", the following rule is defined (bolded or between asterisks): "... The match attribute can't be used in this mode. *Specifying CPU model is not supported* either, but model's fallback attribute may still be used. ..." https://libvirt.org/formatdomain.html#elementsCPU The above rule reads as "if mode is 'host-model' (and the architecture is not PowerPC) then specifying a model name should not be allowed". However, this is not the observed behavior. For example, I can define and start a guest with the following xml snippet without any issues: cpu-name Which seems to contradict what the documentation states. >>> >>> It's not forbidden for compatibility reasons. Old libvirt used to fill >>> in the ... in during >>> migration and save/restore so that the destination would know the actual >>> CPU the domain was started with. We changed this so that host-model >>> automatically turns into mode='custom' CPU when a domain starts, but we >>> still need to support parsing the XML whare mode='host-model' and >>> are used at the same time. When a domain is migrated, >>> libvirt will turn the incoming host-model into custom mode. Otherwise >>> the specified model will just be ignored. >>> >>> Jirka >>> >> >> Thank you very much for your explanation. AFAIU we already differentiate >> between: >> a) 'incoming migration' (~ transient domain definition) --> convert to >> 'custom' >> b) 'otherwise' (~ persistent domain definition) --> ignored. > > Right, but this differentiation is not done in the parser, so we still > need to keep the code there. However, we should be able to do so in the > appropriate PostParse function. > >> Thus b') persistent domain definition --> error or warn would >> not break the scenario you described (again AFAIU). > > I guess we could fail when a new domain is defined, but we must be extra > careful to not break backward compatibility. Alternatively we could just > drop the model name from CPU definition when a new domain is defined > (and log a warning) as suggested by Boris in another email. This > approach would be probably a bit better wrt backward compatibility since > everything would keep working. > I know it has been a while, but I have some time set aside now that I can use to look into this unless someone else feels better about tackling it. Also my apologies for the long silence. -- Respectfully, - Collin Walling -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] qemu: domain: Add support for TLS for NBD
https://bugzilla.redhat.com/show_bug.cgi?id=1544869
Signed-off-by: Peter Krempa
---
docs/formatdomain.html.in | 8 -
docs/schemas/domaincommon.rng | 5 +++
src/qemu/qemu_command.c| 5 +++
src/qemu/qemu_domain.c | 38 --
.../disk-drive-network-tlsx509.args| 9 -
.../disk-drive-network-tlsx509.xml | 8 +
tests/qemuxml2argvtest.c | 2 +-
.../disk-drive-network-tlsx509.xml | 8 +
8 files changed, 78 insertions(+), 5 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index b5a6e33bfe..dccabe7f35 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2786,7 +2786,13 @@
is mandatory to specify which volume/image will be used.
- For "nbd", the name attribute is optional.
+ For "nbd", the name attribute is optional. TLS
+ transport for NBD can be enabled by using the tls
+ attribute. For the QEMU hypervisor, usage of a TLS environment
can
+ lso be globally controlled on the host by the
+ nbd_tls and nbd_tls_x509_cert_dir in
+ /etc/libvirt/qemu.conf.
+ ('tls' Since 4.5.0)
For "iscsi" (since 1.0.4), the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 703a1bb6f8..ce2d1e91e0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1706,6 +1706,11 @@
+
+
+
+
+
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c75595ca6d..75c05f9e9a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1388,6 +1388,11 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src,
virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET))
return true;
+if (actualType == VIR_STORAGE_TYPE_NETWORK &&
+src->protocol == VIR_STORAGE_NET_PROTOCOL_NBD &&
+src->haveTLS == VIR_TRISTATE_BOOL_YES)
+return true;
+
return false;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 873bcec50d..1bfa6a926a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9934,6 +9934,34 @@
qemuProcessPrepareStorageSourceTLSVxhs(virStorageSourcePtr src,
}
+static int
+qemuProcessPrepareStorageSourceTLSNBD(virStorageSourcePtr src,
+ virQEMUDriverConfigPtr cfg,
+ virQEMUCapsPtr qemuCaps)
+{
+if (src->haveTLS == VIR_TRISTATE_BOOL_ABSENT) {
+if (cfg->nbdTLS)
+src->haveTLS = VIR_TRISTATE_BOOL_YES;
+else
+src->haveTLS = VIR_TRISTATE_BOOL_NO;
+src->tlsFromConfig = true;
+}
+
+if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("this qemu does not support TLS transport for
NBD"));
+return -1;
+}
+
+if (VIR_STRDUP(src->tlsCertdir, cfg->nbdTLSx509certdir) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
/* qemuProcessPrepareStorageSourceTLS:
* @source: source for a disk
* @cfg: driver configuration
@@ -9948,7 +9976,8 @@
qemuProcessPrepareStorageSourceTLSVxhs(virStorageSourcePtr src,
static int
qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr src,
virQEMUDriverConfigPtr cfg,
- const char *parentAlias)
+ const char *parentAlias,
+ virQEMUCapsPtr qemuCaps)
{
if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK)
return 0;
@@ -9960,6 +9989,10 @@ qemuDomainPrepareStorageSourceTLS(virStorageSourcePtr
src,
break;
case VIR_STORAGE_NET_PROTOCOL_NBD:
+if (qemuProcessPrepareStorageSourceTLSNBD(src, cfg, qemuCaps) < 0)
+return -1;
+break;
+
case VIR_STORAGE_NET_PROTOCOL_RBD:
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
@@ -12502,7 +12535,8 @@ qemuDomainPrepareDiskSourceLegacy(virDomainDiskDefPtr
disk,
if (qemuDomainPrepareStorageSourcePR(disk->src, priv, disk->info.alias) <
0)
return -1;
-if (qemuDomainPrepareStorageSourceTLS(disk->src, cfg, disk->info.alias) <
0)
+if (qemuDomainPrepareStorageSourceTLS(disk->src, cfg, disk->info.alias,
+ priv->qemuCaps) < 0)
return -1;
return 0;
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
index 91d3a8a70a..970b8a32a6 100644
---
[libvirt] [PATCH v2 3/3] tests: qemublock: Test NBD with TLS in the JSON generator
Signed-off-by: Peter Krempa
---
tests/qemublocktest.c | 1 +
tests/qemublocktestdata/xml2json/network-nbd-tls.json | 19 +++
tests/qemublocktestdata/xml2json/network-nbd-tls.xml | 18 ++
3 files changed, 38 insertions(+)
create mode 100644 tests/qemublocktestdata/xml2json/network-nbd-tls.json
create mode 100644 tests/qemublocktestdata/xml2json/network-nbd-tls.xml
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index d0cd834b05..0c335abc5b 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -486,6 +486,7 @@ mymain(void)
TEST_DISK_TO_JSON("file-backing_basic-cache-unsafe");
TEST_DISK_TO_JSON("network-qcow2-backing-chain-cache-unsafe");
TEST_DISK_TO_JSON("dir-fat-cache");
+TEST_DISK_TO_JSON("network-nbd-tls");
TEST_DISK_TO_JSON("block-raw-noopts");
TEST_DISK_TO_JSON("block-raw-reservations");
diff --git a/tests/qemublocktestdata/xml2json/network-nbd-tls.json
b/tests/qemublocktestdata/xml2json/network-nbd-tls.json
new file mode 100644
index 00..a1529a6c44
--- /dev/null
+++ b/tests/qemublocktestdata/xml2json/network-nbd-tls.json
@@ -0,0 +1,19 @@
+{
+ "node-name": "node-b-f",
+ "read-only": false,
+ "driver": "qcow2",
+ "file": "node-a-s",
+ "backing": null
+}
+{
+ "driver": "nbd",
+ "server": {
+"type": "inet",
+"host": "host1.example.com",
+"port": "10809"
+ },
+ "tls-creds": "node-a-s-tls0",
+ "node-name": "node-a-s",
+ "read-only": false,
+ "discard": "unmap"
+}
diff --git a/tests/qemublocktestdata/xml2json/network-nbd-tls.xml
b/tests/qemublocktestdata/xml2json/network-nbd-tls.xml
new file mode 100644
index 00..1330a5acc7
--- /dev/null
+++ b/tests/qemublocktestdata/xml2json/network-nbd-tls.xml
@@ -0,0 +1,18 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/3] qemu: conf: Add qemu.conf knobs for setting up TLS for NBD
Signed-off-by: Peter Krempa
---
src/qemu/libvirtd_qemu.aug | 4
src/qemu/qemu.conf | 34 ++
src/qemu/qemu_conf.c | 15 +++
src/qemu/qemu_conf.h | 3 +++
src/qemu/test_libvirtd_qemu.aug.in | 2 ++
5 files changed, 58 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 2dc16e91fd..679f48cbca 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -119,6 +119,9 @@ module Libvirtd_qemu =
let vxhs_entry = bool_entry "vxhs_tls"
| str_entry "vxhs_tls_x509_cert_dir"
+ let nbd_entry = bool_entry "nbd_tls"
+| str_entry "nbd_tls_x509_cert_dir"
+
(* Each entry in the config is one of the following ... *)
let entry = default_tls_entry
| vnc_entry
@@ -138,6 +141,7 @@ module Libvirtd_qemu =
| gluster_debug_level_entry
| memory_entry
| vxhs_entry
+ | nbd_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^
\t\n][^\n]*)?/ . del /\n/ "\n" ]
let empty = [ label "#empty" . eol ]
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 31738ff19c..c8e1a62d1c 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -297,6 +297,40 @@
#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs"
+
+# Enable use of TLS encryption for all NBD disk devices that don't
+# specifically disable it.
+#
+# When the NBD server is set up appropriately, x509 certificates are required
+# for authentication between the client and the remote NBD server.
+#
+# It is necessary to setup CA and issue the client certificate before
+# enabling this.
+#
+#nbd_tls = 1
+
+
+# In order to override the default TLS certificate location for NBD
+# backed storage, supply a valid path to the certificate directory.
+# This is used to authenticate the NBD block device clients to the NBD
+# server.
+#
+# If the provided path does not exist, libvirtd will fail to start.
+# If the path is not provided, but nbd_tls = 1, then the
+# default_tls_x509_cert_dir path will be used.
+#
+# NBD block device clients expect the client certificate and key to be
+# present in the certificate directory along with the CA certificate.
+# Since this is only a client the server-key.pem certificate is not needed.
+# Thus a NBD directory must contain the following:
+#
+# ca-cert.pem - the CA master certificate
+# client-cert.pem - the client certificate signed with the ca-cert.pem
+# client-key.pem - the client private key
+#
+#nbd_tls_x509_cert_dir = "/etc/pki/libvirt-nbd"
+
+
# In order to override the default TLS certificate location for migration
# certificates, supply a valid path to the certificate directory. If the
# provided path does not exist, libvirtd will fail to start. If the path is
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 277ab833a8..5f35a49e91 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -279,6 +279,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
SET_TLS_X509_CERT_DEFAULT(chardev);
SET_TLS_X509_CERT_DEFAULT(migrate);
SET_TLS_X509_CERT_DEFAULT(vxhs);
+SET_TLS_X509_CERT_DEFAULT(nbd);
#undef SET_TLS_X509_CERT_DEFAULT
@@ -378,6 +379,7 @@ static void virQEMUDriverConfigDispose(void *obj)
VIR_FREE(cfg->chardevTLSx509secretUUID);
VIR_FREE(cfg->vxhsTLSx509certdir);
+VIR_FREE(cfg->nbdTLSx509certdir);
VIR_FREE(cfg->migrateTLSx509certdir);
VIR_FREE(cfg->migrateTLSx509secretUUID);
@@ -458,6 +460,7 @@
virQEMUDriverConfigTLSDirResetDefaults(virQEMUDriverConfigPtr cfg)
CHECK_RESET_CERT_DIR_DEFAULT(chardev);
CHECK_RESET_CERT_DIR_DEFAULT(migrate);
CHECK_RESET_CERT_DIR_DEFAULT(vxhs);
+CHECK_RESET_CERT_DIR_DEFAULT(nbd);
return 0;
}
@@ -561,6 +564,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir",
&cfg->vxhsTLSx509certdir) < 0)
goto cleanup;
+if (virConfGetValueBool(conf, "nbd_tls", &cfg->nbdTLS) < 0)
+goto cleanup;
+if (virConfGetValueString(conf, "nbd_tls_x509_cert_dir",
&cfg->nbdTLSx509certdir) < 0)
+goto cleanup;
#define GET_CONFIG_TLS_CERTINFO(val) \
do { \
@@ -992,6 +999,14 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg)
return -1;
}
+if (STRNEQ(cfg->nbdTLSx509certdir, SYSCONFDIR "/pki/qemu") &&
+!virFileExists(cfg->nbdTLSx509certdir)) {
+virReportError(VIR_ERR_CONF_SYNTAX,
+ _("nbd_tls_x509_cert_dir directory '%s' does not
exist"),
+ cfg->nbdTLSx509certdir);
+return -1;
+}
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7a63780c48..6d25c3e74f 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -207,6 +207,9 @@ struct _virQEMUDriverConfig {
bool
[libvirt] [PATCH v2 0/3] qemu: Add TLS transport for NBD
v2: - added qemu.conf knobs - added docs - fixed test case for changes in ACKed patches This applies on top of my branch collecting all ACKed postings of recent blockdev-related work. Current version can be fetched by: git fetch git://pipo.sk/pipo/libvirt.git blockdev-staging Peter Krempa (3): qemu: conf: Add qemu.conf knobs for setting up TLS for NBD qemu: domain: Add support for TLS for NBD tests: qemublock: Test NBD with TLS in the JSON generator docs/formatdomain.html.in | 8 - docs/schemas/domaincommon.rng | 5 +++ src/qemu/libvirtd_qemu.aug | 4 +++ src/qemu/qemu.conf | 34 +++ src/qemu/qemu_command.c| 5 +++ src/qemu/qemu_conf.c | 15 + src/qemu/qemu_conf.h | 3 ++ src/qemu/qemu_domain.c | 38 -- src/qemu/test_libvirtd_qemu.aug.in | 2 ++ tests/qemublocktest.c | 1 + .../xml2json/network-nbd-tls.json | 19 +++ .../qemublocktestdata/xml2json/network-nbd-tls.xml | 18 ++ .../disk-drive-network-tlsx509.args| 9 - .../disk-drive-network-tlsx509.xml | 8 + tests/qemuxml2argvtest.c | 2 +- .../disk-drive-network-tlsx509.xml | 8 + 16 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 tests/qemublocktestdata/xml2json/network-nbd-tls.json create mode 100644 tests/qemublocktestdata/xml2json/network-nbd-tls.xml -- 2.16.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] qemu: Split handling of managed and unmanaged persistent reservations
On Thu, May 31, 2018 at 19:30:25 +0200, Peter Krempa wrote: > Add code that will handle the managed persistent reservations object > separately from the unmanaged one. There is only one managed object so > handling it with disks is awkward and does not scale well when backing > chains come into view. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_command.c| 113 > +++-- > src/qemu/qemu_command.h| 1 + > src/qemu/qemu_hotplug.c| 104 --- > ...isk-virtio-scsi-reservations.x86_64-latest.args | 4 +- > 4 files changed, 130 insertions(+), 92 deletions(-) [...] > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index e78aff7adf..21503b3905 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c [...] > @@ -461,16 +441,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, > if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) > goto error; > > -if (qemuMaybeBuildPRManagerInfoProps(vm, disk, &prmgrProps) < 0) > +if (qemuDomainDiskAttachManagedPR(vm, disk, &managedPrmgrProps) < 0) > goto error; > > -/* Start daemon only after prmgrProps is built. Otherwise > - * qemuDomainMaybeStartPRDaemon() might start daemon and set > - * priv->prDaemonRunning which confuses props building code. */ > -if ((rv = qemuDomainMaybeStartPRDaemon(vm, disk)) < 0) > +if (disk->src->pr && > +virStoragePRDefIsManaged(disk->src->pr) && This condition term is supposed to be inverted, since we are adding only non-managed objects here. I'll fix it in my branch. > +!(unmanagedPrmgrProps = qemuBuildPRManagerInfoProps(disk->src))) > goto error; signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] util: storage: Add helper for determining whether a backing chain requires PR
With blockdev support we will need to introspect whether any of the
backing chain members requires PR rather just one of them. Add a helper
and reuse it in virDomainDefHasManagedPR.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c| 2 +-
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 14 ++
src/util/virstoragefile.h | 3 +++
4 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfe863d76d..be78d388df 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29928,7 +29928,7 @@ virDomainDefHasManagedPR(const virDomainDef *def)
size_t i;
for (i = 0; i < def->ndisks; i++) {
-if (virStoragePRDefIsManaged(def->disks[i]->src->pr))
+if (virStorageSourceChainHasManagedPR(def->disks[i]->src))
return true;
}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6001635916..68a1056f83 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2813,6 +2813,7 @@ virStoragePRDefIsEqual;
virStoragePRDefIsManaged;
virStoragePRDefParseXML;
virStorageSourceBackingStoreClear;
+virStorageSourceChainHasManagedPR;
virStorageSourceClear;
virStorageSourceCopy;
virStorageSourceFindByNodeName;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 54de2c1c30..6c71d0cdf9 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2025,6 +2025,20 @@ virStoragePRDefIsManaged(virStoragePRDefPtr prd)
}
+bool
+virStorageSourceChainHasManagedPR(virStorageSourcePtr src)
+{
+virStorageSourcePtr n;
+
+for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+if (virStoragePRDefIsManaged(src->pr))
+return true;
+}
+
+return false;
+}
+
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 1631c4cf66..b1ff5142fb 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -401,6 +401,9 @@ bool virStoragePRDefIsEqual(virStoragePRDefPtr a,
virStoragePRDefPtr b);
bool virStoragePRDefIsManaged(virStoragePRDefPtr prd);
+bool
+virStorageSourceChainHasManagedPR(virStorageSourcePtr src);
+
virSecurityDeviceLabelDefPtr
virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
const char *model);
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] qemu: Handle managed persisten reservations separately
Keep the handling of the singleton managed pr-manager-helper object separate from the unmanaged ones which are instantiated one-per-disk-source. This applies on top of my branch collecting all ACKed postings of recent blockdev-related work. Current version can be fetched by: git fetch git://pipo.sk/pipo/libvirt.git blockdev-staging Peter Krempa (4): util: storage: Add helper for determining whether a backing chain requires PR qemu: command: Pass in 'src' rather than 'disk' to qemuBuildPRManagerInfoProps qemu: command: Return props as return value in qemuBuildPRManagerInfoProps qemu: Split handling of managed and unmanaged persistent reservations src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c| 123 ++--- src/qemu/qemu_command.h| 4 +- src/qemu/qemu_hotplug.c| 101 - src/util/virstoragefile.c | 14 +++ src/util/virstoragefile.h | 3 + ...isk-virtio-scsi-reservations.x86_64-latest.args | 4 +- 8 files changed, 154 insertions(+), 98 deletions(-) -- 2.16.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] qemu: Split handling of managed and unmanaged persistent reservations
Add code that will handle the managed persistent reservations object
separately from the unmanaged one. There is only one managed object so
handling it with disks is awkward and does not scale well when backing
chains come into view.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c| 113 +++--
src/qemu/qemu_command.h| 1 +
src/qemu/qemu_hotplug.c| 104 ---
...isk-virtio-scsi-reservations.x86_64-latest.args | 4 +-
4 files changed, 130 insertions(+), 92 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2d559938ed..a42acb3cfc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2201,6 +2201,35 @@ qemuBulildFloppyCommandLineOptions(virCommandPtr cmd,
}
+static int
+qemuBuildDiskUnmanagedPRCommandLine(virCommandPtr cmd,
+virStorageSourcePtr src)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+virJSONValuePtr props = NULL;
+int ret = -1;
+
+if (!src->pr ||
+virStoragePRDefIsManaged(src->pr))
+return 0;
+
+if (!(props = qemuBuildPRManagerInfoProps(src)))
+return -1;
+
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+goto cleanup;
+
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, &buf);
+
+ret = 0;
+ cleanup:
+virBufferFreeAndReset(&buf);
+virJSONValueFree(props);
+return ret;
+}
+
+
static int
qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
const virDomainDef *def,
@@ -2268,6 +2297,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
}
}
+if (qemuBuildDiskUnmanagedPRCommandLine(cmd, disk->src) < 0)
+return -1;
+
if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
return -1;
@@ -9689,58 +9721,77 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
}
+static virJSONValuePtr
+qemuBuildPRManagerInfoPropsInternal(const char *alias,
+const char *path)
+{
+virJSONValuePtr ret = NULL;
+
+if (qemuMonitorCreateObjectProps(&ret,
+ "pr-manager-helper", alias,
+ "s:path", path, NULL) < 0)
+return NULL;
+
+return ret;
+}
+
+
/**
- * qemuBuildPRManagerInfoProps:
- * @src: storage source
+ * qemuBuildPRManagedManagerInfoProps:
*
- * Build the JSON properties for the pr-manager object.
+ * Build the JSON properties for the pr-manager object corresponding to the PR
+ * daemon managed by libvirt.
*/
virJSONValuePtr
-qemuBuildPRManagerInfoProps(virStorageSourcePtr src)
+qemuBuildPRManagedManagerInfoProps(qemuDomainObjPrivatePtr priv)
{
+char *path = NULL;
virJSONValuePtr ret = NULL;
-if (qemuMonitorCreateObjectProps(&ret,
- "pr-manager-helper", src->pr->mgralias,
- "s:path", src->pr->path, NULL) < 0)
+if (!(path = qemuDomainGetManagedPRSocketPath(priv)))
return NULL;
+ret = qemuBuildPRManagerInfoPropsInternal(qemuDomainGetManagedPRAlias(),
+ path);
+
+VIR_FREE(path);
return ret;
}
+/**
+ * qemuBuildPRManagerInfoProps:
+ * @src: storage source
+ *
+ * Build the JSON properties for the pr-manager object.
+ */
+virJSONValuePtr
+qemuBuildPRManagerInfoProps(virStorageSourcePtr src)
+{
+return qemuBuildPRManagerInfoPropsInternal(src->pr->mgralias,
src->pr->path);
+}
+
+
static int
-qemuBuildMasterPRCommandLine(virCommandPtr cmd,
- const virDomainDef *def)
+qemuBuildManagedPRCommandLine(virCommandPtr cmd,
+ const virDomainDef *def,
+ qemuDomainObjPrivatePtr priv)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
-size_t i;
-bool managedAdded = false;
virJSONValuePtr props = NULL;
int ret = -1;
-for (i = 0; i < def->ndisks; i++) {
-const virDomainDiskDef *disk = def->disks[i];
-
-if (!disk->src->pr)
-continue;
-
-if (virStoragePRDefIsManaged(disk->src->pr)) {
-if (managedAdded)
-continue;
-
-managedAdded = true;
-}
+if (!virDomainDefHasManagedPR(def))
+return 0;
-if (!(props = qemuBuildPRManagerInfoProps(disk->src)))
-goto cleanup;
+if (!(props = qemuBuildPRManagedManagerInfoProps(priv)))
+return -1;
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
-goto cleanup;
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+goto cleanup;
-virCommandAddArg(cmd, "-object");
-virCommandAddArgBuffer(cmd, &buf);
-}
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, &buf);
ret = 0;
cleanup:
@@ -9955,7 +10
[libvirt] [PATCH 2/4] qemu: command: Pass in 'src' rather than 'disk' to qemuBuildPRManagerInfoProps
Everything is contained in the virStorageSourceStructure.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 12 +---
src/qemu/qemu_command.h | 2 +-
src/qemu/qemu_hotplug.c | 2 +-
3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 26e61f26f4..9256104f27 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9691,7 +9691,7 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
/**
* qemuBuildPRManagerInfoProps:
- * @disk: disk definition
+ * @src: storage source
* @propsret: Returns JSON object containing properties of the
pr-manager-helper object
*
* Build the JSON properties for the pr-manager object.
@@ -9700,14 +9700,12 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
* -1 on failure (with error message set).
*/
int
-qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
+qemuBuildPRManagerInfoProps(virStorageSourcePtr src,
virJSONValuePtr *propsret)
{
return qemuMonitorCreateObjectProps(propsret,
-"pr-manager-helper",
-disk->src->pr->mgralias,
-"s:path", disk->src->pr->path,
-NULL);
+"pr-manager-helper", src->pr->mgralias,
+"s:path", src->pr->path, NULL);
}
@@ -9734,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
managedAdded = true;
}
-if (qemuBuildPRManagerInfoProps(disk, &props) < 0)
+if (qemuBuildPRManagerInfoProps(disk->src, &props) < 0)
goto cleanup;
if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index e85efcc980..60b4dcf054 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -55,7 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
int **nicindexes);
/* Generate the object properties for pr-manager */
-int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
+int qemuBuildPRManagerInfoProps(virStorageSourcePtr src,
virJSONValuePtr *propsret);
/* Generate the object properties for a secret */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c656409eaa..44bd41ccb6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -401,7 +401,7 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
return 0;
}
-return qemuBuildPRManagerInfoProps(disk, propsret);
+return qemuBuildPRManagerInfoProps(disk->src, propsret);
}
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] qemu: command: Return props as return value in qemuBuildPRManagerInfoProps
Also since we don't do any conditional formatting, fix the comment for
the function.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 22 +++---
src/qemu/qemu_command.h | 3 +--
src/qemu/qemu_hotplug.c | 5 -
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9256104f27..2d559938ed 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9692,20 +9692,20 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
/**
* qemuBuildPRManagerInfoProps:
* @src: storage source
- * @propsret: Returns JSON object containing properties of the
pr-manager-helper object
*
* Build the JSON properties for the pr-manager object.
- *
- * Returns: 0 on success (@propsret is NULL if no properties are needed),
- * -1 on failure (with error message set).
*/
-int
-qemuBuildPRManagerInfoProps(virStorageSourcePtr src,
-virJSONValuePtr *propsret)
+virJSONValuePtr
+qemuBuildPRManagerInfoProps(virStorageSourcePtr src)
{
-return qemuMonitorCreateObjectProps(propsret,
-"pr-manager-helper", src->pr->mgralias,
-"s:path", src->pr->path, NULL);
+virJSONValuePtr ret = NULL;
+
+if (qemuMonitorCreateObjectProps(&ret,
+ "pr-manager-helper", src->pr->mgralias,
+ "s:path", src->pr->path, NULL) < 0)
+return NULL;
+
+return ret;
}
@@ -9732,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
managedAdded = true;
}
-if (qemuBuildPRManagerInfoProps(disk->src, &props) < 0)
+if (!(props = qemuBuildPRManagerInfoProps(disk->src)))
goto cleanup;
if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 60b4dcf054..10aa21bcdd 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
int **nicindexes);
/* Generate the object properties for pr-manager */
-int qemuBuildPRManagerInfoProps(virStorageSourcePtr src,
-virJSONValuePtr *propsret);
+virJSONValuePtr qemuBuildPRManagerInfoProps(virStorageSourcePtr src);
/* Generate the object properties for a secret */
int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 44bd41ccb6..e78aff7adf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -401,7 +401,10 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
return 0;
}
-return qemuBuildPRManagerInfoProps(disk->src, propsret);
+if (!(*propsret = qemuBuildPRManagerInfoProps(disk->src)))
+return -1;
+
+return 0;
}
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: storage: remove virStorageSource->tlsVerify
Disks are client-only so we don't need to have this variable. We also
always pass false for 'isListen' to qemuBuildTLSx509BackendProps for all
disk-related code-paths.
---
This applies on top of my branch collecting all ACKed postings of
recent blockdev-related work. Current version can be fetched by:
git fetch git://pipo.sk/pipo/libvirt.git blockdev-staging
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_domain.c| 2 --
src/qemu/qemu_hotplug.c | 3 +--
src/util/virstoragefile.c | 1 -
src/util/virstoragefile.h | 1 -
5 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 26e61f26f4..c75595ca6d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -774,7 +774,7 @@ qemuBuildDiskSrcTLSx509CommandLine(virCommandPtr cmd,
return 0;
return qemuBuildTLSx509CommandLine(cmd, src->tlsCertdir,
- false, src->tlsVerify,
+ false, true,
NULL, src->tlsAlias, qemuCaps);
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4368e9be35..873bcec50d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9928,8 +9928,6 @@
qemuProcessPrepareStorageSourceTLSVxhs(virStorageSourcePtr src,
if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
if (VIR_STRDUP(src->tlsCertdir, cfg->vxhsTLSx509certdir) < 0)
return -1;
-
-src->tlsVerify = true;
}
return 0;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c656409eaa..2f76c048aa 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -164,8 +164,7 @@ qemuDomainAddDiskSrcTLSObject(virQEMUDriverPtr driver,
if (qemuDomainGetTLSObjects(priv->qemuCaps, NULL,
src->tlsCertdir,
-false,
-src->tlsVerify,
+false, true,
src->tlsAlias,
&tlsProps, NULL) < 0)
goto cleanup;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 54de2c1c30..10fe0f201a 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2171,7 +2171,6 @@ virStorageSourceCopy(const virStorageSource *src,
ret->shared = src->shared;
ret->haveTLS = src->haveTLS;
ret->tlsFromConfig = src->tlsFromConfig;
-ret->tlsVerify = src->tlsVerify;
ret->detected = src->detected;
ret->debugLevel = src->debugLevel;
ret->debug = src->debug;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 1631c4cf66..4591e6e213 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -310,7 +310,6 @@ struct _virStorageSource {
* certificate directory with listen and verify bools. */
char *tlsAlias;
char *tlsCertdir;
-bool tlsVerify;
bool detected; /* true if this entry was not provided by the user */
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [python PATCH] Add support for virConnectBaselineHypervisorCPU
On Thu, May 31, 2018 at 05:16:51PM +0200, Jiri Denemark wrote: The python bindings for this API cannot be generated because are generator is not capable of handling string arrays (char **) parameters. https://bugzilla.redhat.com/show_bug.cgi?id=1584676 Signed-off-by: Jiri Denemark --- generator.py | 1 + libvirt-override-api.xml | 11 libvirt-override.c | 60 3 files changed, 72 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] conf, schema, docs: Add support for TSEG size setting
On 05/31/2018 08:14 AM, Martin Kletzander wrote: > On Wed, May 30, 2018 at 12:00:26PM -0400, John Ferlan wrote: >> >> >> On 05/21/2018 11:00 AM, Martin Kletzander wrote: >>> TSEG (Top of Memory Segment) is one of many regions that SMM (System >>> Management >>> Mode) can occupy. This one, however is special, because a) most of >>> the SMM code >>> lives in TSEG nowadays and b) QEMU just (well, some time ago) added >>> support for >>> so called 'extended' TSEG. The difference to the TSEG implemented in >>> real q35's >>> MCH (Memory Controller Hub) is that it can have any size from 1 MiB >>> up to 65534 >>> MiB in 1 MiB increments. But more about that in the QEMU patch. >> >> ^^ >> >> Which some reader, but not this one, may be eager to find ;-) >> >> Still is there a valid range QEMU would accept? Or do we just let QEMU >> fail if the range is too high? >> >> I think QEMU has MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX >> > > Rather than promising some value, I adjusted it so that it is no longer > false, > no matter what the max is there. > >>> >>> Signed-off-by: Martin Kletzander >>> --- >>> docs/formatdomain.html.in | 39 +++ >>> docs/schemas/domaincommon.rng | 5 +++ >>> src/conf/domain_conf.c | 60 - >>> src/conf/domain_conf.h | 1 + >>> tests/genericxml2xmlindata/tseg.xml | 23 +++ >>> tests/genericxml2xmltest.c | 2 + >>> 6 files changed, 129 insertions(+), 1 deletion(-) >>> create mode 100644 tests/genericxml2xmlindata/tseg.xml >>> >> >> In the category of I hate it when that happens, git am -3 "merged" in >> two chunks incorrectly! Probably wouldn't have happened if I'd done > > You can enable/disable 3-way merges if you do (not) like them. > >> this sooner! The virDomainDefFeaturesCheckABIStability hunk got merged >> into virDomainRedirFilterDefCheckABIStability and the tseg grammar got >> merged under "vmport" and just before "gic". As you can imagine the >> results weren't pretty ;-). >> > > Yeah, happened to me as well, I should've resent this, but I forgot > about the > merge issue and I also wanted to show that this was posted way before the > freeze. Anyway, it's pointless to force it now, I'll leave it for later > (meaning "after the release"). > *Lots* of things were posted after this that got reviewed and pushed before this - that just seems to be "the norm" (for whatever reason)... Not receiving timely reviews is one of those areas that really bothers me. I've been making the effort more recently to try to go in order of longest without review - sometimes a ping makes me lose order though. > Anyway, I keep my branches updated (every now and then) on my github > repo [1], > so if you want to check that, you always can. > > [1] https://github.com/nertpinx/libvirt > OK - I've added that as a remote... >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index 403b638bd4bd..39ebfe398bd7 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -1897,6 +1897,9 @@ >>>>>> >>> >>> + >>> + >>> >>> ... >>> >>> @@ -2056,6 +2059,42 @@ >>> off, default on) enable or disable >>> System Management Mode. >>> Since 2.1.0 >>> + >>> + Optional sub-element tseg can be used to >>> specify the >>> + amount of memory dedicated to SMM TSEG. The size can be >>> specified as a >>> + value of that element, optional attribute >>> unit can be >>> + used to specify the unit of the aforementioned value >>> (defaults to >>> + 'MiB'). >>> + >> >> If this is to be a true paragraph break then these paragraphs needs to >> be wrapped in ... ; otherwise, this just becomes one long run on >> (and quite ugly IMO) paragraph. >> >>> + This value is configurable due to the fact that the >>> calculation cannot >>> + be done right with the guarantee that it will work >>> correctly. For >>> + QEMU TSEG was disabled up to and including >>> pc-q35-2.9 (it >>> + does not make sense fo any other machine type than q35). >> >> s/fo/for/ >> >> This also appears to be a paragraph break... >> >>> + From pc-q35-2.10 the default value was >>> changed to 16 MiB. >> >> s/From/Starting with/ ??? (not required, just a though) >> >>> + That should suffice for up to 272 VCPUs, 5 GiB guest RAM >>> in total, no >>> + hotplug memory range, and 32 GiB of 64-bit PCI MMIO >>> aperture. Or for >>> + 48 VCPUs, with 1TB of guest RAM, no hotplug DIMM range, >>> and 32GB of >>> + 64-bit PCI MMIO aperture. The values may also vary based >>> on the loader >>> + the VM is using. >>> + >>> + Addi48 >>> +
[libvirt] [python PATCH] Add support for virConnectBaselineHypervisorCPU
The python bindings for this API cannot be generated because are
generator is not capable of handling string arrays (char **) parameters.
https://bugzilla.redhat.com/show_bug.cgi?id=1584676
Signed-off-by: Jiri Denemark
---
generator.py | 1 +
libvirt-override-api.xml | 11
libvirt-override.c | 60
3 files changed, 72 insertions(+)
diff --git a/generator.py b/generator.py
index a0fc720..b7d96a1 100755
--- a/generator.py
+++ b/generator.py
@@ -488,6 +488,7 @@ skip_impl = (
'virDomainGetPerfEvents',
'virDomainSetPerfEvents',
'virDomainGetGuestVcpus',
+'virConnectBaselineHypervisorCPU',
)
lxc_skip_impl = (
diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index b63a403..36d3577 100644
--- a/libvirt-override-api.xml
+++ b/libvirt-override-api.xml
@@ -717,5 +717,16 @@
+
+ Computes the most feature-rich CPU which is compatible with all
given CPUs and can be provided by the specified hypervisor.
+
+
+
+
+
+
+
+
+
diff --git a/libvirt-override.c b/libvirt-override.c
index b4c1529..1c95c18 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -9708,6 +9708,63 @@ libvirt_virStreamRecvFlags(PyObject *self
ATTRIBUTE_UNUSED,
#endif /* LIBVIR_CHECK_VERSION(3, 4, 0) */
+#if LIBVIR_CHECK_VERSION(4, 4, 0)
+static PyObject *
+libvirt_virConnectBaselineHypervisorCPU(PyObject *self ATTRIBUTE_UNUSED,
+PyObject *args)
+{
+virConnectPtr conn;
+PyObject *pyobj_conn;
+char *emulator;
+char *arch;
+char *machine;
+char *virttype;
+PyObject *list;
+unsigned int flags;
+char **xmlCPUs = NULL;
+int ncpus = 0;
+size_t i;
+char *cpu;
+PyObject *ret = NULL;
+
+if (!PyArg_ParseTuple(args, (char
*)"OOI:virConnectBaselineHypervisorCPU",
+ &pyobj_conn, &emulator, &arch, &machine, &virttype,
+ &list, &flags))
+return NULL;
+
+conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
+
+if (PyList_Check(list)) {
+ncpus = PyList_Size(list);
+if (VIR_ALLOC_N(xmlCPUs, ncpus) < 0)
+return PyErr_NoMemory();
+
+for (i = 0; i < ncpus; i++) {
+if (libvirt_charPtrUnwrap(PyList_GetItem(list, i),
+ &(xmlCPUs[i])) < 0 ||
+!xmlCPUs[i])
+goto cleanup;
+}
+}
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+cpu = virConnectBaselineHypervisorCPU(conn, emulator, arch, machine,
virttype,
+ (const char **)xmlCPUs, ncpus,
flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+ret = libvirt_constcharPtrWrap(cpu);
+
+ cleanup:
+for (i = 0; i < ncpus; i++)
+VIR_FREE(xmlCPUs[i]);
+VIR_FREE(xmlCPUs);
+VIR_FREE(cpu);
+
+return ret;
+}
+#endif /* LIBVIR_CHECK_VERSION(4, 4, 0) */
+
+
/
* *
* The registration stuff *
@@ -9941,6 +9998,9 @@ static PyMethodDef libvirtMethods[] = {
{(char *) "virStreamSendHole", libvirt_virStreamSendHole, METH_VARARGS,
NULL},
{(char *) "virStreamRecvFlags", libvirt_virStreamRecvFlags, METH_VARARGS,
NULL},
#endif /* LIBVIR_CHECK_VERSION(3, 4, 0) */
+#if LIBVIR_CHECK_VERSION(4, 4, 0)
+{(char *) "virConnectBaselineHypervisorCPU",
libvirt_virConnectBaselineHypervisorCPU, METH_VARARGS, NULL},
+#endif /* LIBVIR_CHECK_VERSION(4, 4, 0) */
{NULL, NULL, 0, NULL}
};
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/13] qemu: Delete old unused code for adding objects to qemu
On Wed, May 30, 2018 at 07:06:37PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/libvirt_private.syms | 1 - src/qemu/qemu_monitor.c | 34 -- src/qemu/qemu_monitor.h | 5 - src/util/virqemu.c | 22 -- src/util/virqemu.h | 4 5 files changed, 66 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/13] qemu: Convert iothread hotplug to qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:36PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/13] qemu: hotplug: Refactor 'secret' props formatting to qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:35PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 31 +++- src/qemu/qemu_hotplug.c | 77 src/qemu/qemu_hotplug.h | 1 - src/qemu/qemu_migration_params.c | 3 +- 4 files changed, 45 insertions(+), 67 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/13] qemu: hotplug: Refactor tls-credential props formatting to qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:34PM +0200, Peter Krempa wrote: Note that it's okay to pass NULL to qemuDomainDelTLSObjects in qemuDomainAddTLSObjects as the tls-creds-x509 object was either not created or qemu crashed. Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 29 +++-- src/qemu/qemu_command.h | 1 + src/qemu/qemu_hotplug.c | 25 +++-- src/qemu/qemu_hotplug.h | 2 +- src/qemu/qemu_migration_params.c | 4 ++-- 5 files changed, 30 insertions(+), 31 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/13] qemu: hotplug: Refactor shmem props formatting to qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:33PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 53 - src/qemu/qemu_hotplug.c | 8 +--- 2 files changed, 23 insertions(+), 38 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Thu, May 31, 2018 at 09:45:04AM -0400, John Ferlan wrote:
On 05/31/2018 03:24 AM, Martin Kletzander wrote:
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
This is way too sparse.
I can't really think of what to say here that is not already mentioned
in the
documentation or self-explanatory in the code. But maybe only I see
that as
self-explanatory. I'll write something up here.
The whole default thing is what really drew my attention back to the
top... It's where I expected to see an explanation why the default was
being set. I think that's something that should be described - you may
disagree, but that's why it's a review.
[...]
See, I know how to cut - I just forgot in my response to patch 4 ;-)
I'll try not to overcut as I've seen done in for reviews I've received
which means I have to find the context of the feedback (we all have our
favorite things to gripe about).
+
+static void
+qemuBuildTSEGCommandLine(virCommandPtr cmd,
+ const virDomainDef *def)
+{
+ if (!def->tseg_size)
Since it's not bool or pointer, prefer the tseg_size == 0
I don't know if you mean that as indicative or imperative. It is very
subjective and for such scenarios I would like to execute my right to
mention
that it is not part of Contributor Guidelines. I know it might seem
rude, but
this is one of the things that's very subjective and for such things I
like to
first reach a consensus before I change what I'm used to writing. I
hope you
understand that.
There's a lot of things not specifically listed in the CG that are
mentioned in many reviews or that are just now done because they have
been mentioned (2 blank lines between functions, formatting of function
headers, 1 variable per line, etc.). When I see this style and remember
to note it, I do... And I have see others mention it too. Many times
it's for enum comparisons.
I feel the same way about most of those things. I have some unfinished patches
here and there for adding these to the CG so that we don't have that many
subjective discussions. Having said that I don't mind having the discussions.
I just don't like having some of the discussions multiple times =) I will learn
a new way if there is a clearly written consensus, it's just that I don't want
to do that just based on some reviews. Until it is written in stone^Wthe repo,
I'm not that convinced. But what is a great plus for me, here and now, is that
I understand your review more. I wasn't really sure whether that was optional
feedback or a requirement before pushing =) Thanks for the explanation. That's
why it is very mandatory to have a healthy communication.
In the long run, whatever. It's a style preference that denotes variable
usage for those reading code "in the future". We don't have a rule for
it, so go with your style if that's what you prefer.
FWIW: back in the dark ages when I first started doing this... Something
like the above for an integer, long, etc. value would be converted by
the compiler to checking *only* if the low bit was set/cleared (think
little endian) because that instruction was really quick. Try to find
that needle in a haystack for each even value that could be used ;-). I
got very used to the explicit comparison to 0 just to be 100% clear.
+ return;
+
+ virCommandAddArg(cmd, "-global");
+
[...]
+ /* QEMU started defaulting to 16MiB after 2.9 */
+ if (major > 2 || (major == 2 && minor > 9))
+ def->tseg_size = 16 * 1024 * 1024;
So, if QEMU defaults to 16MiB, then why do we need so set this at all?
TL;DR because not setting the default value when it is not explicitly
requested has already bitten us in the back multiple times.
Long story short this way we are safe. No matter what happens (QEMU
changing the default, the user changing the config somewhere else and
not expecting this to change, us wanting to change the default in the
future for some reason, migration to newer libvirt where who-knows-what
is checked there, etc.) it is way easier to keep the guest ABI stable.
It is visible what the value actually is and we're not hiding it
somewhere in the code of the hypervisor. I know we don't do that for
many things. I could've gone with the (often alibistic [1]) "the
default is left for hypervisor to decide", but since we have the
opportunity tomake things right (thanks to Laszlo's explanations), why
shouldn't we?
This seems to me we are setting policy which based on history of many
patches before is a no go. I'd say NAK to this, unless there is some
convincing argument made in the commit message and followup responses to
the series (or you can take Jan's R-By and ignore me - your call.
What patches are you referring to? I can add that to the commit
message, sure.
In my mind, for starters what I mentioned in my response to patch 3 for
formatdomain.html.in changes.
Beyond that, I don't really don't have the patience to go through
hundreds of patches of history in o
Re: [libvirt] [PATCH v3 0/4] capabilities: Provide info about host IOMMU
On 05/31/2018 02:30 PM, Filip Alac wrote: > > Filip Alac (4): > virutil: Introduce virHostHasIOMMU > qemu: hostdev: Refactor code > capabilities: Extend capabilities with iommu_support > docs: news: Explain iommu_support improvement > > docs/news.xml | 8 + > docs/schemas/capability.rng | 13 + > src/conf/capabilities.c | 9 ++ > src/conf/capabilities.h | 3 ++ > src/libvirt_private.syms | 2 ++ > src/qemu/qemu_capabilities.c | 3 ++ > src/qemu/qemu_hostdev.c | 29 +++ > src/test/test_driver.c| 2 ++ > src/util/virutil.c| 28 ++ > src/util/virutil.h| 2 ++ > tests/qemucaps2xmldata/all_1.6.0-1.xml| 1 + > .../nodisksnapshot_1.6.0-1.xml| 1 + > .../vircaps2xmldata/vircaps-aarch64-basic.xml | 1 + > .../vircaps2xmldata/vircaps-x86_64-basic.xml | 1 + > .../vircaps2xmldata/vircaps-x86_64-caches.xml | 1 + > .../vircaps-x86_64-resctrl-cdp.xml| 1 + > .../vircaps-x86_64-resctrl-skx-twocaches.xml | 1 + > .../vircaps-x86_64-resctrl-skx.xml| 1 + > .../vircaps-x86_64-resctrl.xml| 1 + > 19 files changed, 83 insertions(+), 25 deletions(-) > All patches are missing S-o-B line. I have a special alias for that for instance. This is in my ~/.gitconfig: [alias] cs = commit --signoff so then I'm committing patches as: libvirt.git$ git cs -a S-o-B line is now required. And even for projects where it isn't just yet it doesn't hurt. Therefore it makes sense to have it as default. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/4] virutil: Introduce virHostHasIOMMU
On 05/31/2018 02:30 PM, Filip Alac wrote:
> ---
> src/conf/capabilities.c | 6 ++
> src/conf/capabilities.h | 3 +++
> src/util/virutil.c | 28
> src/util/virutil.h | 2 ++
> 4 files changed, 39 insertions(+)
It would be better if you'd merge this and next patch into one and call
it "Move out IOMMU detection into a separate function", or something
similar. The reason is that at no point the code is duplicated, where as
with this approach it is (even if for a short time).
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index dd2fc77f91..ba19d5db8c 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1743,3 +1743,9 @@ virCapabilitiesInitCaches(virCapsPtr caps)
> virBitmapFree(cpus);
> return ret;
> }
> +
> +void
> +virCapabilitiesHostInitIOMMU(virCapsPtr caps)
> +{
> +caps->host.iommu = virHostHasIOMMU();
> +}
This does not belong here. The proper order is to prepare everything and
introduce new feature after that. Imagine that virHostHasIOMMU() will be
used later, in a commit that fixes a bug. Now, downstream maintainers
would probably want to backport that particular fix and since it uses
virHostHasIOMMU() they will also backport this commit. But at the same
time they would be backporting a new feature which is something you
shouldn't do (the whole point of backporting commits is to stabilize
code base and introducing new features goes against that).
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index f0a06a24df..fe1b9ea455 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -183,6 +183,7 @@ struct _virCapsHost {
> int nPagesSize; /* size of pagesSize array */
> unsigned int *pagesSize;/* page sizes support on the system */
> unsigned char host_uuid[VIR_UUID_BUFLEN];
> +bool iommu;
> };
>
> typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
> @@ -327,4 +328,6 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr
> ptr);
>
> int virCapabilitiesInitCaches(virCapsPtr caps);
>
> +void virCapabilitiesHostInitIOMMU(virCapsPtr caps);
> +
> #endif /* __VIR_CAPABILITIES_H */
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index bb4474acd5..7edcda0ee7 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2090,3 +2090,31 @@ virMemoryMaxValue(bool capped)
> else
> return LLONG_MAX;
> }
> +
> +bool
> +virHostHasIOMMU(void)
> +{
> +DIR *iommuDir = NULL;
> +struct dirent *iommuGroup = NULL;
> +bool ret = false;
> +int direrr;
> +
> +/* condition 1 - /sys/kernel/iommu_groups/ contains entries */
> +if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0)
> +goto cleanup;
> +
> +while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) {
> +/* assume we found a group */
> +break;
> +}
> +
> +if (direrr < 0 || !iommuGroup)
> +goto cleanup;
> +/* okay, iommu is on and recognizes groups */
> +
> +ret = true;
> +
> + cleanup:
> +VIR_DIR_CLOSE(iommuDir);
> +return ret;
> +}
Since this is copied from qemuHostdevHostSupportsPassthroughVFIO() the
correct patch would be where you're moving this part out of there into here.
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index be0f6b0ea8..1ba9635bd9 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -216,6 +216,8 @@ unsigned long long virMemoryLimitTruncate(unsigned long
> long value);
> bool virMemoryLimitIsSet(unsigned long long value);
> unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
>
> +bool virHostHasIOMMU(void);
> +
This patch misses libvirt_private.syms adjustment. Again, long story
short, one patch needs to contain one semantic change. And looking into
next patch (where you have the syms change) does not follow the rule.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: hostdev: Refactor code
On 05/31/2018 02:30 PM, Filip Alac wrote:
> Make qemuHostdevHostSupportsPassthroughVFIO use
> virHostHasIOMMU.
>
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_hostdev.c | 29 -
> 2 files changed, 6 insertions(+), 25 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6001635916..99a14ab460 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -58,6 +58,7 @@ virCapabilitiesFreeMachines;
> virCapabilitiesFreeNUMAInfo;
> virCapabilitiesGetCpusForNodemask;
> virCapabilitiesGetNodeInfo;
> +virCapabilitiesHostInitIOMMU;
> virCapabilitiesHostSecModelAddBaseLabel;
> virCapabilitiesInitCaches;
> virCapabilitiesInitNUMA;
This does not belong here.
> @@ -3073,6 +3074,7 @@ virGetUserName;
> virGetUserRuntimeDirectory;
> virGetUserShell;
> virHexToBin;
> +virHostHasIOMMU;
> virIndexToDiskName;
> virIsDevMapperDevice;
> virIsSUID;
As I say in review to previous patch, this should go into 1/4.
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 955b5df1a3..25e2dcf868 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -23,7 +23,6 @@
>
> #include
>
> -#include
> #include
> #include
> #include
> @@ -124,33 +123,13 @@ qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr
> driver,
> bool
> qemuHostdevHostSupportsPassthroughVFIO(void)
> {
> -DIR *iommuDir = NULL;
> -struct dirent *iommuGroup = NULL;
> -bool ret = false;
> -int direrr;
> -
> -/* condition 1 - /sys/kernel/iommu_groups/ contains entries */
> -if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0)
> -goto cleanup;
> -
> -while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) {
> -/* assume we found a group */
> -break;
> -}
> -
> -if (direrr < 0 || !iommuGroup)
> -goto cleanup;
> -/* okay, iommu is on and recognizes groups */
> +if (!virHostHasIOMMU())
> +return false;
>
> -/* condition 2 - /dev/vfio/vfio exists */
> if (!virFileExists("/dev/vfio/vfio"))
> -goto cleanup;
> -
> -ret = true;
> +return false;
>
> - cleanup:
> -VIR_DIR_CLOSE(iommuDir);
> -return ret;
> +return true;
> }
>
>
>
This should be merged into 1/4. What seems to be correct patch
ordering/content:
1) move out parts of qemuHostdevHostSupportsPassthroughVFIO() into a
separate function (be it virHostHasIOMMU()), expose it in
libvirt_private.syms and corresponding header file.
2) Introduce caps->iommu (which is basically patch 3/4) witch all
changes you need (libvirt_private.syms change for
virCapabilitiesHostInitIOMMU(), adding bool to struct _virCapsHost, etc.)
3) document the feature in news.xml.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/13] qemu: hotplug: Refactor memory props formatting to qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:32PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 76 ++--- src/qemu/qemu_command.h | 2 +- src/qemu/qemu_hotplug.c | 8 ++ 3 files changed, 43 insertions(+), 43 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/13] qemu: hotplug: Refactor RNG props formatting to use qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:31PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 61 +++-- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 15 +++- 3 files changed, 27 insertions(+), 50 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] qemu: monitor: Add better APIs for adding of objects to qemu
On Wed, May 30, 2018 at 07:06:28PM +0200, Peter Krempa wrote: Use the new monitor command internal API to allow wrapping of the object name and alias into the JSON props so that they don't have to be passed out of band. The new API also takes a double pointer so that it can be cleared when the value is consumed so that it does not need to happen in every single caller. Signed-off-by: Peter Krempa --- src/qemu/qemu_monitor.c | 116 +-- src/qemu/qemu_monitor.h | 13 + src/qemu/qemu_monitor_json.c | 15 ++ src/qemu/qemu_monitor_json.h | 2 - 4 files changed, 129 insertions(+), 17 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/13] util: qemu: Introduce helper for formatting command line from new object props
On Wed, May 30, 2018 at 07:06:29PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/libvirt_private.syms | 1 + src/util/virqemu.c | 40 src/util/virqemu.h | 3 +++ 3 files changed, 40 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/13] qemu: hotplug: Refactor PR props formatting to use qemuMonitorCreateObjectProps
On Wed, May 30, 2018 at 07:06:30PM +0200, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_command.c | 30 ++ src/qemu/qemu_hotplug.c | 19 +++ 2 files changed, 17 insertions(+), 32 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/13] qemu: monitor: Rename qemuMonitorAddObject to qemuMonitorAddObjectType
On Wed, May 30, 2018 at 07:06:26PM +0200, Peter Krempa wrote: The function adds the object of a certain type. Change the name so that we make room for the generic function. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 50 - src/qemu/qemu_monitor.c | 10 +- src/qemu/qemu_monitor.h | 8 4 files changed, 35 insertions(+), 35 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] qemu: Rename virQEMUBuildObjectCommandlineFromJSON
On Wed, May 30, 2018 at 07:06:27PM +0200, Peter Krempa wrote: s/virQEMUBuildObjectCommandlineFromJSON/virQEMUBuildObjectCommandlineFromJSONType/ The function adds the object of a certain type. Change the name so that we make room for the generic function. Signed-off-by: Peter Krempa --- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 32 src/util/virqemu.c | 6 +++--- src/util/virqemu.h | 6 +++--- 4 files changed, 23 insertions(+), 23 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: hotplug: Fix detach of disk with managed persistent reservations
On 05/31/2018 03:26 PM, Peter Krempa wrote: > In commit 8bebb2b735d I've refactored how the detach of disk with a > managed persistent reservations object is handled. After the commit if > any disk with a managed PR object would be removed libvirt would also > attempt to remove the shared 'pr-manager-helper' object potentially used > by other disks. > > Thankfully this should not have practical impact as qemu should reject > deletion of the object if it was still used and the rest of the code is > correct. > > Fix this by removing the disk from the definition earlier and checking > if the shared/managed pr-manager-helper object is still needed. > > This basically splits the detach code for the managed PR object from the > unmanaged ones. The same separation will follow for the attachment code > as well as it greatly simplifies -blockdev support for this. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_hotplug.c | 29 ++--- > 1 file changed, 18 insertions(+), 11 deletions(-) ACK and SFF (safe for freeze). Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote:
> On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
> > On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
> > > On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> > > > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> > > > >
> > > > > This is way too sparse.
> > > > >
> > > > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > > > > >
> > > > > > Signed-off-by: Martin Kletzander
> > > > > > ---
> > > > > > src/qemu/qemu_command.c | 18
> > > > > > src/qemu/qemu_domain.c| 84
> > > > > > +++
> > > > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++
> > > > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +
> > > > > > tests/qemuxml2argvdata/tseg-i440fx.xml| 23 +
> > > > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +
> > > > > > .../tseg-old-machine-type.args| 27 ++
> > > > > > .../tseg-old-machine-type.xml | 21 +
> > > > > > tests/qemuxml2argvdata/tseg.args | 28 +++
> > > > > > tests/qemuxml2argvdata/tseg.xml | 21 +
> > > > > > tests/qemuxml2argvtest.c | 48 +++
> > > > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++
> > > > > > .../tseg-old-machine-type.xml | 44 ++
> > > > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++
> > > > > > tests/qemuxml2xmltest.c | 25 ++
> > > > > > 15 files changed, 505 insertions(+)
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > > > > create mode 100644
> > > > > > tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > > > > create mode 100644
> > > > > > tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
> > > >
> > > > [...]
> > > >
> > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > > > index 881d0ea46a75..3ea9e3d47344 100644
> > > > > > --- a/src/qemu/qemu_domain.c
> > > > > > +++ b/src/qemu/qemu_domain.c
> > > > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr
> > > > > > def)
> > > > > > }
> > > > > >
> > > > > >
> > > > > > +static int
> > > > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > > > > + virQEMUCapsPtr qemuCaps)
> > > > > > +{
> > > > > > +const char *machine = NULL;
> > > > > > +char *end_ptr = NULL;
> > > > > > +unsigned int major = 0;
> > > > > > +unsigned int minor = 0;
> > > > > > +
> > > > > > +def->tseg_size = 0;
> > > > >
> > > > > Pointless since the only way in here is "if (tseg_size == 0)"
> > > > >
> > > > > > +
> > > > > > +if (!qemuDomainIsQ35(def))
> > > > > > +return 0;
> > > > > > +
> > > > > > +if (!virQEMUCapsGet(qemuCaps,
> > > > > > QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > > > >
> > > > > Reading this now makes me realized _MBYTES is probably unnecessary,
> > > > > IDC
> > > > > though since it does follow the QEMU name.
> > > > >
> > > > > > +return 0;
> > > > > > +
> > > > > > +machine = STRSKIP(def->os.machine, "pc-q35-");
> > > > > > +
> > > > > > +if (!machine)
> > > > > > +goto error;
> > > > > > +
> > > > > > +if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > > > > +goto error;
> > > > > > +
> > > > > > +if (*end_ptr != '.')
> > > > > > +goto error;
> > > > > > +
> > > > > > +machine = end_ptr + 1;
> > > > > > +
> > > > > > +if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > > > > +goto error;
> > > > > > +if (*end_ptr != '\0')
> > > > > > +goto error;
> > > > > > +
> > > > > > +/* QEMU started defaulting to 16MiB after 2.9 */
> > > > > > +if (major > 2 || (major == 2 && minor > 9))
> > > > > > +def->tseg_size = 16 * 1024 * 1024;
> > > > >
> > > > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> > > > >
> > > > > This seems to me we are setting policy which based on history of many
> > > > > patches before is a no go. I'd say NAK to this, unless there is some
> > > > > convincing argument made in the commit message and followup responses
> >
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On 05/31/2018 03:24 AM, Martin Kletzander wrote:
> On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
>>
>> This is way too sparse.
>>
>
> I can't really think of what to say here that is not already mentioned
> in the
> documentation or self-explanatory in the code. But maybe only I see
> that as
> self-explanatory. I'll write something up here.
>
The whole default thing is what really drew my attention back to the
top... It's where I expected to see an explanation why the default was
being set. I think that's something that should be described - you may
disagree, but that's why it's a review.
[...]
See, I know how to cut - I just forgot in my response to patch 4 ;-)
I'll try not to overcut as I've seen done in for reviews I've received
which means I have to find the context of the feedback (we all have our
favorite things to gripe about).
>>> +
>>> +static void
>>> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
>>> + const virDomainDef *def)
>>> +{
>>> + if (!def->tseg_size)
>>
>> Since it's not bool or pointer, prefer the tseg_size == 0
>>
>
> I don't know if you mean that as indicative or imperative. It is very
> subjective and for such scenarios I would like to execute my right to
> mention
> that it is not part of Contributor Guidelines. I know it might seem
> rude, but
> this is one of the things that's very subjective and for such things I
> like to
> first reach a consensus before I change what I'm used to writing. I
> hope you
> understand that.
>
There's a lot of things not specifically listed in the CG that are
mentioned in many reviews or that are just now done because they have
been mentioned (2 blank lines between functions, formatting of function
headers, 1 variable per line, etc.). When I see this style and remember
to note it, I do... And I have see others mention it too. Many times
it's for enum comparisons.
In the long run, whatever. It's a style preference that denotes variable
usage for those reading code "in the future". We don't have a rule for
it, so go with your style if that's what you prefer.
FWIW: back in the dark ages when I first started doing this... Something
like the above for an integer, long, etc. value would be converted by
the compiler to checking *only* if the low bit was set/cleared (think
little endian) because that instruction was really quick. Try to find
that needle in a haystack for each even value that could be used ;-). I
got very used to the explicit comparison to 0 just to be 100% clear.
>>> + return;
>>> +
>>> + virCommandAddArg(cmd, "-global");
>>> +
[...]
>>> + /* QEMU started defaulting to 16MiB after 2.9 */
>>> + if (major > 2 || (major == 2 && minor > 9))
>>> + def->tseg_size = 16 * 1024 * 1024;
>>
>> So, if QEMU defaults to 16MiB, then why do we need so set this at all?
>>
>
> TL;DR because not setting the default value when it is not explicitly
> requested has already bitten us in the back multiple times.
>
> Long story short this way we are safe. No matter what happens (QEMU
> changing the default, the user changing the config somewhere else and
> not expecting this to change, us wanting to change the default in the
> future for some reason, migration to newer libvirt where who-knows-what
> is checked there, etc.) it is way easier to keep the guest ABI stable.
> It is visible what the value actually is and we're not hiding it
> somewhere in the code of the hypervisor. I know we don't do that for
> many things. I could've gone with the (often alibistic [1]) "the
> default is left for hypervisor to decide", but since we have the
> opportunity tomake things right (thanks to Laszlo's explanations), why
> shouldn't we?
>
>> This seems to me we are setting policy which based on history of many
>> patches before is a no go. I'd say NAK to this, unless there is some
>> convincing argument made in the commit message and followup responses to
>> the series (or you can take Jan's R-By and ignore me - your call.
>>
>
> What patches are you referring to? I can add that to the commit
> message, sure.
>
In my mind, for starters what I mentioned in my response to patch 3 for
formatdomain.html.in changes.
Beyond that, I don't really don't have the patience to go through
hundreds of patches of history in order to pick out ones that were
NACK'd or requested to be changed because the patch made some policy
decision or a let's set a default value type decision. I know they've
happened though.
TL;DR: I'm still of the opinion we don't set (or even save) a default.
I would think someone that failed to boot their guest because the memory
size or vCPU count was too large for smm/tseg would then be pointed to
this value which they would then optionally add and then "manage" going
forward.
I still don't see the point of setting a default if the default for QEMU
has been good enough up to now up to and including a certain memory size
of number of vCPU's, then how does us setting that
Re: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Thu, May 31, 2018 at 03:12:17PM +0200, Pavel Hrdina wrote:
> On Thu, May 31, 2018 at 02:50:31PM +0200, Erik Skultety wrote:
> > On Wed, May 30, 2018 at 02:30:04AM +0530, Sukrit Bhatnagar wrote:
> > > New macros are added to src/util/viralloc.h which help in
> > > adding cleanup attribute to variable declarations.
> > >
> > > Signed-off-by: Sukrit Bhatnagar
> > > ---
> > > src/util/viralloc.h | 69
> > > +
> > > 1 file changed, 69 insertions(+)
> > >
> > > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > > index 69d0f90..e1c31c3 100644
> > > --- a/src/util/viralloc.h
> > > +++ b/src/util/viralloc.h
> > > @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> > > int virAllocTestCount(void);
> > > void virAllocTestOOM(int n, int m);
> > > void virAllocTestHook(void (*func)(int, void*), void *data);
> > > +
> > > +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > > +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> > > +
> > > +/**
> > > + * VIR_DEFINE_AUTOPTR_FUNC:
> > > + * @type: type of the variables(s) to free automatically
> > > + * @func: cleanup function to be automatically called
> > > + *
> > > + * Thos macro defines a function for automatically freeing
> >
> > s/Thos/This
> > s/automatically freeing resources/automatic freeing of resources
> >
> > > + * resources allocated to a variable of type @type. The newly
> > > + * defined function calls the corresponding pre-defined
> > > + * function @func.
> > > + */
> > > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > > +{ \
> > > +(func)(*_ptr); \
> > > +} \
> > > +
> > > +/**
> > > + * VIR_DEFINE_AUTOCLEAR_FUNC:
> > > + * @type: type of the variables(s) to free automatically
> > > + * @func: cleanup function to be automatically called
> > > + *
> > > + * This macro defines a function for automatically clearing
> >
> > same minor nit as above...
> >
> > > + * a variable of type @type. The newly deifined function calls
> > > + * the corresponding pre-defined function @func.
> > > + */
> > > +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > > +static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > > +{ \
> > > +(func)(_ptr); \
> > > +} \
> > > +
> > > +/**
> > > + * VIR_AUTOFREE:
> > > + * @type: type of the variables(s) to free automatically
> > > + *
> > > + * Macro to automatically free the memory allocated to
> > > + * the variable(s) declared with it by calling virFree
> > > + * when the variable goes out of scope.
> > > + */
> > > +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > > +
> > > +/**
> > > + * VIR_AUTOPTR:
> > > + * @type: type of the variables(s) to free automatically
> > > + *
> > > + * Macro to automatically free the memory allocated to
> > > + * the variable(s) declared with it by calling the function
> > > + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> > > + * goes out of scope.
> > > + */
> > > +# define VIR_AUTOPTR(type) \
> > > +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
> > > +
> > > +/**
> > > + * VIR_AUTOCLEAR:
> > > + * @type: type of the variables(s) to free automatically
> > > + *
> > > + * Macro to automatically clear the variable(s) declared
> > > + * with it by calling the function defined by
> > > + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
> >
> > s/*/e
> >
> > > + * of scope.
> > > + */
> > > +# define VIR_AUTOCLEAR(type) \
> > > +__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
> >
> > I don't see any functional problems here, I like the changes, however, if
> > we go
> > ahead with merging any future patch sets, I'd probably like to see them to
> > follow the steps we discussed earlier (what the initial focus should be)
> > since,
> > subjectively, it poses a better logical order:
> >
> > 1 patch series
> > 1) Introduce VIR_AUTOFREE macro
> > 2) use it at as many modules and places as possible (doesn't need to 100%,
> > since there probably be a few caveats)
> >
> > another patch series
> > 3) Introduce VIR_AUTOPTR and friends
> > 4) use those in as many places as the simple straightforward replacement
> > goes
> >
> > another patch series
> > 5) Introduce VIR_AUTOCLEAR and friends
> > 6) use those
>
> I don't share the same view, this would result in three huge patch
> series and it would be annoying to review it.
Yeah, I elaborated on my vision more in the last patch, I understand that it's
not visible immediately from this response, my bad, this was just supposed to
be a general idea, we'll of course need some granularity, noone wants to review
such a beast.
>
> IMHO introducing all of these in a single patch make sense and after
> that we should follow with incremental switch to these macros ideally
> having one patch per file. Some parts of the existing code would need
> some modification before we can use these ma
[libvirt] [PATCH] qemu: hotplug: Fix detach of disk with managed persistent reservations
In commit 8bebb2b735d I've refactored how the detach of disk with a
managed persistent reservations object is handled. After the commit if
any disk with a managed PR object would be removed libvirt would also
attempt to remove the shared 'pr-manager-helper' object potentially used
by other disks.
Thankfully this should not have practical impact as qemu should reject
deletion of the object if it was still used and the rest of the code is
correct.
Fix this by removing the disk from the definition earlier and checking
if the shared/managed pr-manager-helper object is still needed.
This basically splits the detach code for the managed PR object from the
unmanaged ones. The same separation will follow for the attachment code
as well as it greatly simplifies -blockdev support for this.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_hotplug.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b4bbe62c75..a14281203a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3839,6 +3839,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
char *drivestr;
char *objAlias = NULL;
char *encAlias = NULL;
+bool prManaged = priv->prDaemonRunning;
+bool prUsed = false;
VIR_DEBUG("Removing disk %s from domain %p %s",
disk->info.alias, vm, vm->def->name);
@@ -3876,6 +3878,16 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
}
}
+for (i = 0; i < vm->def->ndisks; i++) {
+if (vm->def->disks[i] == disk) {
+virDomainDiskRemove(vm->def, i);
+break;
+}
+}
+
+/* check if the last disk with managed PR was just removed */
+prUsed = virDomainDefHasManagedPR(vm->def);
+
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivestr);
@@ -3892,12 +3904,16 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
VIR_FREE(encAlias);
/* If it fails, then so be it - it was a best shot */
-if (disk->src->pr)
+if (disk->src->pr &&
+!virStoragePRDefIsManaged(disk->src->pr))
ignore_value(qemuMonitorDelObject(priv->mon, disk->src->pr->mgralias));
if (disk->src->haveTLS)
ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
+if (prManaged && !prUsed)
+ignore_value(qemuMonitorDelObject(priv->mon,
qemuDomainGetManagedPRAlias()));
+
if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
@@ -3906,16 +3922,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias);
qemuDomainEventQueue(driver, event);
-for (i = 0; i < vm->def->ndisks; i++) {
-if (vm->def->disks[i] == disk) {
-virDomainDiskRemove(vm->def, i);
-break;
-}
-}
-
-/* check if the last disk with managed PR was just removed */
-if (priv->prDaemonRunning &&
-!virDomainDefHasManagedPR(vm->def))
+if (prManaged && !prUsed)
qemuProcessKillManagedPRDaemon(vm);
qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Thu, May 31, 2018 at 02:50:31PM +0200, Erik Skultety wrote:
> On Wed, May 30, 2018 at 02:30:04AM +0530, Sukrit Bhatnagar wrote:
> > New macros are added to src/util/viralloc.h which help in
> > adding cleanup attribute to variable declarations.
> >
> > Signed-off-by: Sukrit Bhatnagar
> > ---
> > src/util/viralloc.h | 69
> > +
> > 1 file changed, 69 insertions(+)
> >
> > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > index 69d0f90..e1c31c3 100644
> > --- a/src/util/viralloc.h
> > +++ b/src/util/viralloc.h
> > @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> > int virAllocTestCount(void);
> > void virAllocTestOOM(int n, int m);
> > void virAllocTestHook(void (*func)(int, void*), void *data);
> > +
> > +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> > +
> > +/**
> > + * VIR_DEFINE_AUTOPTR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * Thos macro defines a function for automatically freeing
>
> s/Thos/This
> s/automatically freeing resources/automatic freeing of resources
>
> > + * resources allocated to a variable of type @type. The newly
> > + * defined function calls the corresponding pre-defined
> > + * function @func.
> > + */
> > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > +{ \
> > +(func)(*_ptr); \
> > +} \
> > +
> > +/**
> > + * VIR_DEFINE_AUTOCLEAR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * This macro defines a function for automatically clearing
>
> same minor nit as above...
>
> > + * a variable of type @type. The newly deifined function calls
> > + * the corresponding pre-defined function @func.
> > + */
> > +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > +static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > +{ \
> > +(func)(_ptr); \
> > +} \
> > +
> > +/**
> > + * VIR_AUTOFREE:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling virFree
> > + * when the variable goes out of scope.
> > + */
> > +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > +
> > +/**
> > + * VIR_AUTOPTR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling the function
> > + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> > + * goes out of scope.
> > + */
> > +# define VIR_AUTOPTR(type) \
> > +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
> > +
> > +/**
> > + * VIR_AUTOCLEAR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically clear the variable(s) declared
> > + * with it by calling the function defined by
> > + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
>
> s/*/e
>
> > + * of scope.
> > + */
> > +# define VIR_AUTOCLEAR(type) \
> > +__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
>
> I don't see any functional problems here, I like the changes, however, if we
> go
> ahead with merging any future patch sets, I'd probably like to see them to
> follow the steps we discussed earlier (what the initial focus should be)
> since,
> subjectively, it poses a better logical order:
>
> 1 patch series
> 1) Introduce VIR_AUTOFREE macro
> 2) use it at as many modules and places as possible (doesn't need to 100%,
> since there probably be a few caveats)
>
> another patch series
> 3) Introduce VIR_AUTOPTR and friends
> 4) use those in as many places as the simple straightforward replacement goes
>
> another patch series
> 5) Introduce VIR_AUTOCLEAR and friends
> 6) use those
I don't share the same view, this would result in three huge patch
series and it would be annoying to review it.
IMHO introducing all of these in a single patch make sense and after
that we should follow with incremental switch to these macros ideally
having one patch per file. Some parts of the existing code would need
some modification before we can use these macros which would be done
separately from the one patch that implements the usage of these macros.
Switching majority of libvirt code into this new concept is dangerous
since it may introduce some issues or bugs and having small changes
makes it easier to figure out what cause the issue.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 4/4] add automatic cleanup support in src/util/virauth.c
On Wed, May 30, 2018 at 02:30:07AM +0530, Sukrit Bhatnagar wrote:
> Define a new cleanup function for virAuthConfigPtr in
> src/util/virauthconfig.h.
> Modifiy code to use cleanup macros where required.
s/fiy/fy
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virauth.c | 66
> ++--
> src/util/virauthconfig.h | 3 +++
> 2 files changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/src/util/virauth.c b/src/util/virauth.c
> index adb093e..aa7593c 100644
> --- a/src/util/virauth.c
> +++ b/src/util/virauth.c
> @@ -26,7 +26,6 @@
>
> #include "virauth.h"
> #include "virutil.h"
> -#include "viralloc.h"
> #include "virlog.h"
> #include "datatypes.h"
> #include "virerror.h"
> @@ -42,10 +41,9 @@ int
> virAuthGetConfigFilePathURI(virURIPtr uri,
> char **path)
> {
> -int ret = -1;
> size_t i;
> const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
> -char *userdir = NULL;
> +VIR_AUTOFREE(char *) userdir = NULL;
>
> *path = NULL;
>
> @@ -54,7 +52,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
> if (authenv) {
> VIR_DEBUG("Using path from env '%s'", authenv);
> if (VIR_STRDUP(*path, authenv) < 0)
> -goto cleanup;
> +return -1;
> return 0;
> }
>
> @@ -64,41 +62,38 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
> uri->params[i].value) {
> VIR_DEBUG("Using path from URI '%s'", uri->params[i].value);
> if (VIR_STRDUP(*path, uri->params[i].value) < 0)
> -goto cleanup;
> +return -1;
> return 0;
> }
> }
> }
>
> if (!(userdir = virGetUserConfigDirectory()))
> -goto cleanup;
> +return -1;
>
> if (virAsprintf(path, "%s/auth.conf", userdir) < 0)
> -goto cleanup;
> +return -1;
>
> VIR_DEBUG("Checking for readability of '%s'", *path);
> -if (access(*path, R_OK) == 0)
> -goto done;
> +if (access(*path, R_OK) == 0) {
> +VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
> +return 0;
> +}
>
> VIR_FREE(*path);
>
> if (VIR_STRDUP(*path, SYSCONFDIR "/libvirt/auth.conf") < 0)
> -goto cleanup;
> +return -1;
>
> VIR_DEBUG("Checking for readability of '%s'", *path);
> -if (access(*path, R_OK) == 0)
> -goto done;
> +if (access(*path, R_OK) == 0) {
> +VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
> +return 0;
> +}
>
> VIR_FREE(*path);
>
> - done:
> -ret = 0;
> -
> -VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
> - cleanup:
> -VIR_FREE(userdir);
> -
> -return ret;
> +return 0;
> }
>
>
> @@ -117,8 +112,7 @@ virAuthGetCredential(const char *servicename,
> const char *path,
> char **value)
> {
> -int ret = -1;
> -virAuthConfigPtr config = NULL;
> +VIR_AUTOPTR(virAuthConfigPtr) config = NULL;
As I said in patch 1, changes related to VIR_AUTOPTR should be a separate patch
series, IOW the patches shouldn't combine changing both VIR_AUTOFREE and
VIR_AUTOPTR.
Otherwise, the changes look good, looking forward to more files. As we
discussed privately, having a series addressing src/util then e.g. src/conf as
separate patch series which can be merged gradually is IMHO better than trying
to make all the VIR_FREE changes everywhere, then sending a massive series
consisting of tens of patches and then doing the same for VIR_AUTOPTR.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 3/4] add automatic cleanup support in src/util/virauthconfig.c
On Wed, May 30, 2018 at 02:30:06AM +0530, Sukrit Bhatnagar wrote:
> Modifiy code to use cleanup macros where required.
s/fiy/fy
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virauthconfig.c | 34 --
> 1 file changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
> index 91c9c0c..66f7f7e 100644
> --- a/src/util/virauthconfig.c
> +++ b/src/util/virauthconfig.c
> @@ -106,10 +106,9 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
> const char *credname,
> const char **value)
> {
> -char *authgroup = NULL;
> -char *credgroup = NULL;
> +VIR_AUTOFREE(char *) authgroup = NULL;
> +VIR_AUTOFREE(char *) credgroup = NULL;
> const char *authcred;
> -int ret = -1;
>
> *value = NULL;
>
> @@ -119,47 +118,38 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
> hostname = "localhost";
>
> if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname) < 0)
> -goto cleanup;
> +return -1;
>
> if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
> VIR_FREE(authgroup);
> if (virAsprintf(&authgroup, "auth-%s-%s", service, "default") < 0)
> - goto cleanup;
> + return -1;
> }
>
> -if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
> -ret = 0;
> -goto cleanup;
> -}
> +if (!virKeyFileHasGroup(auth->keyfile, authgroup))
> +return 0;
>
> if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup,
> "credentials"))) {
> virReportError(VIR_ERR_CONF_SYNTAX,
> _("Missing item 'credentials' in group '%s' in '%s'"),
> authgroup, auth->path);
> -goto cleanup;
> +return -1;
> }
>
> if (virAsprintf(&credgroup, "credentials-%s", authcred) < 0)
> -goto cleanup;
> +return -1;
>
> if (!virKeyFileHasGroup(auth->keyfile, credgroup)) {
> virReportError(VIR_ERR_CONF_SYNTAX,
> _("Missing group 'credentials-%s' referenced from
> group '%s' in '%s'"),
> authcred, authgroup, auth->path);
> -goto cleanup;
> +return -1;
> }
>
> -if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) {
> -ret = 0;
> -goto cleanup;
> -}
> +if (!virKeyFileHasValue(auth->keyfile, credgroup, credname))
> +return 0;
>
> *value = virKeyFileGetValueString(auth->keyfile, credgroup, credname);
>
> -ret = 0;
> -
> - cleanup:
> -VIR_FREE(authgroup);
> -VIR_FREE(credgroup);
> -return ret;
> +return 0;
> }
Looks fine as well.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 2/4] add automatic cleanup support in src/util/virarptable.c
On Wed, May 30, 2018 at 02:30:05AM +0530, Sukrit Bhatnagar wrote:
> Modifiy code to use cleanup macros where required.
s/fiy/fy
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/virarptable.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virarptable.c b/src/util/virarptable.c
> index c0e90dc..f53a479 100644
> --- a/src/util/virarptable.c
> +++ b/src/util/virarptable.c
> @@ -71,9 +71,8 @@ virArpTableGet(void)
> {
> int num = 0;
> int msglen;
> -void *nlData = NULL;
> +VIR_AUTOFREE(void *) nlData = NULL;
> virArpTablePtr table = NULL;
> -char *ipstr = NULL;
> struct nlmsghdr* nh;
> struct rtattr * tb[NDA_MAX+1];
>
> @@ -89,6 +88,7 @@ virArpTableGet(void)
> VIR_WARNINGS_NO_CAST_ALIGN
> for (; NLMSG_OK(nh, msglen); nh = NLMSG_NEXT(nh, msglen)) {
> VIR_WARNINGS_RESET
> +VIR_AUTOFREE(char *) ipstr = NULL;
> struct ndmsg *r = NLMSG_DATA(nh);
> int len = nh->nlmsg_len;
> void *addr;
> @@ -134,8 +134,6 @@ virArpTableGet(void)
>
> if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
> goto cleanup;
> -
> -VIR_FREE(ipstr);
> }
>
> if (tb[NDA_LLADDR]) {
> @@ -155,13 +153,10 @@ virArpTableGet(void)
> }
>
> end_of_netlink_messages:
> -VIR_FREE(nlData);
> return table;
>
> cleanup:
> virArpTableFree(table);
> -VIR_FREE(ipstr);
> -VIR_FREE(nlData);
> return NULL;
> }
Looks good.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Wed, May 30, 2018 at 02:30:04AM +0530, Sukrit Bhatnagar wrote:
> New macros are added to src/util/viralloc.h which help in
> adding cleanup attribute to variable declarations.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/viralloc.h | 69
> +
> 1 file changed, 69 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 69d0f90..e1c31c3 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> int virAllocTestCount(void);
> void virAllocTestOOM(int n, int m);
> void virAllocTestHook(void (*func)(int, void*), void *data);
> +
> +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> +
> +/**
> + * VIR_DEFINE_AUTOPTR_FUNC:
> + * @type: type of the variables(s) to free automatically
> + * @func: cleanup function to be automatically called
> + *
> + * Thos macro defines a function for automatically freeing
s/Thos/This
s/automatically freeing resources/automatic freeing of resources
> + * resources allocated to a variable of type @type. The newly
> + * defined function calls the corresponding pre-defined
> + * function @func.
> + */
> +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> +{ \
> +(func)(*_ptr); \
> +} \
> +
> +/**
> + * VIR_DEFINE_AUTOCLEAR_FUNC:
> + * @type: type of the variables(s) to free automatically
> + * @func: cleanup function to be automatically called
> + *
> + * This macro defines a function for automatically clearing
same minor nit as above...
> + * a variable of type @type. The newly deifined function calls
> + * the corresponding pre-defined function @func.
> + */
> +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> +static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> +{ \
> +(func)(_ptr); \
> +} \
> +
> +/**
> + * VIR_AUTOFREE:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically free the memory allocated to
> + * the variable(s) declared with it by calling virFree
> + * when the variable goes out of scope.
> + */
> +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> +
> +/**
> + * VIR_AUTOPTR:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically free the memory allocated to
> + * the variable(s) declared with it by calling the function
> + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> + * goes out of scope.
> + */
> +# define VIR_AUTOPTR(type) \
> +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
> +
> +/**
> + * VIR_AUTOCLEAR:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically clear the variable(s) declared
> + * with it by calling the function defined by
> + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
s/*/e
> + * of scope.
> + */
> +# define VIR_AUTOCLEAR(type) \
> +__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
I don't see any functional problems here, I like the changes, however, if we go
ahead with merging any future patch sets, I'd probably like to see them to
follow the steps we discussed earlier (what the initial focus should be) since,
subjectively, it poses a better logical order:
1 patch series
1) Introduce VIR_AUTOFREE macro
2) use it at as many modules and places as possible (doesn't need to 100%,
since there probably be a few caveats)
another patch series
3) Introduce VIR_AUTOPTR and friends
4) use those in as many places as the simple straightforward replacement goes
another patch series
5) Introduce VIR_AUTOCLEAR and friends
6) use those
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/4] qemu: hostdev: Refactor code
Make qemuHostdevHostSupportsPassthroughVFIO use
virHostHasIOMMU.
---
src/libvirt_private.syms | 2 ++
src/qemu/qemu_hostdev.c | 29 -
2 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6001635916..99a14ab460 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -58,6 +58,7 @@ virCapabilitiesFreeMachines;
virCapabilitiesFreeNUMAInfo;
virCapabilitiesGetCpusForNodemask;
virCapabilitiesGetNodeInfo;
+virCapabilitiesHostInitIOMMU;
virCapabilitiesHostSecModelAddBaseLabel;
virCapabilitiesInitCaches;
virCapabilitiesInitNUMA;
@@ -3073,6 +3074,7 @@ virGetUserName;
virGetUserRuntimeDirectory;
virGetUserShell;
virHexToBin;
+virHostHasIOMMU;
virIndexToDiskName;
virIsDevMapperDevice;
virIsSUID;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 955b5df1a3..25e2dcf868 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -23,7 +23,6 @@
#include
-#include
#include
#include
#include
@@ -124,33 +123,13 @@ qemuHostdevUpdateActiveDomainDevices(virQEMUDriverPtr
driver,
bool
qemuHostdevHostSupportsPassthroughVFIO(void)
{
-DIR *iommuDir = NULL;
-struct dirent *iommuGroup = NULL;
-bool ret = false;
-int direrr;
-
-/* condition 1 - /sys/kernel/iommu_groups/ contains entries */
-if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0)
-goto cleanup;
-
-while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) {
-/* assume we found a group */
-break;
-}
-
-if (direrr < 0 || !iommuGroup)
-goto cleanup;
-/* okay, iommu is on and recognizes groups */
+if (!virHostHasIOMMU())
+return false;
-/* condition 2 - /dev/vfio/vfio exists */
if (!virFileExists("/dev/vfio/vfio"))
-goto cleanup;
-
-ret = true;
+return false;
- cleanup:
-VIR_DIR_CLOSE(iommuDir);
-return ret;
+return true;
}
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/4] virutil: Introduce virHostHasIOMMU
---
src/conf/capabilities.c | 6 ++
src/conf/capabilities.h | 3 +++
src/util/virutil.c | 28
src/util/virutil.h | 2 ++
4 files changed, 39 insertions(+)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index dd2fc77f91..ba19d5db8c 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1743,3 +1743,9 @@ virCapabilitiesInitCaches(virCapsPtr caps)
virBitmapFree(cpus);
return ret;
}
+
+void
+virCapabilitiesHostInitIOMMU(virCapsPtr caps)
+{
+caps->host.iommu = virHostHasIOMMU();
+}
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index f0a06a24df..fe1b9ea455 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -183,6 +183,7 @@ struct _virCapsHost {
int nPagesSize; /* size of pagesSize array */
unsigned int *pagesSize;/* page sizes support on the system */
unsigned char host_uuid[VIR_UUID_BUFLEN];
+bool iommu;
};
typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
@@ -327,4 +328,6 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr);
int virCapabilitiesInitCaches(virCapsPtr caps);
+void virCapabilitiesHostInitIOMMU(virCapsPtr caps);
+
#endif /* __VIR_CAPABILITIES_H */
diff --git a/src/util/virutil.c b/src/util/virutil.c
index bb4474acd5..7edcda0ee7 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2090,3 +2090,31 @@ virMemoryMaxValue(bool capped)
else
return LLONG_MAX;
}
+
+bool
+virHostHasIOMMU(void)
+{
+DIR *iommuDir = NULL;
+struct dirent *iommuGroup = NULL;
+bool ret = false;
+int direrr;
+
+/* condition 1 - /sys/kernel/iommu_groups/ contains entries */
+if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0)
+goto cleanup;
+
+while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) {
+/* assume we found a group */
+break;
+}
+
+if (direrr < 0 || !iommuGroup)
+goto cleanup;
+/* okay, iommu is on and recognizes groups */
+
+ret = true;
+
+ cleanup:
+VIR_DIR_CLOSE(iommuDir);
+return ret;
+}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index be0f6b0ea8..1ba9635bd9 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -216,6 +216,8 @@ unsigned long long virMemoryLimitTruncate(unsigned long
long value);
bool virMemoryLimitIsSet(unsigned long long value);
unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
+bool virHostHasIOMMU(void);
+
/**
* VIR_ASSIGN_IS_OVERFLOW:
* @rvalue: value that is checked (evaluated twice)
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 3/4] capabilities: Extend capabilities with iommu_support
---
docs/schemas/capability.rng | 13 +
src/conf/capabilities.c | 3 +++
src/qemu/qemu_capabilities.c| 3 +++
src/test/test_driver.c | 2 ++
tests/qemucaps2xmldata/all_1.6.0-1.xml | 1 +
tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml | 1 +
tests/vircaps2xmldata/vircaps-aarch64-basic.xml | 1 +
tests/vircaps2xmldata/vircaps-x86_64-basic.xml | 1 +
tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 1 +
.../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 1 +
.../vircaps-x86_64-resctrl-skx-twocaches.xml| 1 +
.../vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml | 1 +
tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml| 1 +
13 files changed, 30 insertions(+)
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 66c5de62e5..52164d5ecb 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -39,6 +39,9 @@
+
+
+
@@ -155,6 +158,16 @@
+
+
+
+
+
+
+
+
+
+
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index ba19d5db8c..0de1440349 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1025,6 +1025,9 @@ virCapabilitiesFormatXML(virCapsPtr caps)
virBufferAddLit(&buf, "\n");
}
+virBufferAsprintf(&buf, "\n",
+ caps->host.iommu ? "yes" : "no");
+
if (caps->host.offlineMigrate) {
virBufferAddLit(&buf, "\n");
virBufferAdjustIndent(&buf, 2);
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e2e76e4dd8..9b5423944b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -948,6 +948,9 @@ virQEMUCapsInit(virFileCachePtr cache)
if (virNodeSuspendGetTargetMask(&caps->host.powerMgmt) < 0)
VIR_WARN("Failed to get host power management capabilities");
+/* Add IOMMU info */
+virCapabilitiesHostInitIOMMU(caps);
+
/* Add huge pages info */
if (virCapabilitiesInitPages(caps) < 0)
VIR_WARN("Failed to get pages info");
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index a43b9781eb..89121d4220 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -322,6 +322,8 @@ testBuildCapabilities(virConnectPtr conn)
if (virCapabilitiesAddHostFeature(caps, "nonpae") < 0)
goto error;
+virCapabilitiesHostInitIOMMU(caps);
+
if (VIR_ALLOC_N(caps->host.pagesSize, 4) < 0)
goto error;
diff --git a/tests/qemucaps2xmldata/all_1.6.0-1.xml
b/tests/qemucaps2xmldata/all_1.6.0-1.xml
index 84d60008d8..efe86b9a12 100644
--- a/tests/qemucaps2xmldata/all_1.6.0-1.xml
+++ b/tests/qemucaps2xmldata/all_1.6.0-1.xml
@@ -5,6 +5,7 @@
i686
+
diff --git a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
index 28762c263b..981344e6fd 100644
--- a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
+++ b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
@@ -5,6 +5,7 @@
i686
+
diff --git a/tests/vircaps2xmldata/vircaps-aarch64-basic.xml
b/tests/vircaps2xmldata/vircaps-aarch64-basic.xml
index ce156a364e..50466f9162 100644
--- a/tests/vircaps2xmldata/vircaps-aarch64-basic.xml
+++ b/tests/vircaps2xmldata/vircaps-aarch64-basic.xml
@@ -5,6 +5,7 @@
aarch64
+
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-basic.xml
b/tests/vircaps2xmldata/vircaps-x86_64-basic.xml
index 1f2c6659a5..e7be6def3e 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-basic.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-basic.xml
@@ -5,6 +5,7 @@
x86_64
+
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
index 0c6f3769a2..ca671a1640 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
@@ -5,6 +5,7 @@
x86_64
+
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
index 443917c62d..1d3df318c5 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml
@@ -5,6 +5,7 @@
x86_64
+
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx-twocaches.xml
b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx-twocaches.xml
index d18665b24f..44c1042afe 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx-twocaches.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-resctrl-skx-twocaches.xml
@@ -5,6 +5,7 @@
x86_64
+
diff --
[libvirt] [PATCH v3 0/4] capabilities: Provide info about host IOMMU
Filip Alac (4): virutil: Introduce virHostHasIOMMU qemu: hostdev: Refactor code capabilities: Extend capabilities with iommu_support docs: news: Explain iommu_support improvement docs/news.xml | 8 + docs/schemas/capability.rng | 13 + src/conf/capabilities.c | 9 ++ src/conf/capabilities.h | 3 ++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_capabilities.c | 3 ++ src/qemu/qemu_hostdev.c | 29 +++ src/test/test_driver.c| 2 ++ src/util/virutil.c| 28 ++ src/util/virutil.h| 2 ++ tests/qemucaps2xmldata/all_1.6.0-1.xml| 1 + .../nodisksnapshot_1.6.0-1.xml| 1 + .../vircaps2xmldata/vircaps-aarch64-basic.xml | 1 + .../vircaps2xmldata/vircaps-x86_64-basic.xml | 1 + .../vircaps2xmldata/vircaps-x86_64-caches.xml | 1 + .../vircaps-x86_64-resctrl-cdp.xml| 1 + .../vircaps-x86_64-resctrl-skx-twocaches.xml | 1 + .../vircaps-x86_64-resctrl-skx.xml| 1 + .../vircaps-x86_64-resctrl.xml| 1 + 19 files changed, 83 insertions(+), 25 deletions(-) -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 4/4] docs: news: Explain iommu_support improvement
--- docs/news.xml | 8 1 file changed, 8 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index c45850f625..b5ac50f3a1 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -123,6 +123,14 @@ secret-event, pool-event and nodedev-event) + + + capabilities: Provide info about host IOMMU support + + + Capabilities XML now provide information about host IOMMU support. + + -- 2.17.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> >
> > This is way too sparse.
> >
> > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > >
> > > Signed-off-by: Martin Kletzander
> > > ---
> > > src/qemu/qemu_command.c | 18
> > > src/qemu/qemu_domain.c| 84 +++
> > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++
> > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +
> > > tests/qemuxml2argvdata/tseg-i440fx.xml| 23 +
> > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +
> > > .../tseg-old-machine-type.args| 27 ++
> > > .../tseg-old-machine-type.xml | 21 +
> > > tests/qemuxml2argvdata/tseg.args | 28 +++
> > > tests/qemuxml2argvdata/tseg.xml | 21 +
> > > tests/qemuxml2argvtest.c | 48 +++
> > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++
> > > .../tseg-old-machine-type.xml | 44 ++
> > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++
> > > tests/qemuxml2xmltest.c | 25 ++
> > > 15 files changed, 505 insertions(+)
> > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
>
> [...]
>
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 881d0ea46a75..3ea9e3d47344 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > > }
> > >
> > >
> > > +static int
> > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > + virQEMUCapsPtr qemuCaps)
> > > +{
> > > +const char *machine = NULL;
> > > +char *end_ptr = NULL;
> > > +unsigned int major = 0;
> > > +unsigned int minor = 0;
> > > +
> > > +def->tseg_size = 0;
> >
> > Pointless since the only way in here is "if (tseg_size == 0)"
> >
> > > +
> > > +if (!qemuDomainIsQ35(def))
> > > +return 0;
> > > +
> > > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> >
> > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> > though since it does follow the QEMU name.
> >
> > > +return 0;
> > > +
> > > +machine = STRSKIP(def->os.machine, "pc-q35-");
> > > +
> > > +if (!machine)
> > > +goto error;
> > > +
> > > +if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > +goto error;
> > > +
> > > +if (*end_ptr != '.')
> > > +goto error;
> > > +
> > > +machine = end_ptr + 1;
> > > +
> > > +if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > +goto error;
> > > +if (*end_ptr != '\0')
> > > +goto error;
> > > +
> > > +/* QEMU started defaulting to 16MiB after 2.9 */
> > > +if (major > 2 || (major == 2 && minor > 9))
> > > +def->tseg_size = 16 * 1024 * 1024;
> >
> > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> >
> > This seems to me we are setting policy which based on history of many
> > patches before is a no go. I'd say NAK to this, unless there is some
> > convincing argument made in the commit message and followup responses to
> > the series (or you can take Jan's R-By and ignore me - your call.
>
> I agree with John, this whole function should be removed, we don't have
> to set the default in config XML. However we should record that default
> value into live/status XML to make sure that it will not be changed
> during migration and to let the user know what is the default value.
>
That doesn't make sense. This is part of guest ABI and if you want to be on the
safe side when migrating, then you should be way more cautious with the
stop/start scenario. Migration will probably fail (or silently corrupt data,
but I believe that wouldn't happen), but stop/start without keeping the default
will change that in case QEMU changes default and then the guest
Re: [libvirt] [PATCH 3/5] conf, schema, docs: Add support for TSEG size setting
On Wed, May 30, 2018 at 12:00:26PM -0400, John Ferlan wrote: On 05/21/2018 11:00 AM, Martin Kletzander wrote: TSEG (Top of Memory Segment) is one of many regions that SMM (System Management Mode) can occupy. This one, however is special, because a) most of the SMM code lives in TSEG nowadays and b) QEMU just (well, some time ago) added support for so called 'extended' TSEG. The difference to the TSEG implemented in real q35's MCH (Memory Controller Hub) is that it can have any size from 1 MiB up to 65534 MiB in 1 MiB increments. But more about that in the QEMU patch. ^^ Which some reader, but not this one, may be eager to find ;-) Still is there a valid range QEMU would accept? Or do we just let QEMU fail if the range is too high? I think QEMU has MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX Rather than promising some value, I adjusted it so that it is no longer false, no matter what the max is there. Signed-off-by: Martin Kletzander --- docs/formatdomain.html.in | 39 +++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 60 - src/conf/domain_conf.h | 1 + tests/genericxml2xmlindata/tseg.xml | 23 +++ tests/genericxml2xmltest.c | 2 + 6 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 tests/genericxml2xmlindata/tseg.xml In the category of I hate it when that happens, git am -3 "merged" in two chunks incorrectly! Probably wouldn't have happened if I'd done You can enable/disable 3-way merges if you do (not) like them. this sooner! The virDomainDefFeaturesCheckABIStability hunk got merged into virDomainRedirFilterDefCheckABIStability and the tseg grammar got merged under "vmport" and just before "gic". As you can imagine the results weren't pretty ;-). Yeah, happened to me as well, I should've resent this, but I forgot about the merge issue and I also wanted to show that this was posted way before the freeze. Anyway, it's pointless to force it now, I'll leave it for later (meaning "after the release"). Anyway, I keep my branches updated (every now and then) on my github repo [1], so if you want to check that, you always can. [1] https://github.com/nertpinx/libvirt diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 403b638bd4bd..39ebfe398bd7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1897,6 +1897,9 @@+ + ... @@ -2056,6 +2059,42 @@ off, default on) enable or disable System Management Mode. Since 2.1.0 + + Optional sub-element tseg can be used to specify the + amount of memory dedicated to SMM TSEG. The size can be specified as a + value of that element, optional attribute unit can be + used to specify the unit of the aforementioned value (defaults to + 'MiB'). + If this is to be a true paragraph break then these paragraphs needs to be wrapped in ... ; otherwise, this just becomes one long run on (and quite ugly IMO) paragraph. + This value is configurable due to the fact that the calculation cannot + be done right with the guarantee that it will work correctly. For + QEMU TSEG was disabled up to and including pc-q35-2.9 (it + does not make sense fo any other machine type than q35). s/fo/for/ This also appears to be a paragraph break... + From pc-q35-2.10 the default value was changed to 16 MiB. s/From/Starting with/ ??? (not required, just a though) + That should suffice for up to 272 VCPUs, 5 GiB guest RAM in total, no + hotplug memory range, and 32 GiB of 64-bit PCI MMIO aperture. Or for + 48 VCPUs, with 1TB of guest RAM, no hotplug DIMM range, and 32GB of + 64-bit PCI MMIO aperture. The values may also vary based on the loader + the VM is using. + + Additional size might be needed for significantly higher VCPU counts + or increased address space (that can be memory, maxMemory, 64-bit PCI + MMIO aperture size; roughly 8 MiB of TSEG per 1 TiB of address space) + which can also be rounded up. Uh, oh, hmmm... We seem to have this (perhaps more recent) "thing" about libvirt having to supply some attribute based on some (mostly difficult to describe) algorithm that QEMU would use in order to create the "optimum" size or use for some alternate algorithm. Of course, a few libvir-list patches like that have been NACK'd because of the feeling that providing a useful algorithm for a user to "decide upon" a satisfactory attribute value cannot really be done. Off the top of my head I can come up with two: It's kind of a different story. Think of this as a48 +
[libvirt] ANNOUNCE: virt-bootstrap 1.1.0 released
I'm happy to announce the release of virt-bootstrap 1.1.0! virt-bootstrap is a tool providing an easy way to setup the root file system for libvirt-based containers. The release can be downloaded from: http://virt-manager.org/download/ This release includes: - safe_untar: check for permissions to set attribs (Radostin Stoyanov) - docker source, support blobs without .tar extension (Radostin Stoyanov) - docker-source, preserve extended file attributes (Radostin Stoyanov) - docker-source, get list of layers without `--raw` (Radostin Stoyanov) - docker-source, void skopeo copy in cache (Radostin Stoyanov) - Show error when guestfs-python or skopeo is not installed (Radostin Stoyanov) - pylint cleanups (Radostin Stoyanov) Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, -- Cédric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] testUpdateQEMUCaps: Don't leak host cpuData
On Thu, May 31, 2018 at 12:11:36 +0200, Michal Privoznik wrote:
> When preparing qemuCaps for test cases the following is
> happening:
>
> qemuTestParseCapabilitiesArch() is called, which calls
> virQEMUCapsLoadCache() which in turn calls
> virQEMUCapsInitHostCPUModel() which sets qemuCaps->kvmCPU and
> qemuCaps->tcgCPU.
>
> But then the code tries to update the capabilities:
>
> testCompareXMLToArgv() calls testUpdateQEMUCaps() which calls
> virQEMUCapsInitHostCPUModel() again overwriting previously
> allocated memory. The solution is to free host cpuData in
> testUpdateQEMUCaps().
>
> Signed-off-by: Michal Privoznik
> ---
>
> Technically this is v2 of [1] but since it implements completely
> different approach I'm sending it as v1.
>
> 1: https://www.redhat.com/archives/libvir-list/2018-May/msg02260.html
>
> src/qemu/qemu_capabilities.c | 21 +++--
> src/qemu/qemu_capspriv.h | 4
> tests/qemuxml2argvtest.c | 3 +++
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e2e76e4dd8..ea1ff520d8 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1516,12 +1516,19 @@ virQEMUCapsHostCPUDataCopy(virQEMUCapsHostCPUDataPtr
> dst,
>
>
> static void
> -virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
> +virQEMUCapsHostCPUDataClearHostCPU(virQEMUCapsHostCPUDataPtr cpuData)
The name is weired with two HostCPU substrings. How about
virQEMUCapsHostCPUDataClearModels
as it frees the generated CPU models, or
virQEMUCapsHostCPUDataClearGenerated
to emphasize the function clears the CPU models generated from other
data which we store in the cache?
> {
> -qemuMonitorCPUModelInfoFree(cpuData->info);
> virCPUDefFree(cpuData->reported);
> virCPUDefFree(cpuData->migratable);
> virCPUDefFree(cpuData->full);
> +}
> +
> +
> +static void
> +virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
> +{
> +qemuMonitorCPUModelInfoFree(cpuData->info);
> +virQEMUCapsHostCPUDataClearHostCPU(cpuData);
And this would need to be changed too, of course.
>
> memset(cpuData, 0, sizeof(*cpuData));
> }
With the changed name
Reviewed-by: Jiri Denemark
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5] qemu: Forbid old qcow/qcow2 encryption
On Thu, May 31, 2018 at 07:22:28 -0400, John Ferlan wrote: > > > On 05/31/2018 02:18 AM, Peter Krempa wrote: > > On Wed, May 30, 2018 at 16:14:31 -0400, John Ferlan wrote: > >> > >> > >> On 05/23/2018 10:13 AM, Peter Krempa wrote: > >>> The old qcow/qcow2 encryption format is so broken that qemu decided to > >>> drop it completely. This series forbids the use of such images even with > >>> qemus prior to this and removes all the cruft necessary to support it. > >>> > >>> v2: > >>> - fixed check to include the qcow format too > >>> - reworded the error message slightly > >>> - split second patch into two with proper justification for the > >>>user-alias test since checking LUKS there actually makes sense > >>> > >>> Peter Krempa (5): > >>> tests: qemuxml2argv: Drop disk encryption from 'interface-server' test > >>> tests: qemuxml2argv: Verify that disk secret alias is correct with > >>> user-aliases > >>> tests: qemublock: Switch to qcow2+luks in test files > >>> qemu: domain: Forbid storage with old QCOW2 encryption > >>> qemu: Remove code for setting up disk passphrases > >>> > >> > >> Why not remove it from storage as well? It's not like anything could or > >> would want to use whatever the storage driver created. There's always > >> the fall back to indicate to use qemu-img for the die hards. > > > > If we've ever supported the use case of converting a qcow2 encrypted > > volume even into a unencrypted volume, we should keep that for allowing > > migration from those volumes. > > > > Without (at least parts of) for qemu's 2.9 or later: > > https://www.redhat.com/archives/libvir-list/2018-May/msg01268.html > > it won't work anyway because of qemu secret work. Well, given that the required setup for qcow2 encryption is almost identical to luks I think we can support the old encryption on the input of the conversion process. > I think you probably need to make some documentation updates too. If not > removing things, then updating certain pages to indicate as of libvirt > 4.X.0 it won't be possible to use for domains (and possibly storage). Hmm, yeah, there should be a section describing the element. I'll add a note there and into news.xml. (probably as a separate patch. > > John > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 38/38] qemu: domain: Add support for TLS for NBD with default TLS env
On Thu, May 31, 2018 at 07:34:14 -0400, John Ferlan wrote:
> [...]
>
> >>> +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
> >>> + virQEMUDriverConfigPtr cfg,
> >>> + virQEMUCapsPtr qemuCaps)
> >>> +{
> >>> +/* XXX: for NBD we don't have the qemu.conf knobs for private TLS
> >>> env */
> >>
> >> I believe the thought was to use the migrate ones and not default. That
> >> way we could modify the qemu.conf to note that the migrate environment
> >> would be used for NBD as it made no sense to have/use separate envs.
> >
> > No. The migration environment shall be used only for NBD when migrating
> > disks. This is already the case by the way.
> >
> > For accessing regular disks we should use the default one or a specific
> > one (e.g. as we do have for vxhs) if that will ever be added.
> >
> > The separate environment might be wanted in the future if somebody wants
> > to have separate certificates for it, but it's not strictly required and
> > can easily be retrofitted into this optional way.
> >
>
> And how would anyone really know this? Why was this decision was made in
> favor of creating an NBD specific set of values. Ironically not a
> shortcut we've used/allowed for when adding TLS to chardev, migrate, or
> vxhs.
Well, this patch is mostly a simple implementation of TLS which allows
to use the default environment. Adding all the bloat necessary to setup
custom environment was not really a focus of this series.
I can move out this patch into hold if you think that the private
environment is necessary right from the beginning since adding TLS for
NBD wasn't really the main focus.
>
>
> >>> +if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
> >>> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
> >>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>> + _("this qemu does not support TLS transport
> >>> for nbd"));
> >>> +return -1;
> >>> +}
> >>> +
> >>> +if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
> >>> +return -1;
> >>> +
> >>> +src->tlsVerify = true;
> >>
> >> I think this is problematic for the default environment w/r/t since the
> >> right certs won't be present...
> >
> > Please elaborate on this. I didn't quite get what you meant.
> >
>
> tlsVerify is what's used for the verifypeer - in order for it to be
> useful, then the default environment would need:
>
> # client-cert.pem - the client certificate signed with the ca-cert.pem
> # client-key.pem - the client private key
These are required for verifying that the client is allowed to contact
the server ...
> if the default environment doesn't have those, then blindly setting this
> will cause a TLS failure if the default environment doesn't have those
> files.
No, that's not how it works. Both NBD and VXHS are 'clients' so they
always need to verify the server. [1]
> Since you're using cfg->defaultTLSx509certdir, then this should use
> cfg->defaultTLSx509verify.
In fact, tlsVerify for disks is pointless as long as we don't have
server-mode disks.
I'll actually try to remove that variable for now.
>
> John
>
[1] https://security.libvirt.org/2017/0002.html
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 38/38] qemu: domain: Add support for TLS for NBD with default TLS env
[...]
>>> +qemuProcessPrepareStorageSourceTlsNbd(virStorageSourcePtr src,
>>> + virQEMUDriverConfigPtr cfg,
>>> + virQEMUCapsPtr qemuCaps)
>>> +{
>>> +/* XXX: for NBD we don't have the qemu.conf knobs for private TLS env
>>> */
>>
>> I believe the thought was to use the migrate ones and not default. That
>> way we could modify the qemu.conf to note that the migrate environment
>> would be used for NBD as it made no sense to have/use separate envs.
>
> No. The migration environment shall be used only for NBD when migrating
> disks. This is already the case by the way.
>
> For accessing regular disks we should use the default one or a specific
> one (e.g. as we do have for vxhs) if that will ever be added.
>
> The separate environment might be wanted in the future if somebody wants
> to have separate certificates for it, but it's not strictly required and
> can easily be retrofitted into this optional way.
>
And how would anyone really know this? Why was this decision was made in
favor of creating an NBD specific set of values. Ironically not a
shortcut we've used/allowed for when adding TLS to chardev, migrate, or
vxhs.
>>> +if (src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>>> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NBD_TLS)) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("this qemu does not support TLS transport for
>>> nbd"));
>>> +return -1;
>>> +}
>>> +
>>> +if (VIR_STRDUP(src->tlsCertdir, cfg->defaultTLSx509certdir) < 0)
>>> +return -1;
>>> +
>>> +src->tlsVerify = true;
>>
>> I think this is problematic for the default environment w/r/t since the
>> right certs won't be present...
>
> Please elaborate on this. I didn't quite get what you meant.
>
tlsVerify is what's used for the verifypeer - in order for it to be
useful, then the default environment would need:
# client-cert.pem - the client certificate signed with the ca-cert.pem
# client-key.pem - the client private key
if the default environment doesn't have those, then blindly setting this
will cause a TLS failure if the default environment doesn't have those
files.
Since you're using cfg->defaultTLSx509certdir, then this should use
cfg->defaultTLSx509verify.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5] qemu: Forbid old qcow/qcow2 encryption
On 05/31/2018 02:18 AM, Peter Krempa wrote: > On Wed, May 30, 2018 at 16:14:31 -0400, John Ferlan wrote: >> >> >> On 05/23/2018 10:13 AM, Peter Krempa wrote: >>> The old qcow/qcow2 encryption format is so broken that qemu decided to >>> drop it completely. This series forbids the use of such images even with >>> qemus prior to this and removes all the cruft necessary to support it. >>> >>> v2: >>> - fixed check to include the qcow format too >>> - reworded the error message slightly >>> - split second patch into two with proper justification for the >>>user-alias test since checking LUKS there actually makes sense >>> >>> Peter Krempa (5): >>> tests: qemuxml2argv: Drop disk encryption from 'interface-server' test >>> tests: qemuxml2argv: Verify that disk secret alias is correct with >>> user-aliases >>> tests: qemublock: Switch to qcow2+luks in test files >>> qemu: domain: Forbid storage with old QCOW2 encryption >>> qemu: Remove code for setting up disk passphrases >>> >> >> Why not remove it from storage as well? It's not like anything could or >> would want to use whatever the storage driver created. There's always >> the fall back to indicate to use qemu-img for the die hards. > > If we've ever supported the use case of converting a qcow2 encrypted > volume even into a unencrypted volume, we should keep that for allowing > migration from those volumes. > Without (at least parts of) for qemu's 2.9 or later: https://www.redhat.com/archives/libvir-list/2018-May/msg01268.html it won't work anyway because of qemu secret work. I think you probably need to make some documentation updates too. If not removing things, then updating certain pages to indicate as of libvirt 4.X.0 it won't be possible to use for domains (and possibly storage). John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
On Thu, May 31, 2018 at 11:44:06AM +0200, Michal Privoznik wrote:
> On 05/31/2018 10:56 AM, Peter Krempa wrote:
> > On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
> >> On 05/31/2018 10:05 AM, Erik Skultety wrote:
> >
> > [...]
> >
> >>> @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> >>> virDomainHostdevDefPtr hostdev,
> >>> goto cleanup;
> >>> }
> >>> break;
> >>> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> >>> +if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
> >>> +VIR_WARN("OOM while enconding audit message");
> >>> +goto cleanup;
> >>> +}
> >>> +break;
> >>
> >> This makes sense.
> >>
> >>> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:> default:
> >>
> >> But this does not. Well, in combination with [1] it doesn't. Firstly,
> >> why do we even have "default" labels? Secondly, what's the point of
> >> typecasting when we have "default" label? Same goes for the outer
> >> switch(). I think we should remove "default" labels.
> >
> > We are doing the opposite now. Some reading you probably missed:
> >
> > https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
> >
>
> Ah, good point. ACK and safe for freeze then.
Pushed, thanks.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] conf: fixup USB input bus check
On Thu, May 31, 2018 at 11:11:58AM +0200, Xiao Feng Ren wrote: This patch fixes the USB input bus check, the bug was introduced by commit 317badb Signed-off-by: Xiao Feng Ren --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko And pushed, congratulations on your first libvirt patch! Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
On 31/05/2018 10:19, Martin Kletzander wrote: >> > > Oh, it definitely helps. And I always enjoy when someone explains > low level details of something like you do, it makes me feel very > smart after I read it =) > > ...something about the shoulders of giants and stuff... > > I'll fix this up according to what you described, that is smm=on/off > will be possible to be specified whenever -machine supports smm=. That sounds like the right thing, thanks! Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] testUpdateQEMUCaps: Don't leak host cpuData
When preparing qemuCaps for test cases the following is
happening:
qemuTestParseCapabilitiesArch() is called, which calls
virQEMUCapsLoadCache() which in turn calls
virQEMUCapsInitHostCPUModel() which sets qemuCaps->kvmCPU and
qemuCaps->tcgCPU.
But then the code tries to update the capabilities:
testCompareXMLToArgv() calls testUpdateQEMUCaps() which calls
virQEMUCapsInitHostCPUModel() again overwriting previously
allocated memory. The solution is to free host cpuData in
testUpdateQEMUCaps().
Signed-off-by: Michal Privoznik
---
Technically this is v2 of [1] but since it implements completely
different approach I'm sending it as v1.
1: https://www.redhat.com/archives/libvir-list/2018-May/msg02260.html
src/qemu/qemu_capabilities.c | 21 +++--
src/qemu/qemu_capspriv.h | 4
tests/qemuxml2argvtest.c | 3 +++
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e2e76e4dd8..ea1ff520d8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1516,12 +1516,19 @@ virQEMUCapsHostCPUDataCopy(virQEMUCapsHostCPUDataPtr
dst,
static void
-virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
+virQEMUCapsHostCPUDataClearHostCPU(virQEMUCapsHostCPUDataPtr cpuData)
{
-qemuMonitorCPUModelInfoFree(cpuData->info);
virCPUDefFree(cpuData->reported);
virCPUDefFree(cpuData->migratable);
virCPUDefFree(cpuData->full);
+}
+
+
+static void
+virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData)
+{
+qemuMonitorCPUModelInfoFree(cpuData->info);
+virQEMUCapsHostCPUDataClearHostCPU(cpuData);
memset(cpuData, 0, sizeof(*cpuData));
}
@@ -2834,6 +2841,16 @@ virQEMUCapsNewHostCPUModel(void)
}
+void
+virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps,
+virDomainVirtType type)
+{
+virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps,
type);
+
+virQEMUCapsHostCPUDataClearHostCPU(cpuData);
+}
+
+
void
virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
virArch hostArch,
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index 0199501c93..fea039ef3a 100644
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -56,6 +56,10 @@ void
virQEMUCapsSetArch(virQEMUCapsPtr qemuCaps,
virArch arch);
+void
+virQEMUCapsFreeHostCPUModel(virQEMUCapsPtr qemuCaps,
+virDomainVirtType type);
+
void
virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
virArch hostArch,
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a6a40e273e..61c7ae59aa 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -388,6 +388,9 @@ testUpdateQEMUCaps(const struct testInfo *info,
if (testAddCPUModels(info->qemuCaps, info->skipLegacyCPUs) < 0)
goto cleanup;
+virQEMUCapsFreeHostCPUModel(info->qemuCaps, VIR_DOMAIN_VIRT_KVM);
+virQEMUCapsFreeHostCPUModel(info->qemuCaps, VIR_DOMAIN_VIRT_QEMU);
+
virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch,
VIR_DOMAIN_VIRT_KVM);
virQEMUCapsInitHostCPUModel(info->qemuCaps, caps->host.arch,
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
On 05/31/2018 10:56 AM, Peter Krempa wrote:
> On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
>> On 05/31/2018 10:05 AM, Erik Skultety wrote:
>
> [...]
>
>>> @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
>>> virDomainHostdevDefPtr hostdev,
>>> goto cleanup;
>>> }
>>> break;
>>> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
>>> +if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
>>> +VIR_WARN("OOM while enconding audit message");
>>> +goto cleanup;
>>> +}
>>> +break;
>>
>> This makes sense.
>>
>>> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:> default:
>>
>> But this does not. Well, in combination with [1] it doesn't. Firstly,
>> why do we even have "default" labels? Secondly, what's the point of
>> typecasting when we have "default" label? Same goes for the outer
>> switch(). I think we should remove "default" labels.
>
> We are doing the opposite now. Some reading you probably missed:
>
> https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
>
Ah, good point. ACK and safe for freeze then.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
On 05/31/2018 10:57 AM, Jiri Denemark wrote:
> On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
>> On 05/30/2018 06:57 PM, Peter Krempa wrote:
>>> On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
While this leak happens in tests only, it is still worth fixing.
==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are
definitely lost in loss record 325 of 331
==12962==at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
==12962==by 0x5D285D5: virAlloc (viralloc.c:144)
==12962==by 0x5E823EB: virCPUGetHost (cpu.c:411)
==12962==by 0x56DE953: virQEMUCapsInitHostCPUModel
(qemu_capabilities.c:2874)
==12962==by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
==12962==by 0x13802F: qemuTestParseCapabilitiesArch
(testutilsqemu.c:500)
==12962==by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
==12962==by 0x13AD0B: virTestMain (testutils.c:1120)
==12962==by 0x1372FD: main (qemuxml2argvtest.c:2883)
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e2e76e4dd8..9ec9089d5b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
{
virQEMUCapsHostCPUDataPtr cpuData =
virQEMUCapsGetHostCPUData(qemuCaps, type);
+virCPUDefFree(cpuData->reported);
+virCPUDefFree(cpuData->migratable);
+virCPUDefFree(cpuData->full);
>>>
>>> This looks fishy. The test code always unrefs the qemuCaps object, so
>>> there should be no leak. Could you please elaborate on how this happens?
>>
>> Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
>> (which exists only for sake of tests) which does two things:
>>
>> 1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
>> where cpuData is firstly allocated
>
> virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which
> sets cpuData->info
>
>> 2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
>> whatever was loaded in 1).
>
> while virQEMUCapsInitHostCPUModel fills cpuData->reported,
> cpuData->migratable, and cpuData->full using the data from
> cpuData->info.
>
> It must be something else which causes the leak.
Ah right. It's different call trace:
Hardware watchpoint 2: -location qemuCaps->kvmCPU->full
Old value = (virCPUDef *) 0x0
New value = (virCPUDef *) 0x558f1740
virQEMUCapsSetHostModel (qemuCaps=0x557e7ea0, type=VIR_DOMAIN_VIRT_KVM,
reported=0x558f0770, migratable=0x558f1de0, full=0x558f1740) at
qemu/qemu_capabilities.c:1863
1863}
virQEMUCapsSetHostModel 1 $ bt
#0 virQEMUCapsSetHostModel (qemuCaps=0x557e7ea0, type=VIR_DOMAIN_VIRT_KVM,
reported=0x558f0770, migratable=0x558f1de0, full=0x558f1740) at
qemu/qemu_capabilities.c:1863
#1 0x77211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x557e7ea0,
hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at
qemu/qemu_capabilities.c:2903
#2 0x77213630 in virQEMUCapsLoadCache (hostArch=VIR_ARCH_X86_64,
qemuCaps=0x557e7ea0, filename=0x557eaef0
"tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at
qemu/qemu_capabilities.c:3435
#3 0x55584030 in qemuTestParseCapabilitiesArch (arch=VIR_ARCH_X86_64,
capsFile=0x557eaef0 "tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at
testutilsqemu.c:500
#4 0xfe1b in mymain () at qemuxml2argvtest.c:802
#5 0x55586d0c in virTestMain (argc=1, argv=0x7fffd698,
func=0xf4f9 ) at testutils.c:1120
#6 0x555832fe in main (argc=1, argv=0x7fffd698) at
qemuxml2argvtest.c:2883
virQEMUCapsSetHostModel 1 $ c
Continuing.
Hardware watchpoint 2: -location qemuCaps->kvmCPU->full
Old value = (virCPUDef *) 0x558f1740
New value = (virCPUDef *) 0x558f1820
virQEMUCapsSetHostModel (qemuCaps=0x557e7ea0, type=VIR_DOMAIN_VIRT_KVM,
reported=0x557e2ad0, migratable=0x558f3190, full=0x558f1820) at
qemu/qemu_capabilities.c:1863
1863}
virQEMUCapsSetHostModel 1 $ bt
#0 virQEMUCapsSetHostModel (qemuCaps=0x557e7ea0, type=VIR_DOMAIN_VIRT_KVM,
reported=0x557e2ad0, migratable=0x558f3190, full=0x558f1820) at
qemu/qemu_capabilities.c:1863
#1 0x77211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x557e7ea0,
hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at
qemu/qemu_capabilities.c:2903
#2 0xeb34 in testUpdateQEMUCaps (info=0x5579f800 ,
vm=0x557e2eb0, caps=0x557e18b0) at qemuxml2argvtest.c:391
#3 0xf116 in testCompareXMLToArgv (data=0x5579f800 ) at
qemuxml2argvtest.c:518
#4 0x55584c50 in virTestRun (title=0x55588e28 "QEMU XML-2-ARGV
genid.x86_64
[libvirt] [PATCH 1/1] conf: fixup USB input bus check
This patch fixes the USB input bus check, the bug was introduced by commit 317badb Signed-off-by: Xiao Feng Ren --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..e4d39c06e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27878,7 +27878,7 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev) if ((t == VIR_DOMAIN_DEVICE_DISK && dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) || (t == VIR_DOMAIN_DEVICE_INPUT && - dev->data.input->type == VIR_DOMAIN_INPUT_BUS_USB) || + dev->data.input->bus == VIR_DOMAIN_INPUT_BUS_USB) || (t == VIR_DOMAIN_DEVICE_HOSTDEV && dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == -- 2.16.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/1] bug: fixup USB input bus check
On 5/31/2018 4:21 PM, Ján Tomko wrote: On Thu, May 31, 2018 at 09:55:38AM +0200, Xiao Feng Ren wrote: There's one explicit bug(introduced in commit:317badb) when checking the bus of one input device is USB. Let's fix it. The fix looks good to me, but the explanation (which is not really and the mention of the commit that broke it should be a part of the commit message, not the cover letter. (Also, a cover letter is not required for single patches) Jano Ok, will modify. Xiao Feng Ren (1): Fixup USB input bus check src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.16.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
> On 05/30/2018 06:57 PM, Peter Krempa wrote:
> > On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
> >> While this leak happens in tests only, it is still worth fixing.
> >>
> >> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are
> >> definitely lost in loss record 325 of 331
> >> ==12962==at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
> >> ==12962==by 0x5D285D5: virAlloc (viralloc.c:144)
> >> ==12962==by 0x5E823EB: virCPUGetHost (cpu.c:411)
> >> ==12962==by 0x56DE953: virQEMUCapsInitHostCPUModel
> >> (qemu_capabilities.c:2874)
> >> ==12962==by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
> >> ==12962==by 0x13802F: qemuTestParseCapabilitiesArch
> >> (testutilsqemu.c:500)
> >> ==12962==by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
> >> ==12962==by 0x13AD0B: virTestMain (testutils.c:1120)
> >> ==12962==by 0x1372FD: main (qemuxml2argvtest.c:2883)
> >>
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/qemu/qemu_capabilities.c | 4
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> >> index e2e76e4dd8..9ec9089d5b 100644
> >> --- a/src/qemu/qemu_capabilities.c
> >> +++ b/src/qemu/qemu_capabilities.c
> >> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
> >> {
> >> virQEMUCapsHostCPUDataPtr cpuData =
> >> virQEMUCapsGetHostCPUData(qemuCaps, type);
> >>
> >> +virCPUDefFree(cpuData->reported);
> >> +virCPUDefFree(cpuData->migratable);
> >> +virCPUDefFree(cpuData->full);
> >
> > This looks fishy. The test code always unrefs the qemuCaps object, so
> > there should be no leak. Could you please elaborate on how this happens?
>
> Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
> (which exists only for sake of tests) which does two things:
>
> 1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
> where cpuData is firstly allocated
virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which
sets cpuData->info
> 2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
> whatever was loaded in 1).
while virQEMUCapsInitHostCPUModel fills cpuData->reported,
cpuData->migratable, and cpuData->full using the data from
cpuData->info.
It must be something else which causes the leak.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
On Thu, May 31, 2018 at 10:16:41 +0200, Michal Privoznik wrote:
> On 05/31/2018 10:05 AM, Erik Skultety wrote:
[...]
> > @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> > virDomainHostdevDefPtr hostdev,
> > goto cleanup;
> > }
> > break;
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > +if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
> > +VIR_WARN("OOM while enconding audit message");
> > +goto cleanup;
> > +}
> > +break;
>
> This makes sense.
>
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:> default:
>
> But this does not. Well, in combination with [1] it doesn't. Firstly,
> why do we even have "default" labels? Secondly, what's the point of
> typecasting when we have "default" label? Same goes for the outer
> switch(). I think we should remove "default" labels.
We are doing the opposite now. Some reading you probably missed:
https://www.redhat.com/archives/libvir-list/2018-February/msg00728.html
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
> On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> > >
> > > This is way too sparse.
> > >
> > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > > >
> > > > Signed-off-by: Martin Kletzander
> > > > ---
> > > > src/qemu/qemu_command.c | 18
> > > > src/qemu/qemu_domain.c| 84 +++
> > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++
> > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +
> > > > tests/qemuxml2argvdata/tseg-i440fx.xml| 23 +
> > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +
> > > > .../tseg-old-machine-type.args| 27 ++
> > > > .../tseg-old-machine-type.xml | 21 +
> > > > tests/qemuxml2argvdata/tseg.args | 28 +++
> > > > tests/qemuxml2argvdata/tseg.xml | 21 +
> > > > tests/qemuxml2argvtest.c | 48 +++
> > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++
> > > > .../tseg-old-machine-type.xml | 44 ++
> > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++
> > > > tests/qemuxml2xmltest.c | 25 ++
> > > > 15 files changed, 505 insertions(+)
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > > create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
> >
> > [...]
> >
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 881d0ea46a75..3ea9e3d47344 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > > > }
> > > >
> > > >
> > > > +static int
> > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > > + virQEMUCapsPtr qemuCaps)
> > > > +{
> > > > +const char *machine = NULL;
> > > > +char *end_ptr = NULL;
> > > > +unsigned int major = 0;
> > > > +unsigned int minor = 0;
> > > > +
> > > > +def->tseg_size = 0;
> > >
> > > Pointless since the only way in here is "if (tseg_size == 0)"
> > >
> > > > +
> > > > +if (!qemuDomainIsQ35(def))
> > > > +return 0;
> > > > +
> > > > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > >
> > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> > > though since it does follow the QEMU name.
> > >
> > > > +return 0;
> > > > +
> > > > +machine = STRSKIP(def->os.machine, "pc-q35-");
> > > > +
> > > > +if (!machine)
> > > > +goto error;
> > > > +
> > > > +if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > > +goto error;
> > > > +
> > > > +if (*end_ptr != '.')
> > > > +goto error;
> > > > +
> > > > +machine = end_ptr + 1;
> > > > +
> > > > +if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > > +goto error;
> > > > +if (*end_ptr != '\0')
> > > > +goto error;
> > > > +
> > > > +/* QEMU started defaulting to 16MiB after 2.9 */
> > > > +if (major > 2 || (major == 2 && minor > 9))
> > > > +def->tseg_size = 16 * 1024 * 1024;
> > >
> > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> > >
> > > This seems to me we are setting policy which based on history of many
> > > patches before is a no go. I'd say NAK to this, unless there is some
> > > convincing argument made in the commit message and followup responses to
> > > the series (or you can take Jan's R-By and ignore me - your call.
> >
> > I agree with John, this whole function should be removed, we don't have
> > to set the default in config XML. However we should record that default
> > value into live/status XML to make sure that it will not be changed
> > during migration and to let the user know what is the default value.
> >
>
> That doesn't make sense. This is part of guest ABI and if you want to be on
> the
> safe side when migrating, then you should be way more cautious with the
> stop/start scenario. Migration will probably fail (o
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
On Thu, May 31, 2018 at 10:16:41AM +0200, Michal Privoznik wrote:
> On 05/31/2018 10:05 AM, Erik Skultety wrote:
> > There was a missing enum for mdev causing a strange 'unknown device type'
> > warning when hot-plugging mdev.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1583927
> >
> > Signed-off-by: Erik Skultety
> > ---
> > src/conf/domain_audit.c | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
> > index 82868bca76..14138d93af 100644
> > --- a/src/conf/domain_audit.c
> > +++ b/src/conf/domain_audit.c
> > @@ -361,6 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> > virDomainHostdevDefPtr hostdev,
> > virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> > virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> > virDomainHostdevSubsysSCSIVHostPtr hostsrc =
> > &hostdev->source.subsys.u.scsi_host;
> > +virDomainHostdevSubsysMediatedDevPtr mdevsrc =
> > &hostdev->source.subsys.u.mdev;
> >
> > virUUIDFormat(vm->def->uuid, uuidstr);
> > if (!(vmname = virAuditEncode("vm", vm->def->name))) {
> > @@ -373,9 +374,9 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> > virDomainHostdevDefPtr hostdev,
> > virt = "?";
> > }
> >
> > -switch (hostdev->mode) {
> > +switch ((virDomainHostdevMode) hostdev->mode) {
> > case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
> > -switch (hostdev->source.subsys.type) {
> > +switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
>
> 1: ^^^
>
> > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> > if (virAsprintfQuiet(&address, "%.4x:%.2x:%.2x.%.1x",
> > pcisrc->addr.domain,
> > @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> > virDomainHostdevDefPtr hostdev,
> > goto cleanup;
> > }
> > break;
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > +if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
> > +VIR_WARN("OOM while enconding audit message");
> > +goto cleanup;
> > +}
> > +break;
>
> This makes sense.
>
> > +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:> default:
>
> But this does not. Well, in combination with [1] it doesn't. Firstly,
> why do we even have "default" labels? Secondly, what's the point of
We have them because type casting won't save you from rogue values that can
appear during runtime. Yes, I know, when can that happen if we're in charge,
i.e. it's not a value provided by user? I said something similar at some point
to an email thread on the mailing list that we're in charge here, if we change
an enum value to some garbage we should know about it, ergo the default cases
shouldn't be even needed, but the consensus seemed to be that this kind of
defensive programming doesn't make any performance difference and we are fairly
consistent, we do this on a lot of places, it's a pity I can't find the email
thread to link it here.
> typecasting when we have "default" label? Same goes for the outer
typecasting is for compile-time errors that would tell the programmer about all
the places he missed to add the enum to a switch, which is pretty much the
nature of this bug.
> switch(). I think we should remove "default" labels.
Although you're right because parsing of the XML precedes auditing, having a
default case in a switch doesn't add any overhead, doesn't contribute to the
code being less readable, on the other hand adds some "safety" so I'd like to
keep it in the patch.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/1] bug: fixup USB input bus check
On Thu, May 31, 2018 at 09:55:38AM +0200, Xiao Feng Ren wrote: There's one explicit bug(introduced in commit:317badb) when checking the bus of one input device is USB. Let's fix it. The fix looks good to me, but the explanation (which is not really and the mention of the commit that broke it should be a part of the commit message, not the cover letter. (Also, a cover letter is not required for single patches) Jano Xiao Feng Ren (1): Fixup USB input bus check src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.16.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
On Thu, May 31, 2018 at 09:33:54AM +0200, Laszlo Ersek wrote:
adding qemu-devel, Paolo and Gerd; comments below:
On 05/30/18 23:08, Martin Kletzander wrote:
On Wed, May 30, 2018 at 11:02:59AM -0400, John Ferlan wrote:
On 05/21/2018 11:00 AM, Martin Kletzander wrote:
We are still hoping all of such checks will be moved there and this
is one small
step in that direction.
One of the things that this is improving is the error message you get
when
starting a domain with SMM and i440fx, for example. Instead of
saying that the
QEMU binary doesn't support that option, we correctly say that it is
only
supported with q35 machine type.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_capabilities.c | 21 +++--
src/qemu/qemu_capabilities.h | 4 ++--
src/qemu/qemu_command.c | 12 ++--
src/qemu/qemu_domain.c | 12 +---
4 files changed, 28 insertions(+), 21 deletions(-)
I know it's outside the bounds of what you're doing; however,
qemuDomainDefValidateFeatures could check the capabilities for other
bits too...
Probably, but I mostly wanted to do that because SMM is not only about the
capability, but also about the machine. Good idea for the future, though.
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d3beee5d8760..881d0ea46a75 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const
virDomainDef *def)
static int
-qemuDomainDefValidateFeatures(const virDomainDef *def)
+qemuDomainDefValidateFeatures(const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
size_t i;
@@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const
virDomainDef *def)
}
break;
+ case VIR_DOMAIN_FEATURE_SMM:
+ if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
Probably should change to != _ABSENT, since qemu_command will supply
smm={on|off}
That makes sense, kind of. For 'off' we only need to check if we can
specify
the smm= option. The thing is that you can even specify smm=on with
non-q35
machine type, but it is unclear what that's going to mean since it doesn't
really make sense.
@Laszlo: What would you say? Should we allow users to specify smm=on
for users?
Or even better, does it makes sense to allow specifying smm=anything for
non-q35
machine types? If not, we'll leave it like this, that is smm=anything is
forbidden for non-q35 machine types.
Technically the "smm" machine type property is defined for both i440fx
and q35:
$ qemu-system-x86_64 -machine pc,\? 2>&1 | grep smm
pc-i440fx-2.11.smm=OnOffAuto (Enable SMM (pc & q35))
$ qemu-system-x86_64 -machine q35,\? 2>&1 | grep smm
pc-q35-2.11.smm=OnOffAuto (Enable SMM (pc & q35))
The "smm" property controls whether system management mode is emulated,
to my knowledge. That's an x86 processor operating mode, which isn't
specific to the Q35 board. What's specific to Q35 is the TSEG.
The primary reason why OVMF (built with the edk2 SMM driver stack)
requires Q35 is not that i440fx does not emulate SMM, it's that i440fx
doesn't have a large enough SMRAM area. (The legacy SMRAM areas are too
small for OVMF.)
For example, SeaBIOS too includes some SMM code (optionally, if it's
built like that), and that runs on i440fx just fine, because the legacy
SMRAM areas are large enough for SeaBIOS's purposes, AIUI.
(Now, there's at least one other reason why OVMF/SMM needs Q35: because
the SMI broadcast feature too is only implemented on Q35. But that's
rather a consequence of the above "primary reason", and not a
stand-alone reason itself -- we restricted the SMI broadcast feature to
Q35 *because* OVMF could only run on Q35. Anyhow, you need not care
about SMI broadcast because it's automatically negotiated between guest
fw and QEMU.)
Summary:
(1) For making Secure Boot actually secure in OVMF, OVMF needs to be
built with -D SMM_REQUIRE, so that it include the SMM driver stack.
(2) Booting such a firmware binary requires Q35, because only Q35 offers
TSEG, and the legacy -- non-TSEG -- SMRAM ranges are too small even for
a "basic boot" of OVMF.
(3) QEMU has to be configured with "-machine smm=on"; this is already
covered by libvirt via .
(4) QEMU has to be configured to restrict pflash writes to code that
executes in SMM, with "-global
driver=cfi.pflash01,property=secure,value=on". Libvirt covers this
already with the @secure=yes attribute in .
(5) There are two *quality of service* features related to SMM; with
both of them being Q35-only. The first feature is SMI broadcast; libvirt
need not care because it's auto-negotiated.
(6) The second QoS feature is *extended* TSEG (a paravirt feature on top
of the standard TSEG), which lets the edk2 SMM driver stack scale to
large RAM sizes and high VCPU counts. Libvirt should let the user
configure the extended TSEG size, because the necessary minimum is super
difficult to calculate (and one "maximum
Re: [libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
On 05/31/2018 10:05 AM, Erik Skultety wrote:
> There was a missing enum for mdev causing a strange 'unknown device type'
> warning when hot-plugging mdev.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1583927
>
> Signed-off-by: Erik Skultety
> ---
> src/conf/domain_audit.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
> index 82868bca76..14138d93af 100644
> --- a/src/conf/domain_audit.c
> +++ b/src/conf/domain_audit.c
> @@ -361,6 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev,
> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> virDomainHostdevSubsysSCSIVHostPtr hostsrc =
> &hostdev->source.subsys.u.scsi_host;
> +virDomainHostdevSubsysMediatedDevPtr mdevsrc =
> &hostdev->source.subsys.u.mdev;
>
> virUUIDFormat(vm->def->uuid, uuidstr);
> if (!(vmname = virAuditEncode("vm", vm->def->name))) {
> @@ -373,9 +374,9 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev,
> virt = "?";
> }
>
> -switch (hostdev->mode) {
> +switch ((virDomainHostdevMode) hostdev->mode) {
> case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
> -switch (hostdev->source.subsys.type) {
> +switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
1: ^^^
> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> if (virAsprintfQuiet(&address, "%.4x:%.2x:%.2x.%.1x",
> pcisrc->addr.domain,
> @@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev,
> goto cleanup;
> }
> break;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
> +VIR_WARN("OOM while enconding audit message");
> +goto cleanup;
> +}
> +break;
This makes sense.
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:> default:
But this does not. Well, in combination with [1] it doesn't. Firstly,
why do we even have "default" labels? Secondly, what's the point of
typecasting when we have "default" label? Same goes for the outer
switch(). I think we should remove "default" labels.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
This is way too sparse.
On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>
> Signed-off-by: Martin Kletzander
> ---
> src/qemu/qemu_command.c | 18
> src/qemu/qemu_domain.c| 84 +++
> .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++
> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +
> tests/qemuxml2argvdata/tseg-i440fx.xml| 23 +
> tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +
> .../tseg-old-machine-type.args| 27 ++
> .../tseg-old-machine-type.xml | 21 +
> tests/qemuxml2argvdata/tseg.args | 28 +++
> tests/qemuxml2argvdata/tseg.xml | 21 +
> tests/qemuxml2argvtest.c | 48 +++
> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++
> .../tseg-old-machine-type.xml | 44 ++
> tests/qemuxml2xmloutdata/tseg.xml | 46 ++
> tests/qemuxml2xmltest.c | 25 ++
> 15 files changed, 505 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2argvdata/tseg.args
> create mode 100644 tests/qemuxml2argvdata/tseg.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 881d0ea46a75..3ea9e3d47344 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> }
>
>
> +static int
> +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +{
> +const char *machine = NULL;
> +char *end_ptr = NULL;
> +unsigned int major = 0;
> +unsigned int minor = 0;
> +
> +def->tseg_size = 0;
Pointless since the only way in here is "if (tseg_size == 0)"
> +
> +if (!qemuDomainIsQ35(def))
> +return 0;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
Reading this now makes me realized _MBYTES is probably unnecessary, IDC
though since it does follow the QEMU name.
> +return 0;
> +
> +machine = STRSKIP(def->os.machine, "pc-q35-");
> +
> +if (!machine)
> +goto error;
> +
> +if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> +goto error;
> +
> +if (*end_ptr != '.')
> +goto error;
> +
> +machine = end_ptr + 1;
> +
> +if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> +goto error;
> +if (*end_ptr != '\0')
> +goto error;
> +
> +/* QEMU started defaulting to 16MiB after 2.9 */
> +if (major > 2 || (major == 2 && minor > 9))
> +def->tseg_size = 16 * 1024 * 1024;
So, if QEMU defaults to 16MiB, then why do we need so set this at all?
This seems to me we are setting policy which based on history of many
patches before is a no go. I'd say NAK to this, unless there is some
convincing argument made in the commit message and followup responses to
the series (or you can take Jan's R-By and ignore me - your call.
I agree with John, this whole function should be removed, we don't have
to set the default in config XML. However we should record that default
value into live/status XML to make sure that it will not be changed
during migration and to let the user know what is the default value.
That doesn't make sense. This is part of guest ABI and if you want to be on the
safe side when migrating, then you should be way more cautious with the
stop/start scenario. Migration will probably fail (or silently corrupt data,
but I believe that wouldn't happen), but stop/start without keeping the default
will change that in case QEMU changes default and then the guest ABI will
change. See my other reply to this particular concern.
In order to do that, look at qemuProcessRefreshState() function where we
already gather some information from QEMU after starting domain, the
tseg default value can by updated by calling:
qemuMonitorJSONGetObjectProperty() with
path: /machine/q35/mch
property: extended-tseg-mbytes
Pavel
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinf
Re: [libvirt] What is the strategy to update the CPU Models in src/cpu/cpu_map.xml based on?
On Wed, May 30, 2018 at 12:55:19 -0300, Eduardo Habkost wrote: > The main question that I'm still unable to answer is: when > cpu_map.xml is still used? Does it affect libvirt behavior in > any way when using a recent QEMU version? Yes, it is still used. Mainly for reporting host CPU capabilities (even those retrieved from QEMU), because we only get a list of features (both from QEMU and CPUID) and need to somehow choose a corresponding CPU model. And its also partially used to check whether a particular CPU can be provided on the host when we start a domain. But this checking is done mostly for backward compatibility and it is not very strict, the final decision of what can or cannot be used is on QEMU. In other words, we accept any feature which is either supported by the host CPU (using CPUID) or reported by QEMU as enabled for the "host" CPU model. Any requested features which cannot be provided by QEMU would just be disabled in the virtual CPU. This is of course all about the default check='partial'. Libvirt doesn't do any checks with check='full' and just asks QEMU whether it disabled any requested features. > Is the file considered part of the libvirt API? The file format is not considered an API and it can change, but the CPU models, features and any other names defined there which can be used to describe a CPU are an API similarly to domain XML, for example. That is, each name has the same meaning across all versions of libvirt which support it. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemuxml2argvtest: Don't initialize qemuCaps twice
On Thu, May 31, 2018 at 10:06:38 +0200, Michal Privoznik wrote: > On 05/30/2018 06:46 PM, Peter Krempa wrote: > > On Wed, May 30, 2018 at 18:04:28 +0200, Michal Privoznik wrote: > >> There's no point in calling testInitQEMUCaps() (which sets > >> info.qemuCaps) only to overwrite (and leak) it on the very next > >> line. > >> > >> ==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely > >> lost in loss record 265 of 331 > >> ==12962==at 0x4C2CF26: calloc (vg_replace_malloc.c:711) > >> ==12962==by 0x5D28D9F: virAllocVar (viralloc.c:560) > >> ==12962==by 0x5D96AB4: virObjectNew (virobject.c:239) > >> ==12962==by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) > >> ==12962==by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) > >> ==12962==by 0x1371C8: mymain (qemuxml2argvtest.c:2871) > >> ==12962==by 0x13AD0B: virTestMain (testutils.c:1120) > >> ==12962==by 0x1372FD: main (qemuxml2argvtest.c:2883) > >> > >> Signed-off-by: Michal Privoznik > >> --- > >> tests/qemuxml2argvtest.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > >> index ddd2b88c0a..6f421ce8f5 100644 > >> --- a/tests/qemuxml2argvtest.c > >> +++ b/tests/qemuxml2argvtest.c > >> @@ -699,8 +699,6 @@ mymain(void) > >> (flags), parseFlags, false, NULL \ > >> }; \ > >> info.skipLegacyCPUs = skipLegacyCPUs; \ > >> -if (testInitQEMUCaps(&info, gic) < 0) \ > >> -return EXIT_FAILURE; \ > > > > > > This makes the 'gic' macro argument unused. You probably need to replace > > it with testQemuCapsSetGIC after the caps are parsed if the parser does > > not do that. > > Ah good point. However, since all callers of this macro pass GIC_NONE > (if anything) the argument is not needed. The qemuCaps code is perfectly > capable of handling no gic set (seevirQEMUCapsSupportsGICVersion()). > Because of this I'm going to remove the argument in v2. Well, that was so that we could later add macros for ARM, but as we parse in capabilities, it should perhaps be encoded in them rather than provided externally. pre-emptive ACK and obvious-safe-for-freeze for the version with the GIC argument removed. > > Michal signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemuxml2argvtest: Don't initialize qemuCaps twice
On 05/30/2018 06:46 PM, Peter Krempa wrote: > On Wed, May 30, 2018 at 18:04:28 +0200, Michal Privoznik wrote: >> There's no point in calling testInitQEMUCaps() (which sets >> info.qemuCaps) only to overwrite (and leak) it on the very next >> line. >> >> ==12962== 296 (208 direct, 88 indirect) bytes in 1 blocks are definitely >> lost in loss record 265 of 331 >> ==12962==at 0x4C2CF26: calloc (vg_replace_malloc.c:711) >> ==12962==by 0x5D28D9F: virAllocVar (viralloc.c:560) >> ==12962==by 0x5D96AB4: virObjectNew (virobject.c:239) >> ==12962==by 0x56DB7C7: virQEMUCapsNew (qemu_capabilities.c:1480) >> ==12962==by 0x112A5B: testInitQEMUCaps (qemuxml2argvtest.c:361) >> ==12962==by 0x1371C8: mymain (qemuxml2argvtest.c:2871) >> ==12962==by 0x13AD0B: virTestMain (testutils.c:1120) >> ==12962==by 0x1372FD: main (qemuxml2argvtest.c:2883) >> >> Signed-off-by: Michal Privoznik >> --- >> tests/qemuxml2argvtest.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index ddd2b88c0a..6f421ce8f5 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c >> @@ -699,8 +699,6 @@ mymain(void) >> (flags), parseFlags, false, NULL \ >> }; \ >> info.skipLegacyCPUs = skipLegacyCPUs; \ >> -if (testInitQEMUCaps(&info, gic) < 0) \ >> -return EXIT_FAILURE; \ > > > This makes the 'gic' macro argument unused. You probably need to replace > it with testQemuCapsSetGIC after the caps are parsed if the parser does > not do that. Ah good point. However, since all callers of this macro pass GIC_NONE (if anything) the argument is not needed. The qemuCaps code is perfectly capable of handling no gic set (seevirQEMUCapsSupportsGICVersion()). Because of this I'm going to remove the argument in v2. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
On 05/30/2018 06:57 PM, Peter Krempa wrote:
> On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
>> While this leak happens in tests only, it is still worth fixing.
>>
>> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are
>> definitely lost in loss record 325 of 331
>> ==12962==at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
>> ==12962==by 0x5D285D5: virAlloc (viralloc.c:144)
>> ==12962==by 0x5E823EB: virCPUGetHost (cpu.c:411)
>> ==12962==by 0x56DE953: virQEMUCapsInitHostCPUModel
>> (qemu_capabilities.c:2874)
>> ==12962==by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
>> ==12962==by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
>> ==12962==by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
>> ==12962==by 0x13AD0B: virTestMain (testutils.c:1120)
>> ==12962==by 0x1372FD: main (qemuxml2argvtest.c:2883)
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_capabilities.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index e2e76e4dd8..9ec9089d5b 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
>> {
>> virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps,
>> type);
>>
>> +virCPUDefFree(cpuData->reported);
>> +virCPUDefFree(cpuData->migratable);
>> +virCPUDefFree(cpuData->full);
>
> This looks fishy. The test code always unrefs the qemuCaps object, so
> there should be no leak. Could you please elaborate on how this happens?
Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
(which exists only for sake of tests) which does two things:
1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
where cpuData is firstly allocated
2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
whatever was loaded in 1).
>
> My problem is that this is in the setter code.
>
Not at all. It's pretty common that setter frees whatever it is
replacing (e.g. virQEMUCapsSetGICCapabilities()).
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] audit: Enforce enum switch type cast in virDomainAuditHostdev
There was a missing enum for mdev causing a strange 'unknown device type'
warning when hot-plugging mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1583927
Signed-off-by: Erik Skultety
---
src/conf/domain_audit.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 82868bca76..14138d93af 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -361,6 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
virDomainHostdevSubsysSCSIVHostPtr hostsrc =
&hostdev->source.subsys.u.scsi_host;
+virDomainHostdevSubsysMediatedDevPtr mdevsrc =
&hostdev->source.subsys.u.mdev;
virUUIDFormat(vm->def->uuid, uuidstr);
if (!(vmname = virAuditEncode("vm", vm->def->name))) {
@@ -373,9 +374,9 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
virt = "?";
}
-switch (hostdev->mode) {
+switch ((virDomainHostdevMode) hostdev->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
-switch (hostdev->source.subsys.type) {
+switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
if (virAsprintfQuiet(&address, "%.4x:%.2x:%.2x.%.1x",
pcisrc->addr.domain,
@@ -419,6 +420,13 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
goto cleanup;
}
break;
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+if (VIR_STRDUP_QUIET(address, mdevsrc->uuidstr) < 0) {
+VIR_WARN("OOM while enconding audit message");
+goto cleanup;
+}
+break;
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
default:
VIR_WARN("Unexpected hostdev type while encoding audit message:
%d",
hostdev->source.subsys.type);
@@ -470,6 +478,7 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
}
break;
+case VIR_DOMAIN_HOSTDEV_MODE_LAST:
default:
VIR_WARN("Unexpected hostdev mode while encoding audit message: %d",
hostdev->mode);
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu: Add capability flag for setting the extended tseg size
On Wed, May 30, 2018 at 12:23:12PM -0400, John Ferlan wrote: On 05/21/2018 11:00 AM, Martin Kletzander wrote: Signed-off-by: Martin Kletzander --- src/qemu/qemu_capabilities.c | 10 +++ src/qemu/qemu_capabilities.h | 2 + .../caps_1.5.3.x86_64.replies | 38 +-- .../caps_1.5.3.x86_64.xml | 3 +- .../caps_1.6.0.x86_64.replies | 38 +-- .../caps_1.6.0.x86_64.xml | 3 +- .../caps_1.7.0.x86_64.replies | 38 +-- .../caps_1.7.0.x86_64.xml | 3 +- .../caps_2.1.1.x86_64.replies | 38 +-- .../caps_2.1.1.x86_64.xml | 3 +- .../caps_2.10.0.x86_64.replies| 48 ++--- .../caps_2.10.0.x86_64.xml| 3 +- .../caps_2.12.0.x86_64.replies| 67 +++ .../caps_2.12.0.x86_64.xml| 4 +- .../caps_2.4.0.x86_64.replies | 38 +-- .../caps_2.4.0.x86_64.xml | 3 +- .../caps_2.5.0.x86_64.replies | 40 +-- .../caps_2.5.0.x86_64.xml | 3 +- .../caps_2.6.0.x86_64.replies | 40 +-- .../caps_2.6.0.x86_64.xml | 3 +- .../caps_2.7.0.x86_64.replies | 40 +-- .../caps_2.7.0.x86_64.xml | 3 +- .../caps_2.8.0.x86_64.replies | 40 +-- .../caps_2.8.0.x86_64.xml | 3 +- .../caps_2.9.0.x86_64.replies | 48 ++--- .../caps_2.9.0.x86_64.xml | 3 +- 26 files changed, 458 insertions(+), 104 deletions(-) Is there no other way to determine this without getting mch? and needing to update all those replies from earlier releases? I assume those are there because "mch" exists in 1.5.3 and beyond, but we never checked for it. So did you update the .replies files manually or did you run this against each version mentioned? I, personally ran tests/qemucapsprobe for newest QEMU from git and for the oldest one we have replies for in the repo (1.5.3). That way I got the reply for positive and negative result. I then copied the positive one to all QEMU versions that support it and the other one to those that don't. Then the next step is pretty important, I added the parameter to all of the outputs to check that the reply is in the right position in the file. I then fixed those where the position was slightly different (cleanly seen by the capability not being ther even though it should), removed the parameter and ran tests/qemucapsfixreplies for all the files. I'll add that to the commit message and I'll be reposting the series again anyway, so you can say whether that's looks like you imagined it or not. Personally I think it's always a bonus if how the replies adjustments were made is described. It perhaps helps the next person with the same conundrum. Everyone is talking about how we could have some images with all the QEMUs and automatically update it... But because this is once per (quite a long) time that someone has to do this nobody is really pressed to make this more streamlined, because once you fix it for your particular capability, you no longer have the need. Pavel is closest to this as he has bunch of VMs, one for each QEMU version, but still has to run the update manually. Is there no other way to get this without supplying the "mch"/MCH as well? It's a parameter of the MCH device that exists (or rather is exposed) only on x86_64 machines (MCH is/used to be the north bridge in the PC). Without knowing whether it exists we don't know if we can ask QEMU about the device. We could do that without the capability itself and then try to parse the error message etc., but why? Also this just is how the virQEMUCapsObjectTypeProps is structured. In any case, with some minor updates to the commit message to give a synopsis related to how the .replies were updated... Reviewed-by: John Ferlan John psst, it would be nice if you removed the irrelevant parts below in your reply, like this, look: signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] Fixup USB input bus check
Signed-off-by: Xiao Feng Ren --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27e2bd50eb..e4d39c06e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27878,7 +27878,7 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev) if ((t == VIR_DOMAIN_DEVICE_DISK && dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) || (t == VIR_DOMAIN_DEVICE_INPUT && - dev->data.input->type == VIR_DOMAIN_INPUT_BUS_USB) || + dev->data.input->bus == VIR_DOMAIN_INPUT_BUS_USB) || (t == VIR_DOMAIN_DEVICE_HOSTDEV && dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == -- 2.16.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/1] bug: fixup USB input bus check
There's one explicit bug(introduced in commit:317badb) when checking the bus of one input device is USB. Let's fix it. Xiao Feng Ren (1): Fixup USB input bus check src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.16.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] qemu: Move checks for SMM from command-line creation into validation phase
adding qemu-devel, Paolo and Gerd; comments below:
On 05/30/18 23:08, Martin Kletzander wrote:
> On Wed, May 30, 2018 at 11:02:59AM -0400, John Ferlan wrote:
>>
>>
>> On 05/21/2018 11:00 AM, Martin Kletzander wrote:
>>> We are still hoping all of such checks will be moved there and this
>>> is one small
>>> step in that direction.
>>>
>>> One of the things that this is improving is the error message you get
>>> when
>>> starting a domain with SMM and i440fx, for example. Instead of
>>> saying that the
>>> QEMU binary doesn't support that option, we correctly say that it is
>>> only
>>> supported with q35 machine type.
>>>
>>> Signed-off-by: Martin Kletzander
>>> ---
>>> src/qemu/qemu_capabilities.c | 21 +++--
>>> src/qemu/qemu_capabilities.h | 4 ++--
>>> src/qemu/qemu_command.c | 12 ++--
>>> src/qemu/qemu_domain.c | 12 +---
>>> 4 files changed, 28 insertions(+), 21 deletions(-)
>>>
>>
>> I know it's outside the bounds of what you're doing; however,
>> qemuDomainDefValidateFeatures could check the capabilities for other
>> bits too...
>>
>
> Probably, but I mostly wanted to do that because SMM is not only about the
> capability, but also about the machine. Good idea for the future, though.
>
>> [...]
>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index d3beee5d8760..881d0ea46a75 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -3430,7 +3430,8 @@ qemuDomainDefGetVcpuHotplugGranularity(const
>>> virDomainDef *def)
>>>
>>>
>>> static int
>>> -qemuDomainDefValidateFeatures(const virDomainDef *def)
>>> +qemuDomainDefValidateFeatures(const virDomainDef *def,
>>> + virQEMUCapsPtr qemuCaps)
>>> {
>>> size_t i;
>>>
>>> @@ -3477,6 +3478,12 @@ qemuDomainDefValidateFeatures(const
>>> virDomainDef *def)
>>> }
>>> break;
>>>
>>> + case VIR_DOMAIN_FEATURE_SMM:
>>> + if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
>>
>> Probably should change to != _ABSENT, since qemu_command will supply
>> smm={on|off}
>>
>
> That makes sense, kind of. For 'off' we only need to check if we can
> specify
> the smm= option. The thing is that you can even specify smm=on with
> non-q35
> machine type, but it is unclear what that's going to mean since it doesn't
> really make sense.
>
> @Laszlo: What would you say? Should we allow users to specify smm=on
> for users?
> Or even better, does it makes sense to allow specifying smm=anything for
> non-q35
> machine types? If not, we'll leave it like this, that is smm=anything is
> forbidden for non-q35 machine types.
Technically the "smm" machine type property is defined for both i440fx
and q35:
$ qemu-system-x86_64 -machine pc,\? 2>&1 | grep smm
pc-i440fx-2.11.smm=OnOffAuto (Enable SMM (pc & q35))
$ qemu-system-x86_64 -machine q35,\? 2>&1 | grep smm
pc-q35-2.11.smm=OnOffAuto (Enable SMM (pc & q35))
The "smm" property controls whether system management mode is emulated,
to my knowledge. That's an x86 processor operating mode, which isn't
specific to the Q35 board. What's specific to Q35 is the TSEG.
The primary reason why OVMF (built with the edk2 SMM driver stack)
requires Q35 is not that i440fx does not emulate SMM, it's that i440fx
doesn't have a large enough SMRAM area. (The legacy SMRAM areas are too
small for OVMF.)
For example, SeaBIOS too includes some SMM code (optionally, if it's
built like that), and that runs on i440fx just fine, because the legacy
SMRAM areas are large enough for SeaBIOS's purposes, AIUI.
(Now, there's at least one other reason why OVMF/SMM needs Q35: because
the SMI broadcast feature too is only implemented on Q35. But that's
rather a consequence of the above "primary reason", and not a
stand-alone reason itself -- we restricted the SMI broadcast feature to
Q35 *because* OVMF could only run on Q35. Anyhow, you need not care
about SMI broadcast because it's automatically negotiated between guest
fw and QEMU.)
Summary:
(1) For making Secure Boot actually secure in OVMF, OVMF needs to be
built with -D SMM_REQUIRE, so that it include the SMM driver stack.
(2) Booting such a firmware binary requires Q35, because only Q35 offers
TSEG, and the legacy -- non-TSEG -- SMRAM ranges are too small even for
a "basic boot" of OVMF.
(3) QEMU has to be configured with "-machine smm=on"; this is already
covered by libvirt via .
(4) QEMU has to be configured to restrict pflash writes to code that
executes in SMM, with "-global
driver=cfi.pflash01,property=secure,value=on". Libvirt covers this
already with the @secure=yes attribute in .
(5) There are two *quality of service* features related to SMM; with
both of them being Q35-only. The first feature is SMI broadcast; libvirt
need not care because it's auto-negotiated.
(6) The second QoS feature is *extended* TSEG (a paravirt feature on top
of the standard TSEG), which lets the edk2 SMM driver stack scale to
large RAM sizes and
Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size
On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
This is way too sparse.
I can't really think of what to say here that is not already mentioned in the
documentation or self-explanatory in the code. But maybe only I see that as
self-explanatory. I'll write something up here.
On 05/21/2018 11:00 AM, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_command.c | 18
src/qemu/qemu_domain.c| 84 +++
.../qemuxml2argvdata/tseg-explicit-size.args | 28 +++
tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +
tests/qemuxml2argvdata/tseg-i440fx.xml| 23 +
tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +
.../tseg-old-machine-type.args| 27 ++
.../tseg-old-machine-type.xml | 21 +
tests/qemuxml2argvdata/tseg.args | 28 +++
tests/qemuxml2argvdata/tseg.xml | 21 +
tests/qemuxml2argvtest.c | 48 +++
.../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++
.../tseg-old-machine-type.xml | 44 ++
tests/qemuxml2xmloutdata/tseg.xml | 46 ++
tests/qemuxml2xmltest.c | 25 ++
15 files changed, 505 insertions(+)
create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
create mode 100644 tests/qemuxml2argvdata/tseg.args
create mode 100644 tests/qemuxml2argvdata/tseg.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 328f3c0a2386..36f557676fb0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7200,6 +7200,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
return ret;
}
+
+static void
+qemuBuildTSEGCommandLine(virCommandPtr cmd,
+ const virDomainDef *def)
+{
+if (!def->tseg_size)
Since it's not bool or pointer, prefer the tseg_size == 0
I don't know if you mean that as indicative or imperative. It is very
subjective and for such scenarios I would like to execute my right to mention
that it is not part of Contributor Guidelines. I know it might seem rude, but
this is one of the things that's very subjective and for such things I like to
first reach a consensus before I change what I'm used to writing. I hope you
understand that.
+return;
+
+virCommandAddArg(cmd, "-global");
+
+/* PostParse callback guarantees that the size is divisible by 1 MiB */
+virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu",
+ def->tseg_size >> 20);
+}
+
+
static int
qemuBuildSmpCommandLine(virCommandPtr cmd,
virDomainDefPtr def)
@@ -9921,6 +9937,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0)
goto error;
+qemuBuildTSEGCommandLine(cmd, def);
+
if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
goto error;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 881d0ea46a75..3ea9e3d47344 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
}
+static int
+qemuDomainSetDefaultTsegSize(virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+const char *machine = NULL;
+char *end_ptr = NULL;
+unsigned int major = 0;
+unsigned int minor = 0;
+
+def->tseg_size = 0;
Pointless since the only way in here is "if (tseg_size == 0)"
My bad for not commenting the function. But with this line the function
is self-sufficient and clearly does what the name suggests (set the
default TSEG size) no matter where it is called from and under what
circumstances/conditions.
+
+if (!qemuDomainIsQ35(def))
+return 0;
+
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
Reading this now makes me realized _MBYTES is probably unnecessary, IDC
though since it does follow the QEMU name.
That's exactly why I chose that naming (after changing it many, _many_
times). I imagined there will come a time when QEMU adds an option
named mch.extended_tseg(_maybe_some_suffix) and the person adding a
capability for that will not adjust the naming on this one. I see this
as:
a) rather safe than sorry
b) very much precise with what it descr
Re: [libvirt] [PATCH 1/3] virDomainDefParseXML: Free @tmp when parsing genid
On Thu, May 31, 2018 at 09:16:10 +0200, Michal Privoznik wrote:
> On 05/30/2018 06:46 PM, Peter Krempa wrote:
> > On Wed, May 30, 2018 at 18:04:27 +0200, Michal Privoznik wrote:
[...]
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 27e2bd50eb..d8b4fbd6b0 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> >> "%s", _("malformed genid element"));
> >> goto error;
> >> }
> >> +VIR_FREE(tmp);
> >
> >
> > ACK
> >
>
> And safe for freeze? ;-) This one fixes a feature that was introduced in
> this release. The rest of the patches can go after the release.
Yes. I thought it was obvious :)
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virDomainDefParseXML: Free @tmp when parsing genid
On 05/30/2018 06:46 PM, Peter Krempa wrote:
> On Wed, May 30, 2018 at 18:04:27 +0200, Michal Privoznik wrote:
>> We need to free return value of virXPathString().
>>
>> ==12962== 37 bytes in 1 blocks are definitely lost in loss record 156 of 331
>> ==12962==at 0x4C2AF0F: malloc (vg_replace_malloc.c:299)
>> ==12962==by 0x91E8439: strdup (in /lib64/libc-2.25.so)
>> ==12962==by 0x5DBD551: virStrdup (virstring.c:977)
>> ==12962==by 0x5DD3E5E: virXPathString (virxml.c:84)
>> ==12962==by 0x5E178AB: virDomainDefParseXML (domain_conf.c:19110)
>> ==12962==by 0x5E1E985: virDomainDefParseNode (domain_conf.c:20885)
>> ==12962==by 0x5E1E7CB: virDomainDefParse (domain_conf.c:20827)
>> ==12962==by 0x5E1E871: virDomainDefParseFile (domain_conf.c:20853)
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/conf/domain_conf.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 27e2bd50eb..d8b4fbd6b0 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19120,6 +19120,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>> "%s", _("malformed genid element"));
>> goto error;
>> }
>> +VIR_FREE(tmp);
>
>
> ACK
>
And safe for freeze? ;-) This one fixes a feature that was introduced in
this release. The rest of the patches can go after the release.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
