Re: Libvirt NVME support
On Wed, Nov 18, 2020 at 20:31:03 +0100, Michal Privoznik wrote: > On 11/18/20 11:24 AM, Peter Krempa wrote: > > On Wed, Nov 18, 2020 at 09:57:14 +, Thanos Makatos wrote: [...] > Would it make sense to relax the current limitation in libvirt and allow > (which is meant for cases where the backend is a NVMe > disk) to be on something else than 'virtio' bus? This is really orthogonal to the emulated NVMe controller. A can theoretically back any disk frontend. I don't remember now why we actually mandate it only for virtio. Do you? Apart from that, it doesn't make that much sense to use
Re: [PATCH 1/2] hyperv: XML parsing of storage volumes
> On Nov 18, 2020, at 1:48 PM, Matt Coleman wrote:
>
> +if (!(disk = virDomainDiskDefNew(priv->xmlopt))) {
>
> +if (!(disk = virDomainDiskDefNew(priv->xmlopt))) {
I forgot to include changes to hyperv_private.h in this commit. Please
squash the following additional change with this patch...
Signed-off-by: Matt Coleman
diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h
index f400f58c3a..7a2a1d59ee 100644
--- a/src/hyperv/hyperv_private.h
+++ b/src/hyperv/hyperv_private.h
@@ -28,10 +28,12 @@
#include "virerror.h"
#include "hyperv_util.h"
#include "capabilities.h"
+#include "domain_conf.h"
typedef struct _hypervPrivate hypervPrivate;
struct _hypervPrivate {
hypervParsedUri *parsedUri;
WsManClient *client;
virCapsPtr caps;
+virDomainXMLOptionPtr xmlopt;
};
[PATCH] NEWS: support phytium processor
From: Shaojun Yang FT-2000+ and Tengyun-S2500 are two chips by produced by Phytium Technology Co., Ltd., which based on ARMv8 architecture. Shaojun Yang (1): add phytium FT-2000+ and Tengyun-S2500 support on arm architecture. src/cpu_map/arm_FT-2000plus.xml | 6 ++ src/cpu_map/arm_Tengyun-S2500.xml | 6 ++ src/cpu_map/arm_vendors.xml | 1 + src/cpu_map/index.xml | 4 src/cpu_map/meson.build | 2 ++ 5 files changed, 19 insertions(+) create mode 100644 src/cpu_map/arm_FT-2000plus.xml create mode 100644 src/cpu_map/arm_Tengyun-S2500.xml -- 2.7.4
[PATCH] add phytium FT-2000+ and Tengyun-S2500 support on arm architecture.
From: Shaojun Yang Signed-off-by: Shaojun Yang --- src/cpu_map/arm_FT-2000plus.xml | 6 ++ src/cpu_map/arm_Tengyun-S2500.xml | 6 ++ src/cpu_map/arm_vendors.xml | 1 + src/cpu_map/index.xml | 4 src/cpu_map/meson.build | 2 ++ 5 files changed, 19 insertions(+) create mode 100644 src/cpu_map/arm_FT-2000plus.xml create mode 100644 src/cpu_map/arm_Tengyun-S2500.xml diff --git a/src/cpu_map/arm_FT-2000plus.xml b/src/cpu_map/arm_FT-2000plus.xml new file mode 100644 index 000..b532f65 --- /dev/null +++ b/src/cpu_map/arm_FT-2000plus.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/src/cpu_map/arm_Tengyun-S2500.xml b/src/cpu_map/arm_Tengyun-S2500.xml new file mode 100644 index 000..22b865e --- /dev/null +++ b/src/cpu_map/arm_Tengyun-S2500.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/src/cpu_map/arm_vendors.xml b/src/cpu_map/arm_vendors.xml index ff799ef..4465463 100644 --- a/src/cpu_map/arm_vendors.xml +++ b/src/cpu_map/arm_vendors.xml @@ -11,4 +11,5 @@ + diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml index 08f052e..065d9ae 100644 --- a/src/cpu_map/index.xml +++ b/src/cpu_map/index.xml @@ -103,5 +103,9 @@ + + + + diff --git a/src/cpu_map/meson.build b/src/cpu_map/meson.build index b86612b..7eeec6d 100644 --- a/src/cpu_map/meson.build +++ b/src/cpu_map/meson.build @@ -5,6 +5,8 @@ cpumap_data = [ 'arm_Falkor.xml', 'arm_features.xml', 'arm_Kunpeng-920.xml', + 'arm_FT-2000plus.xml', + 'arm_Tengyun-S2500.xml', 'arm_ThunderX299xx.xml', 'arm_vendors.xml', 'index.xml', -- 2.7.4
[PATCH] virsh: Added attach-disk support for network disk
Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport
Added documentation to virsh.rst specifying usage.
Signed-off-by: Ryan Gahagan
---
docs/manpages/virsh.rst | 31 ++---
tools/virsh-domain.c| 135 +++-
2 files changed, 145 insertions(+), 21 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 1ae6d1a0d4..36c868a3e6 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4538,14 +4538,18 @@ attach-disk
[--current]] | [--persistent]] [--targetbus bus]
[--driver driver] [--subdriver subdriver] [--iothread iothread]
[--cache cache] [--io io] [--type type] [--alias alias]
- [--mode mode] [--sourcetype sourcetype] [--serial serial]
- [--wwn wwn] [--rawio] [--address address] [--multifunction]
- [--print-xml]
+ [--mode mode] [--sourcetype sourcetype]
+ [--source-protocol protocol] [--source-host-name hostname:port]
+ [--source-host-transport transport] [--source-host-socket socket]
+ [--serial serial] [--wwn wwn] [--rawio] [--address address]
+ [--multifunction] [--print-xml]
Attach a new disk device to the domain.
-*source* is path for the files and devices. *target* controls the bus or
-device under which the disk is exposed to the guest OS. It indicates the
-"logical" device name; the optional *targetbus* attribute specifies the type
+*source* is path for the files and devices unless *--source-protocol*
+is specified, in which case *source* is the name of a network disk.
+*target* controls the bus or device under which the disk is exposed
+to the guest OS. It indicates the "logical" device name;
+the optional *targetbus* attribute specifies the type
of disk device to emulate; possible values are driver specific, with typical
values being *ide*, *scsi*, *virtio*, *xen*, *usb*, *sata*, or *sd*, if
omitted, the bus type is inferred from the style of the device name (e.g. a
@@ -4563,7 +4567,7 @@ within the existing virtual cdrom or floppy device;
consider using
``update-device`` for this usage instead.
*alias* can set user supplied alias.
*mode* can specify the two specific mode *readonly* or *shareable*.
-*sourcetype* can indicate the type of source (block|file)
+*sourcetype* can indicate the type of source (block|file|network)
*cache* can be one of "default", "none", "writethrough", "writeback",
"directsync" or "unsafe".
*io* controls specific policies on I/O; QEMU guests support "threads",
@@ -4579,6 +4583,19 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their
cssid set to 0xfe.
*multifunction* indicates specified pci address is a multifunction pci device
address.
+There is also support for using a network disk. As specified, the user can
+provide a *--source-protocol* in which case the *source* parameter will
+be interpreted as the source name. *--source-protocol* must be provided
+if the user intends to provide a network disk or host information.
+Host information can be provided using the tags
+*--source-host-name*, *--source-host-transport*, and *--source-host-socket*,
+which respectively denote the name of the host, the host's transport method,
+and the socket that the host uses. *--source-host-socket* and
+*--source-host-name* cannot both be provided, and the user must provide a
+*--source-host-transport* if they want to provide a *--source-host-socket*.
+The *--source-host-name* parameter supports host:port syntax
+if the user wants to provide a port as well.
+
If *--print-xml* is specified, then the XML of the disk that would be attached
is printed instead.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c999458d72..1303676c33 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -222,7 +222,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
{.name = "source",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
- .help = N_("source of disk device")
+ .help = N_("source of disk device or name of network disk")
},
{.name = "target",
.type = VSH_OT_DATA,
@@ -268,7 +268,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
},
{.name = "sourcetype",
.type = VSH_OT_STRING,
- .help = N_("type of source (block|file)")
+ .help = N_("type of source (block|file|network)")
},
{.name = "serial",
.type = VSH_OT_STRING,
@@ -298,6 +298,22 @@ static const vshCmdOptDef opts_attach_disk[] = {
.type = VSH_OT_BOOL,
.help = N_("print XML document rather than attach the disk")
},
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name =
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On Tue, 17 Nov 2020, Neal Gompa wrote: > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt > wrote: > > > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik > > wrote: > > > > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote: > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a > > > > wrapper > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use > > > > for quite a while anymore, but required to work for compatibility e.g. > > > > when migrating in old guests. > > > > > > > > For years this was a symlink kvm-spice->kvm and therefore covered > > > > apparmor-wise by the existing entry: > > > > /usr/bin/kvm rmix, > > > > But due to a recent change [1] in qemu packaging this now is no symlink, > > > > but a wrapper on its own and therefore needs an own entry that allows it > > > > to be executed. > > > > > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > --- > > > > src/security/apparmor/libvirt-qemu | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > Reviewed-by: Michal Privoznik > > > > Thank you Michal, > > it also passed fine through my tests (as backport to 6.8 and 6.9). > > We are not in any freeze, review has happened, tests LGTM - pushed to git. > > > > Hold up, why was this merged? Did anyone validate whether this would > break the other AppArmor user (SUSE)? > > Unlike SELinux, AppArmor functionality is quite fragmented between > Ubuntu and SUSE distributions (the two major users of AppArmor), and > there did not seem to be any indication that this AppArmor patch was > validated with openSUSE before merging. My personal experience with > AppArmor across the two distribution families is that it's really easy > to make profiles that work for Ubuntu but fail on SUSE because of the > disparity of functionality. I also don't see Jim Fehlig stepping in to > indicate that this worked for him. > > I haven't had a chance to test this myself, but I am immediately > suspicious of a change that references a commit based on Debian > packaging of QEMU. Others have referred to how this list handles SUSE policies, but I'll point out that the request was for a simple file rule that only adds additional access. This should be no problem at all on SUSE. Outside of this rule, the apparmor userspace understands kernel differences and various rules and any modern SUSE would have a new enough parser to handle the various rules syntax we use in the current libvirt policy and be parseable without issues. The typical distro pattern for new rule syntax would be that when a distro pulled in a new libvirt with new policy syntax that the distro doesn't support, then it would be abundantly clear to the distro maintainer when the parser failed and the distro would either choose to upgrade apparmor or patch out the problematic rules. Hope this helps -- Jamie Strandboge | http://www.canonical.com
[PATCH 2/2] schema: add support for Windows file paths and device names
Signed-off-by: Matt Coleman
---
docs/schemas/basictypes.rng | 2 +-
docs/schemas/domaincommon.rng | 5 +++-
.../disk-hyperv-physical.xml | 17 ++
.../disk-hyperv-virtual.xml | 17 ++
.../disk-hyperv-physical.xml | 23 +++
.../disk-hyperv-virtual.xml | 23 +++
tests/genericxml2xmltest.c| 2 ++
7 files changed, 87 insertions(+), 2 deletions(-)
create mode 100644 tests/genericxml2xmlindata/disk-hyperv-physical.xml
create mode 100644 tests/genericxml2xmlindata/disk-hyperv-virtual.xml
create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-physical.xml
create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-virtual.xml
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index ea18b2d2fb..fc52799466 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -299,7 +299,7 @@
- /.+
+ (/|[a-zA-Z]:\\).+
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f86a854863..da9d1da187 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1691,7 +1691,10 @@
-
+
+
+
+
diff --git a/tests/genericxml2xmlindata/disk-hyperv-physical.xml
b/tests/genericxml2xmlindata/disk-hyperv-physical.xml
new file mode 100644
index 00..a7f34c18f4
--- /dev/null
+++ b/tests/genericxml2xmlindata/disk-hyperv-physical.xml
@@ -0,0 +1,17 @@
+
+ test_disk-hyperv
+ deadbeef-dead-beef-dead-beefdeadbeef
+ 1048576
+ 3
+
+hvm
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/genericxml2xmlindata/disk-hyperv-virtual.xml
b/tests/genericxml2xmlindata/disk-hyperv-virtual.xml
new file mode 100644
index 00..bbc56c9f89
--- /dev/null
+++ b/tests/genericxml2xmlindata/disk-hyperv-virtual.xml
@@ -0,0 +1,17 @@
+
+ test_disk-hyperv
+ deadbeef-dead-beef-dead-beefdeadbeef
+ 1048576
+ 3
+
+hvm
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/genericxml2xmloutdata/disk-hyperv-physical.xml
b/tests/genericxml2xmloutdata/disk-hyperv-physical.xml
new file mode 100644
index 00..112a5081cd
--- /dev/null
+++ b/tests/genericxml2xmloutdata/disk-hyperv-physical.xml
@@ -0,0 +1,23 @@
+
+ test_disk-hyperv
+ deadbeef-dead-beef-dead-beefdeadbeef
+ 1048576
+ 1048576
+ 3
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+
+
+
+
+
+
+
+
diff --git a/tests/genericxml2xmloutdata/disk-hyperv-virtual.xml
b/tests/genericxml2xmloutdata/disk-hyperv-virtual.xml
new file mode 100644
index 00..dadd3318ab
--- /dev/null
+++ b/tests/genericxml2xmloutdata/disk-hyperv-virtual.xml
@@ -0,0 +1,23 @@
+
+ test_disk-hyperv
+ deadbeef-dead-beef-dead-beefdeadbeef
+ 1048576
+ 1048576
+ 3
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+
+
+
+
+
+
+
+
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 5110bfba86..5e8f58b4a2 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -154,6 +154,8 @@ mymain(void)
DO_TEST_FULL(name, 1, false, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS)
DO_TEST_DIFFERENT("disk-virtio");
+DO_TEST_DIFFERENT("disk-hyperv-physical");
+DO_TEST_DIFFERENT("disk-hyperv-virtual");
DO_TEST_DIFFERENT("graphics-vnc-minimal");
DO_TEST_DIFFERENT("graphics-vnc-manual-port");
--
2.27.0
[PATCH 0/2] hyperv: storage volume XML changes
Here's a GitLab MR if you'd prefer to review it there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/12/commits Matt Coleman (2): hyperv: XML parsing of storage volumes schema: add support for Windows file paths and device names docs/schemas/basictypes.rng | 2 +- docs/schemas/domaincommon.rng | 5 +- src/hyperv/hyperv_driver.c| 408 +- src/hyperv/hyperv_driver.h| 3 + src/hyperv/hyperv_wmi.c | 45 ++ src/hyperv/hyperv_wmi.h | 8 + src/hyperv/hyperv_wmi_classes.h | 19 + src/hyperv/hyperv_wmi_generator.input | 134 ++ .../disk-hyperv-physical.xml | 17 + .../disk-hyperv-virtual.xml | 17 + .../disk-hyperv-physical.xml | 23 + .../disk-hyperv-virtual.xml | 23 + tests/genericxml2xmltest.c| 2 + 13 files changed, 703 insertions(+), 3 deletions(-) create mode 100644 tests/genericxml2xmlindata/disk-hyperv-physical.xml create mode 100644 tests/genericxml2xmlindata/disk-hyperv-virtual.xml create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-physical.xml create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-virtual.xml -- 2.27.0
[PATCH 1/2] hyperv: XML parsing of storage volumes
dumpxml can now serialize:
* floppy drives
* file-backed and device-backed disk drives
* images mounted to virtual CD/DVD drives
* IDE and SCSI controllers
Co-authored-by: Sri Ramanujam
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c| 408 +-
src/hyperv/hyperv_driver.h| 3 +
src/hyperv/hyperv_wmi.c | 45 +++
src/hyperv/hyperv_wmi.h | 8 +
src/hyperv/hyperv_wmi_classes.h | 19 ++
src/hyperv/hyperv_wmi_generator.input | 134 +
6 files changed, 616 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 40739595ac..ce9ba2a02a 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -293,6 +293,385 @@ hypervCapsInit(hypervPrivate *priv)
return NULL;
}
+/*
+ * Virtual device functions
+ */
+static int
+hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId,
+ Msvm_ResourceAllocationSettingData *list,
+ Msvm_ResourceAllocationSettingData **out)
+{
+Msvm_ResourceAllocationSettingData *entry = list;
+g_autofree char *escapedDeviceId = NULL;
+*out = NULL;
+
+while (entry) {
+escapedDeviceId = g_strdup_printf("%s\"", entry->data->InstanceID);
+escapedDeviceId = virStringReplace(escapedDeviceId, "\\", "");
+
+if (g_str_has_suffix(parentDeviceId, escapedDeviceId)) {
+*out = entry;
+break;
+}
+
+entry = entry->next;
+}
+
+if (*out)
+return 0;
+
+return -1;
+}
+
+
+
+/*
+ * Functions for deserializing device entries
+ */
+static int
+hypervDomainDefAppendController(virDomainDefPtr def,
+int idx,
+virDomainControllerType controllerType)
+{
+virDomainControllerDefPtr controller = NULL;
+
+if (!(controller = virDomainControllerDefNew(controllerType)))
+return -1;
+
+controller->idx = idx;
+
+if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, controller) <
0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseIDEController(virDomainDefPtr def, int idx)
+{
+return hypervDomainDefAppendController(def, idx,
VIR_DOMAIN_CONTROLLER_TYPE_IDE);
+}
+
+
+static int
+hypervDomainDefParseSCSIController(virDomainDefPtr def, int idx)
+{
+return hypervDomainDefAppendController(def, idx,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+}
+
+
+static int
+hypervDomainDefAppendDisk(virDomainDefPtr def,
+ virDomainDiskDefPtr disk,
+ virDomainDiskBus busType,
+ int diskNameOffset,
+ const char *diskNamePrefix,
+ int maxControllers,
+ Msvm_ResourceAllocationSettingData **controllers,
+ Msvm_ResourceAllocationSettingData *diskParent,
+ Msvm_ResourceAllocationSettingData *diskController)
+{
+size_t i = 0;
+int ctrlr_idx = -1;
+int addr = -1;
+
+if (virStrToLong_i(diskParent->data->AddressOnParent, NULL, 10, ) < 0)
+return -1;
+
+if (addr < 0)
+return -1;
+
+/* Find controller index */
+for (i = 0; i < maxControllers; i++) {
+if (diskController == controllers[i]) {
+ctrlr_idx = i;
+break;
+}
+}
+
+if (ctrlr_idx < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find
controller for disk!"));
+return -1;
+}
+
+disk->bus = busType;
+disk->dst = virIndexToDiskName(ctrlr_idx * diskNameOffset + addr,
diskNamePrefix);
+disk->info.addr.drive.controller = ctrlr_idx;
+disk->info.addr.drive.bus = 0;
+disk->info.addr.drive.target = 0;
+disk->info.addr.drive.unit = addr;
+
+if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseFloppyStorageExtent(virDomainDefPtr def,
virDomainDiskDefPtr disk)
+{
+disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+disk->dst = g_strdup("fda");
+
+if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseVirtualExtent(hypervPrivate *priv,
+ virDomainDefPtr def,
+ Msvm_StorageAllocationSettingData
*disk_entry,
+ Msvm_ResourceAllocationSettingData *rasd,
+ Msvm_ResourceAllocationSettingData
**ideControllers,
+ Msvm_ResourceAllocationSettingData
**scsiControllers)
+{
+Msvm_ResourceAllocationSettingData *diskParent = NULL;
+Msvm_ResourceAllocationSettingData *controller = NULL;
+virDomainDiskDefPtr disk = NULL;
+int result = -1;
+
+if
[PATCH] util: convert char pointers to use g_autofree
From: Barrett Schonefeld
additional conversions to the GLib API in src/util per issue #11.
Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11
Signed-off-by: Barrett Schonefeld
---
src/util/vircgroupv1.c | 3 +-
src/util/virdnsmasq.c| 43 -
src/util/virfile.c | 9 ++---
src/util/virhostcpu.c| 4 +-
src/util/virlockspace.c | 6 +--
src/util/virlog.c| 12 ++
src/util/virmacmap.c | 3 +-
src/util/virnetdevbandwidth.c| 48 ---
src/util/virresctrl.c| 25
src/util/virrotatingfile.c | 11 ++
src/util/virscsihost.c | 25 +---
src/util/virsecret.c | 14 +++
src/util/virstorageencryption.c | 19 +++--
src/util/virstoragefilebackend.c | 4 +-
src/util/virsysinfo.c| 18 +++--
src/util/viruri.c| 7 +---
src/util/virutil.c | 66 +++-
src/util/virvhba.c | 57 ++-
src/util/virxml.c| 7 +---
19 files changed, 130 insertions(+), 251 deletions(-)
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 731e9d61d4..984cd50409 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
unsigned long long *unevictable)
{
int ret = -1;
-char *stat = NULL;
+g_autofree char *stat = NULL;
char *line = NULL;
unsigned long long cacheVal = 0;
unsigned long long activeAnonVal = 0;
@@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
ret = 0;
cleanup:
-VIR_FREE(stat);
return ret;
}
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index 9030f9218a..93bc4a129f 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -164,7 +164,7 @@ addnhostsWrite(const char *path,
dnsmasqAddnHost *hosts,
unsigned int nhosts)
{
-char *tmp;
+g_autofree char *tmp = NULL;
FILE *f;
bool istmp = true;
size_t i, j;
@@ -180,7 +180,7 @@ addnhostsWrite(const char *path,
istmp = false;
if (!(f = fopen(path, "w"))) {
rc = -errno;
-goto cleanup;
+return rc;
}
}
@@ -192,7 +192,7 @@ addnhostsWrite(const char *path,
if (istmp)
unlink(tmp);
-goto cleanup;
+return rc;
}
for (j = 0; j < hosts[i].nhostnames; j++) {
@@ -203,7 +203,7 @@ addnhostsWrite(const char *path,
if (istmp)
unlink(tmp);
-goto cleanup;
+return rc;
}
}
@@ -214,24 +214,21 @@ addnhostsWrite(const char *path,
if (istmp)
unlink(tmp);
-goto cleanup;
+return rc;
}
}
if (VIR_FCLOSE(f) == EOF) {
rc = -errno;
-goto cleanup;
+return rc;
}
if (istmp && rename(tmp, path) < 0) {
rc = -errno;
unlink(tmp);
-goto cleanup;
+return rc;
}
- cleanup:
-VIR_FREE(tmp);
-
return rc;
}
@@ -364,7 +361,7 @@ hostsfileWrite(const char *path,
dnsmasqDhcpHost *hosts,
unsigned int nhosts)
{
-char *tmp;
+g_autofree char *tmp = NULL;
FILE *f;
bool istmp = true;
size_t i;
@@ -380,7 +377,7 @@ hostsfileWrite(const char *path,
istmp = false;
if (!(f = fopen(path, "w"))) {
rc = -errno;
-goto cleanup;
+return rc;
}
}
@@ -392,24 +389,21 @@ hostsfileWrite(const char *path,
if (istmp)
unlink(tmp);
-goto cleanup;
+return rc;
}
}
if (VIR_FCLOSE(f) == EOF) {
rc = -errno;
-goto cleanup;
+return rc;
}
if (istmp && rename(tmp, path) < 0) {
rc = -errno;
unlink(tmp);
-goto cleanup;
+return rc;
}
- cleanup:
-VIR_FREE(tmp);
-
return rc;
}
@@ -686,15 +680,13 @@ static int
dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path)
{
int ret = -1;
-char *buf = NULL;
+g_autofree char *buf = NULL;
if (virFileReadAll(path, 1024 * 1024, ) < 0)
-goto cleanup;
+return ret;
ret = dnsmasqCapsSetFromBuffer(caps, buf);
- cleanup:
-VIR_FREE(buf);
return ret;
}
@@ -704,7 +696,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force)
int ret = -1;
struct stat sb;
virCommandPtr cmd = NULL;
-char *help = NULL, *version = NULL, *complete = NULL;
+g_autofree char *help = NULL;
+g_autofree char *version = NULL;
+g_autofree char *complete = NULL;
if (!caps || caps->noRefresh)
Re: [PATCH 00/23] hyperv: storage volume XML changes
Completely botched this submission. Disregard this thread... :shame-emoji: -- Matt
[PATCH 01/23] hyperv: XML parsing of storage volumes
dumpxml can now serialize:
* floppy drives
* file-backed and device-backed disk drives
* images mounted to virtual CD/DVD drives
* IDE and SCSI controllers
Co-authored-by: Sri Ramanujam
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c| 408 +-
src/hyperv/hyperv_driver.h| 3 +
src/hyperv/hyperv_wmi.c | 45 +++
src/hyperv/hyperv_wmi.h | 8 +
src/hyperv/hyperv_wmi_classes.h | 19 ++
src/hyperv/hyperv_wmi_generator.input | 134 +
6 files changed, 616 insertions(+), 1 deletion(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 40739595ac..ce9ba2a02a 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -293,6 +293,385 @@ hypervCapsInit(hypervPrivate *priv)
return NULL;
}
+/*
+ * Virtual device functions
+ */
+static int
+hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId,
+ Msvm_ResourceAllocationSettingData *list,
+ Msvm_ResourceAllocationSettingData **out)
+{
+Msvm_ResourceAllocationSettingData *entry = list;
+g_autofree char *escapedDeviceId = NULL;
+*out = NULL;
+
+while (entry) {
+escapedDeviceId = g_strdup_printf("%s\"", entry->data->InstanceID);
+escapedDeviceId = virStringReplace(escapedDeviceId, "\\", "");
+
+if (g_str_has_suffix(parentDeviceId, escapedDeviceId)) {
+*out = entry;
+break;
+}
+
+entry = entry->next;
+}
+
+if (*out)
+return 0;
+
+return -1;
+}
+
+
+
+/*
+ * Functions for deserializing device entries
+ */
+static int
+hypervDomainDefAppendController(virDomainDefPtr def,
+int idx,
+virDomainControllerType controllerType)
+{
+virDomainControllerDefPtr controller = NULL;
+
+if (!(controller = virDomainControllerDefNew(controllerType)))
+return -1;
+
+controller->idx = idx;
+
+if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, controller) <
0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseIDEController(virDomainDefPtr def, int idx)
+{
+return hypervDomainDefAppendController(def, idx,
VIR_DOMAIN_CONTROLLER_TYPE_IDE);
+}
+
+
+static int
+hypervDomainDefParseSCSIController(virDomainDefPtr def, int idx)
+{
+return hypervDomainDefAppendController(def, idx,
VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
+}
+
+
+static int
+hypervDomainDefAppendDisk(virDomainDefPtr def,
+ virDomainDiskDefPtr disk,
+ virDomainDiskBus busType,
+ int diskNameOffset,
+ const char *diskNamePrefix,
+ int maxControllers,
+ Msvm_ResourceAllocationSettingData **controllers,
+ Msvm_ResourceAllocationSettingData *diskParent,
+ Msvm_ResourceAllocationSettingData *diskController)
+{
+size_t i = 0;
+int ctrlr_idx = -1;
+int addr = -1;
+
+if (virStrToLong_i(diskParent->data->AddressOnParent, NULL, 10, ) < 0)
+return -1;
+
+if (addr < 0)
+return -1;
+
+/* Find controller index */
+for (i = 0; i < maxControllers; i++) {
+if (diskController == controllers[i]) {
+ctrlr_idx = i;
+break;
+}
+}
+
+if (ctrlr_idx < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find
controller for disk!"));
+return -1;
+}
+
+disk->bus = busType;
+disk->dst = virIndexToDiskName(ctrlr_idx * diskNameOffset + addr,
diskNamePrefix);
+disk->info.addr.drive.controller = ctrlr_idx;
+disk->info.addr.drive.bus = 0;
+disk->info.addr.drive.target = 0;
+disk->info.addr.drive.unit = addr;
+
+if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseFloppyStorageExtent(virDomainDefPtr def,
virDomainDiskDefPtr disk)
+{
+disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
+disk->dst = g_strdup("fda");
+
+if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
+return -1;
+
+return 0;
+}
+
+
+static int
+hypervDomainDefParseVirtualExtent(hypervPrivate *priv,
+ virDomainDefPtr def,
+ Msvm_StorageAllocationSettingData
*disk_entry,
+ Msvm_ResourceAllocationSettingData *rasd,
+ Msvm_ResourceAllocationSettingData
**ideControllers,
+ Msvm_ResourceAllocationSettingData
**scsiControllers)
+{
+Msvm_ResourceAllocationSettingData *diskParent = NULL;
+Msvm_ResourceAllocationSettingData *controller = NULL;
+virDomainDiskDefPtr disk = NULL;
+int result = -1;
+
+if
[PATCH 00/23] hyperv: storage volume XML changes
Here's a GitLab MR if you'd prefer to review it there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/12/commits Matt Coleman (23): hyperv: XML parsing of storage volumes schema: add support for Windows file paths and device names hyperv: ambiguous VM names will throw an error hyperv: implement domainUndefine and domainUndefineFlags hyperv: implement domainCreateXML and domainDefineXML hyperv: add hypervMsvmVSMSAddResourceSettings hyperv: create SCSI controllers when defining domains hyperv: attach virtual disks when defining domains hyperv: attach physical disks when defining domains hyperv: attach virtual optical disks when defining domains hyperv: attach floppy disks when defining domains hyperv: implement domainAttachDevice and domainAttachDeviceFlags hyperv: store the Hyper-V version when connecting hyperv: add inheritance to the WMI generator XML parsing of serial ports hyperv: add support for creating serial devices hyperv: XML parsing of Ethernet adapters hyperv: add support for creating network adapters hyperv: implement connectListAllNetworks and connectNumOfNetworks hyperv: implement networkLookupByName and networkLookupByUUID hyperv: implement connectNumOfDefinedNetworks and connectListDefinedNetworks hyperv: implement networkGetAutostart, networkIsActive, and networkIsPersistent hyperv: implement networkGetXMLDesc docs/schemas/basictypes.rng |2 +- docs/schemas/domaincommon.rng |5 +- include/libvirt/virterror.h |1 + po/POTFILES.in|1 + scripts/hyperv_wmi_generator.py | 12 +- src/hyperv/hyperv_driver.c| 1812 - src/hyperv/hyperv_driver.h|3 + src/hyperv/hyperv_network_driver.c| 250 +++ src/hyperv/hyperv_network_driver.h| 26 + src/hyperv/hyperv_private.h |3 + src/hyperv/hyperv_wmi.c | 156 ++ src/hyperv/hyperv_wmi.h | 25 + src/hyperv/hyperv_wmi_classes.h | 35 + src/hyperv/hyperv_wmi_generator.input | 280 +++ src/hyperv/meson.build|1 + src/util/virerror.c |6 +- .../disk-hyperv-physical.xml | 17 + .../disk-hyperv-virtual.xml | 17 + .../disk-hyperv-physical.xml | 23 + .../disk-hyperv-virtual.xml | 23 + tests/genericxml2xmltest.c|2 + 21 files changed, 2655 insertions(+), 45 deletions(-) create mode 100644 src/hyperv/hyperv_network_driver.c create mode 100644 src/hyperv/hyperv_network_driver.h create mode 100644 tests/genericxml2xmlindata/disk-hyperv-physical.xml create mode 100644 tests/genericxml2xmlindata/disk-hyperv-virtual.xml create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-physical.xml create mode 100644 tests/genericxml2xmloutdata/disk-hyperv-virtual.xml -- 2.27.0
Re: [PATCH] util: convert char pointers to use g_autofree
I spent a significant chunk of time trying to get `git send-email` working but struggled to get the tool to work on my computer. I instead used the `Email Patch` feature in GitLab to format the patch as an email. Since the formatting is wrong, I may have someone submit the patch on my behalf. On Wed, Nov 18, 2020 at 11:46 AM Michal Privoznik wrote: > On 11/17/20 9:45 PM, Barrett J Schonefeld wrote: > >>From 82f992c7ff4ef59682f42c863fca242dd84208f9 Mon Sep 17 00:00:00 2001 > > From: Barrett Schonefeld > > Date: Mon, 9 Nov 2020 16:07:25 -0600 > > Subject: [PATCH] util: convert char pointers to use g_autofree > > > > additional conversions to the GLib API in src/util per issue #11. > > > > Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11 > > > > Signed-off-by: Barrett Schonefeld > > --- > > src/util/vircgroupv1.c | 3 +- > > src/util/virdnsmasq.c| 43 - > > src/util/virfile.c | 9 ++--- > > src/util/virhostcpu.c| 4 +- > > src/util/virlockspace.c | 6 +-- > > src/util/virlog.c| 12 ++ > > src/util/virmacmap.c | 3 +- > > src/util/virnetdevbandwidth.c| 48 --- > > src/util/virresctrl.c| 25 > > src/util/virrotatingfile.c | 11 ++ > > src/util/virscsihost.c | 25 +--- > > src/util/virsecret.c | 14 +++ > > src/util/virstorageencryption.c | 19 +++-- > > src/util/virstoragefilebackend.c | 4 +- > > src/util/virsysinfo.c| 18 +++-- > > src/util/viruri.c| 7 +--- > > src/util/virutil.c | 66 +++- > > src/util/virvhba.c | 57 ++- > > src/util/virxml.c| 7 +--- > > 19 files changed, 130 insertions(+), 251 deletions(-) > > > I'm sorry, I can't apply this patch, it is corrupted. Looks like you've > wrapped lines. Does 'git send-email' not work for you? Because that is > the recommended way to send patches. > > https://libvirt.org/submitting-patches.html > > Michal > >
[libvirt PATCH 0/2] Add more documentation for migrations over UNIX sockets
Few words about SELinux that might not be very clear to some. Martin Kletzander (2): qemu: Disable NBD TLS migration over UNIX socket docs: Document SELinux caveats when migrating over UNIX sockets docs/manpages/virsh.rst | 9 - docs/migration.html.in| 9 + src/qemu/qemu_migration.c | 10 -- 3 files changed, 25 insertions(+), 3 deletions(-) -- 2.29.2
[libvirt PATCH 1/2] qemu: Disable NBD TLS migration over UNIX socket
Even though it is technically possible, when running the migrations QEMU's
nbd-server-start errors out with:
"TLS is only supported with IPv4/IPv6"
We can always enable it when QEMU adds this feature, but for now it is safer to
show our error message rather than rely on QEMU to error out properly.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_migration.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index fef0be63a1a7..dd44849b1a87 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1100,6 +1100,12 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver,
if (uri->port)
port = uri->port;
} else if (STREQ(uri->scheme, "unix")) {
+if (flags & VIR_MIGRATE_TLS) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("NBD migration with TLS is not supported over
UNIX socket"));
+return -1;
+}
+
if (!uri->path) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("UNIX disks URI does not include path"));
@@ -4330,12 +4336,12 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr
driver,
VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, dconnuri=%s, vm=%p, xmlin=%s, "
"dname=%s, uri=%s, graphicsuri=%s, listenAddress=%s, "
- "nmigrate_disks=%zu, migrate_disks=%p, nbdPort=%d, "
+ "nmigrate_disks=%zu, migrate_disks=%p, nbdPort=%d, nbdURI=%s, "
"bandwidth=%llu, useParams=%d, flags=0x%lx",
driver, sconn, dconn, NULLSTR(dconnuri), vm, NULLSTR(xmlin),
NULLSTR(dname), NULLSTR(uri), NULLSTR(graphicsuri),
NULLSTR(listenAddress), nmigrate_disks, migrate_disks, nbdPort,
- bandwidth, useParams, flags);
+ NULLSTR(nbdURI), bandwidth, useParams, flags);
/* Unlike the virDomainMigrateVersion3 counterpart, we don't need
* to worry about auto-setting the VIR_MIGRATE_CHANGE_PROTECTION
--
2.29.2
[libvirt PATCH 2/2] docs: Document SELinux caveats when migrating over UNIX sockets
The information about sockets having different label than the one on the file
and the way it needs to be set is very difficult to find for those who did not
come across it before. Let's describe what needs to happen in order for the
migration to go through rather than rely on general knowledge of others.
Signed-off-by: Martin Kletzander
---
docs/manpages/virsh.rst | 9 -
docs/migration.html.in | 9 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 1ae6d1a0d450..f0836b14defa 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -3340,7 +3340,14 @@ migrating disks. This can be *tcp://address:port* to
specify a listen address
UNIX socket with that specified path. In this case you need to make sure the
same socket path is accessible to both source and destination hypervisors and
connecting to the socket on the source (after hypervisor creates it on the
-destination) will actually connect to the destination.
+destination) will actually connect to the destination. If you are using
SELinux
+(at least on the source host) you need to make sure the socket on the source is
+accessible to libvirtd/QEMU for connection. That is because libvirt cannot
+change the context of the socket because it is different from the file
+representation of the socket and because the context is chosen by its creator
+(usually by using *setsockcreatecon{,_raw}()* functions). Generally
+*system_r:system_u:svirt_socket_t:s0* should do the trick, but check the
SELinux
+rules and settings of your system.
migrate-compcache
diff --git a/docs/migration.html.in b/docs/migration.html.in
index 77731eeb373e..79ceed62747f 100644
--- a/docs/migration.html.in
+++ b/docs/migration.html.in
@@ -658,6 +658,15 @@ virsh migrate --p2p --tunnelled web1
qemu+ssh://desthost/system qemu+ssh://10.0.
virsh migrate web1 [--p2p] --copy-storage-all
'qemu+unix:///system?socket=/tmp/migdir/test-sock-driver'
'unix:///tmp/migdir/test-sock-qemu' --disks-uri unix:///tmp/migdir/test-sock-nbd
+
+ One caveat is that on SELinux-enabled systems all the sockets that the
+ hypervisor is going to connect to needs to have the proper context and
+ that is chosen before its creation by the process that creates it. That
+ is usually done by using setsockcreatecon{,raw}() functions.
+ Generally *system_r:system_u:svirt_socket_t:s0* should do the trick, but
+ check the SELinux rules and settings of your system.
+
+
Supported by QEMU driver
--
2.29.2
[PATCH v2 6/6] qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE
qemuDomainAlignMemorySizes() has an operation order problem. We are
calculating 'initialmem' without aligning the memory modules first.
Since we're aligning the dimms afterwards this can create inconsistencies
in the end result. x86 has alignment of 1-2MiB and it's not severely
impacted by it, but pSeries works with 256MiB alignment and the difference
is noticeable.
This is the case of the existing 'memory-hotplug-ppc64-nonuma' test.
The test consists of a 2GiB (aligned value) guest with 2 ~520MiB dimms,
both unaligned. 'initialmem' is calculated by taking total_mem and
subtracting the dimms size (via virDomainDefGetMemoryInitial()), which
wil give us 2GiB - 520MiB - 520MiB, ending up with a little more than
an 1GiB of 'initialmem'. Note that this value is now unaligned, and
will be aligned up via VIR_ROUND_UP(), and we'll end up with 'initialmem'
of 1GiB + 256MiB. Given that the dimms are aligned later on, the end
result for QEMU is that the guest will have a 'mem' size of 1310720k,
plus the two 512 MiB dimms, exceeding in 256MiB the desired 2GiB
memory and currentMemory specified in the XML.
Existing guests can't be fixed without breaking ABI, but we have
code already in place to align pSeries NVDIMM modules for new guests.
Let's extend it to align all pSeries mem modules.
A new test, 'memory-hotplug-ppc64-nonuma-abi-update', a copy of the
existing 'memory-hotplug-ppc64-nonuma', was added to demonstrate the
result for new pSeries guests. For the same unaligned XML mentioned
above, after applying this patch:
- starting QEMU mem size without PARSE_ABI_UPDATE:
-m size=1310720k,slots=16,maxmem=4194304k \ (no changes)
- starting QEMU mem size with PARSE_ABI_UPDATE:
-m size=1048576k,slots=16,maxmem=4194304k \ (size fixed)
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c| 14 --
...emory-hotplug-ppc64-nonuma-abi-update.args | 34 ++
...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 +
tests/qemuxml2argvtest.c | 7 +++
...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++
tests/qemuxml2xmltest.c | 7 +++
6 files changed, 135 insertions(+), 4 deletions(-)
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml
create mode 100644
tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b9eb54a11c..a16ec9ac58 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5354,10 +5354,16 @@ qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
virArch arch,
* later on by qemuDomainAlignMemorySizes() to contemplate existing
* guests as well. */
if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
-if (ARCH_IS_PPC64(arch) &&
-mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-virDomainNVDimmAlignSizePseries(mem) < 0)
-return -1;
+if (ARCH_IS_PPC64(arch)) {
+unsigned long long ppc64MemModuleAlign = 256 * 1024;
+
+if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+if (virDomainNVDimmAlignSizePseries(mem) < 0)
+return -1;
+} else {
+mem->size = VIR_ROUND_UP(mem->size, ppc64MemModuleAlign);
+}
+}
}
return 0;
diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args
b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args
new file mode 100644
index 00..78406f7f04
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-name QEMUGuest1 \
+-S \
+-machine pseries,accel=kvm,usb=off,dump-guest-core=off \
+-m size=1048576k,slots=16,maxmem=4194304k \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-object memory-backend-ram,id=memdimm0,size=536870912 \
+-device pc-dimm,memdev=memdimm0,id=dimm0,slot=0 \
+-object memory-backend-ram,id=memdimm1,size=536870912 \
+-device pc-dimm,memdev=memdimm1,id=dimm1,slot=1 \
+-uuid 49545eb3-75e1-2d0a-acdd-f0294406c99e \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-kernel /media/ram/uImage \
+-initrd /media/ram/ramdisk \
+-append 'root=/dev/ram rw console=ttyS0,115200' \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2
diff --git
[PATCH v2 4/6] qemuxml2xmltest.c: honor ARG_PARSEFLAGS
At this moment, it is not possible to create a test specifying ARG_PARSEFLAGS because info->parseFlags is not being forwarded to testCompareDomXML2XMLFiles(). Let's fix it now so next patch can make use of it. Signed-off-by: Daniel Henrique Barboza --- tests/qemuxml2xmltest.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c006719dee..603ba71686 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -32,7 +32,8 @@ testXML2XMLActive(const void *opaque) const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, true, 0, + info->infile, info->outfile, true, + info->parseFlags, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } @@ -43,7 +44,8 @@ testXML2XMLInactive(const void *opaque) const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, false, 0, + info->infile, info->outfile, false, + info->parseFlags, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } -- 2.26.2
[PATCH v2 2/6] qemu: move memory size align to qemuProcessPrepareDomain()
qemuBuildCommandLine() is calling qemuDomainAlignMemorySizes(),
which is an operation that changes live XML and domain and has
little to do with the command line build process.
Move it to qemuProcessPrepareDomain() where we're supposed to
make live XML and domain changes before launch. qemuProcessStart()
is setting VIR_QEMU_PROCESS_START_NEW if !migrate && !snapshot,
same conditions used in qemuBuildCommandLine() to call
qemuDomainAlignMemorySizes(), making this change seamless.
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_command.c | 3 ---
src/qemu/qemu_process.c | 6 ++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 34b5746c1a..2bcdb28244 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9792,9 +9792,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
qemuBuildDomainLoaderCommandLine(cmd, def, qemuCaps);
-if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0)
-return NULL;
-
if (qemuBuildMemCommandLine(cmd, def, qemuCaps, priv) < 0)
return NULL;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3677da635c..39c3edf4b9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6493,6 +6493,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
if (qemuExtDevicesPrepareDomain(driver, vm) < 0)
return -1;
+if (flags & VIR_QEMU_PROCESS_START_NEW) {
+VIR_DEBUG("Aligning guest memory");
+if (qemuDomainAlignMemorySizes(vm->def) < 0)
+return -1;
+}
+
for (i = 0; i < vm->def->nchannels; i++) {
if (qemuDomainPrepareChannel(vm->def->channels[i],
priv->channelTargetDir) < 0)
--
2.26.2
[PATCH v2 5/6] qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE
A previous patch removed the pSeries NVDIMM align that wasn't
being done properly. This patch reintroduces it in the right
fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE.
This makes it complying with the intended design defined by
commit c7d7ba85a624.
Since the PARSE_ABI_FLAG is more restrictive than checking for
!migrate && !snapshot, like is being currently done with
qemuDomainAlignMemorySizes(), this means that we'll align the
pSeries NVDIMMs in two places - in post parse time for new
guests, and in qemuDomainAlignMemorySizes() for all guests
that aren't migrating or in a snapshot.
Another difference is that the logic is now in the QEMU driver
instead of domain_conf.c. This was necessary because all
considerations made about the PARSE_ABI_UPDATE flag were done
under QEMU. Given that no other driver supports ppc64 there is no
impact in this change.
A new test was added to exercise what we're doing. It consists
of a a copy of the existing 'memory-hotplug-nvdimm-ppc64' xml2xml
test, called with the PARSE_ABI_UPDATE flag. As intended, we're
not changing QEMU command line or any XML without the flag,
while the pseries NVDIMM memory is being aligned when the
flag is used.
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c| 33 +++-
...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++
...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++
tests/qemuxml2xmltest.c | 7 +++
4 files changed, 139 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
create mode 100644
tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2158080a56..b9eb54a11c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5337,6 +5337,33 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm,
}
+static int
+qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch,
+ unsigned int parseFlags)
+{
+/* Memory alignment can't be done for migration or snapshot
+ * scenarios. This logic was defined by commit c7d7ba85a624.
+ *
+ * There is no easy way to replicate at this point the same conditions
+ * used to call qemuDomainAlignMemorySizes(), which means checking if
+ * we're not migrating and not in a snapshot.
+ *
+ * We can use the PARSE_ABI_UPDATE flag, which is more strict -
+ * existing guests will not activate the flag to avoid breaking
+ * boot ABI. This means that any alignment done here will be replicated
+ * later on by qemuDomainAlignMemorySizes() to contemplate existing
+ * guests as well. */
+if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
+if (ARCH_IS_PPC64(arch) &&
+mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+virDomainNVDimmAlignSizePseries(mem) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
static int
qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
const virDomainDef *def,
@@ -5394,6 +5421,11 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
ret = qemuDomainTPMDefPostParse(dev->data.tpm, def->os.arch);
break;
+case VIR_DOMAIN_DEVICE_MEMORY:
+ret = qemuDomainMemoryDefPostParse(dev->data.memory, def->os.arch,
+ parseFlags);
+break;
+
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
@@ -5406,7 +5438,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_MEMBALLOON:
case VIR_DOMAIN_DEVICE_NVRAM:
case VIR_DOMAIN_DEVICE_RNG:
-case VIR_DOMAIN_DEVICE_MEMORY:
case VIR_DOMAIN_DEVICE_IOMMU:
case VIR_DOMAIN_DEVICE_AUDIO:
ret = 0;
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
new file mode 100644
index 00..ae5a17d3c8
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
@@ -0,0 +1,50 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 1099511627776
+ 1267710
+ 1267710
+ 2
+
+hvm
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-ppc64
+
+
+
+
+
+
+
+
+
+
+
+
+ 49545eb3-75e1-2d0a-acdd-f0294406c99e
+
+/tmp/nvdimm
+
+
+55
+0
+
+ 128
+
+
+
+
+
+
diff --git
a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
new file mode 100644
index 00..24b0982a7b
--- /dev/null
+++
[PATCH v2 1/6] qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW
qemuProcessCreatePretendCmdPrepare() is setting the VIR_QEMU_PROCESS_START_NEW regardless of whether this is a migration case or not. This behavior differs from what we're doing in qemuProcessStart(), where the flag is set only if !migrate && !snapshot. Fix it by making the flag setting consistent with what we're doing in qemuProcessStart(). Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_process.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 579b3c3713..3677da635c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7436,7 +7436,10 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_AUTODESTROY, -1); flags |= VIR_QEMU_PROCESS_START_PRETEND; -flags |= VIR_QEMU_PROCESS_START_NEW; + +if (!migrateURI) +flags |= VIR_QEMU_PROCESS_START_NEW; + if (standalone) flags |= VIR_QEMU_PROCESS_START_STANDALONE; -- 2.26.2
[PATCH v2 3/6] Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()"
The code to align ppc64 NVDIMMs on post parse was introduced in
commit d3f3c2c97f9b. That commit failed to realize that we
can't align memory unconditionally. As of commit c7d7ba85a624
("qemu: command: Align memory sizes only on fresh starts"),
all memory alignment should be executed only when we're not
migrating or in a snapshot.
This revert does not break any guests in the wild, given that
ppc64 NVDIMMs are still being aligned in qemuDomainAlignMemorySizes().
Next patch will introduce a mechanism where we can have post
parse NVDIMM alignment for pSeries without breaking the
intended design, as defined by c7d7ba85a624.
This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c| 23 +--
.../memory-hotplug-nvdimm-ppc64.xml | 2 +-
2 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 498a8b6ef0..961f292e1f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5351,24 +5351,6 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
}
-static int
-virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
-const virDomainDef *def)
-{
-/* Although only the QEMU driver implements PPC64 support, this
- * code is related to the platform specification (PAPR), i.e. it
- * is hypervisor agnostic, and any future PPC64 hypervisor driver
- * will have the same restriction.
- */
-if (ARCH_IS_PPC64(def->os.arch) &&
-mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-virDomainNVDimmAlignSizePseries(mem) < 0)
-return -1;
-
-return 0;
-}
-
-
static int
virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
const virDomainDef *def,
@@ -5414,10 +5396,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr
dev,
ret = 0;
break;
-case VIR_DOMAIN_DEVICE_MEMORY:
-ret = virDomainMemoryDefPostParse(dev->data.memory, def);
-break;
-
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
@@ -5432,6 +5410,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr
dev,
case VIR_DOMAIN_DEVICE_SHMEM:
case VIR_DOMAIN_DEVICE_TPM:
case VIR_DOMAIN_DEVICE_PANIC:
+case VIR_DOMAIN_DEVICE_MEMORY:
case VIR_DOMAIN_DEVICE_IOMMU:
case VIR_DOMAIN_DEVICE_AUDIO:
ret = 0;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
index ecb1b83b4a..ae5a17d3c8 100644
--- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
@@ -38,7 +38,7 @@
/tmp/nvdimm
-524416
+55
0
128
--
2.26.2
[PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c| 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c| 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml -- 2.26.2
Re: Libvirt NVME support
On 11/18/20 11:24 AM, Peter Krempa wrote: On Wed, Nov 18, 2020 at 09:57:14 +, Thanos Makatos wrote: As a separate question, is there any performance benefit of emulating a NVMe controller compared to e.g. virtio-scsi? We haven't measured that yet; I would expect it to be slight faster and/or more CPU efficient but wouldn't be surprised if it isn't. The main benefit of using NVMe is that we don't have to install VirtIO drivers in the guest. Okay, I'm not sold on the drivers bit but that is definitely not a problem in regards of adding support for emulating NVME controllers to libvirt. As a starting point a trivial way to model this in the XML will be: And then add the storage into it as: The 'drive' address here maps the disk to the controller. This example uses unit= as the way to specify the namespace ID. Both 'bus' and 'target' must be 0. You can theoretically also add your own address type if 'drive' doesn't look right. This model will have problems with hotplug/unplug if the NVMe spec doesn't actually allow hotplug of a single namespace into a controller as libvirt's hotplug APIs only deal with one element at time. We theoretically could work this around by allowing hotplug of disks which correspond to the namespace used while the controller was not attached yet, and the attach of the controller then attaches both the backends and the controller. This is a bit hacky though. Another obvious solution is to disallow hotplug of the namespaces and thus also the controller. Would it make sense to relax the current limitation in libvirt and allow (which is meant for cases where the backend is a NVMe disk) to be on something else than 'virtio' bus? Michal
Re: [PATCH] util: convert char pointers to use g_autofree
On 11/17/20 9:45 PM, Barrett J Schonefeld wrote: From 82f992c7ff4ef59682f42c863fca242dd84208f9 Mon Sep 17 00:00:00 2001 From: Barrett Schonefeld Date: Mon, 9 Nov 2020 16:07:25 -0600 Subject: [PATCH] util: convert char pointers to use g_autofree additional conversions to the GLib API in src/util per issue #11. Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11 Signed-off-by: Barrett Schonefeld --- src/util/vircgroupv1.c | 3 +- src/util/virdnsmasq.c| 43 - src/util/virfile.c | 9 ++--- src/util/virhostcpu.c| 4 +- src/util/virlockspace.c | 6 +-- src/util/virlog.c| 12 ++ src/util/virmacmap.c | 3 +- src/util/virnetdevbandwidth.c| 48 --- src/util/virresctrl.c| 25 src/util/virrotatingfile.c | 11 ++ src/util/virscsihost.c | 25 +--- src/util/virsecret.c | 14 +++ src/util/virstorageencryption.c | 19 +++-- src/util/virstoragefilebackend.c | 4 +- src/util/virsysinfo.c| 18 +++-- src/util/viruri.c| 7 +--- src/util/virutil.c | 66 +++- src/util/virvhba.c | 57 ++- src/util/virxml.c| 7 +--- 19 files changed, 130 insertions(+), 251 deletions(-) I'm sorry, I can't apply this patch, it is corrupted. Looks like you've wrapped lines. Does 'git send-email' not work for you? Because that is the recommended way to send patches. https://libvirt.org/submitting-patches.html Michal
[libvirt PATCH 10/10] tests: add minimal XML example for sparc VM
Signed-off-by: Daniel P. Berrangé
---
tests/qemuxml2argvdata/sparc-minimal.args | 34 +++
tests/qemuxml2argvdata/sparc-minimal.xml | 21 ++
tests/qemuxml2argvtest.c | 3 ++
3 files changed, 58 insertions(+)
create mode 100644 tests/qemuxml2argvdata/sparc-minimal.args
create mode 100644 tests/qemuxml2argvdata/sparc-minimal.xml
diff --git a/tests/qemuxml2argvdata/sparc-minimal.args
b/tests/qemuxml2argvdata/sparc-minimal.args
new file mode 100644
index 00..65cf99c895
--- /dev/null
+++ b/tests/qemuxml2argvdata/sparc-minimal.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-redhat62sparc \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-redhat62sparc/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-redhat62sparc/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-redhat62sparc/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-sparc \
+-name redhat62sparc \
+-S \
+-machine SS-5,accel=tcg,usb=off,dump-guest-core=off \
+-m 500 \
+-realtime mlock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 00010203-0405-4607-8809-0a0b0c0d0e0f \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,\
+path=/tmp/lib/domain--1-redhat62sparc/monitor.sock,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-usb \
+-drive file=/home/berrange/VirtualMachines/redhat-6.2-sparc.img,format=qcow2,\
+if=none,id=drive-scsi0-0-0-0 \
+-device scsi-hd,bus=scsi.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
+id=scsi0-0-0-0,bootindex=1 \
+-drive file=/home/berrange/VirtualMachines/redhat-6.2-sparc.iso,format=raw,\
+if=none,id=drive-scsi0-0-0-1,readonly=on \
+-device scsi-cd,bus=scsi.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,\
+id=scsi0-0-0-1
diff --git a/tests/qemuxml2argvdata/sparc-minimal.xml
b/tests/qemuxml2argvdata/sparc-minimal.xml
new file mode 100644
index 00..f69942020b
--- /dev/null
+++ b/tests/qemuxml2argvdata/sparc-minimal.xml
@@ -0,0 +1,21 @@
+
+ redhat62sparc
+ 00010203-0405-4607-8809-0a0b0c0d0e0f
+ 500
+ 1
+
+hvm
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 174294c0f1..42d147243e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3161,6 +3161,9 @@ mymain(void)
QEMU_CAPS_OBJECT_GPEX,
QEMU_CAPS_NEC_USB_XHCI);
+DO_TEST("sparc-minimal",
+QEMU_CAPS_SCSI_NCR53C90);
+
/* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag
* will avoid the error during parse. This will cause us to fill in
* the missing machine type using the i386 binary, despite it being
--
2.28.0
[libvirt PATCH 09/10] tests: define QEMU driver capabilities for sparc architecture
Signed-off-by: Daniel P. Berrangé
---
tests/testutilsqemu.c | 62 ++-
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 23a9777fd8..906fdf5c1a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -31,7 +31,8 @@ static const char *qemu_emulators[VIR_ARCH_LAST] = {
[VIR_ARCH_PPC] = "/usr/bin/qemu-system-ppc",
[VIR_ARCH_RISCV32] = "/usr/bin/qemu-system-riscv32",
[VIR_ARCH_RISCV64] = "/usr/bin/qemu-system-riscv64",
-[VIR_ARCH_S390X] = "/usr/bin/qemu-system-s390x"
+[VIR_ARCH_S390X] = "/usr/bin/qemu-system-s390x",
+[VIR_ARCH_SPARC] = "/usr/bin/qemu-system-sparc",
};
static const virArch arch_alias[VIR_ARCH_LAST] = {
@@ -74,6 +75,11 @@ static const char *const riscv64_machines[] = {
static const char *const s390x_machines[] = {
"s390-virtio", "s390-ccw-virtio", "s390-ccw", NULL
};
+static const char *const sparc_machines[] = {
+"SS-5", "LX", "SPARCClassic", "SPARCbook",
+"SS-10", "SS-20", "SS-4", "SS-600MP",
+"Voyager", "leon3_generic", NULL
+};
static const char *const *qemu_machines[VIR_ARCH_LAST] = {
[VIR_ARCH_I686] = i386_machines,
@@ -85,6 +91,7 @@ static const char *const *qemu_machines[VIR_ARCH_LAST] = {
[VIR_ARCH_RISCV32] = riscv32_machines,
[VIR_ARCH_RISCV64] = riscv64_machines,
[VIR_ARCH_S390X] = s390x_machines,
+[VIR_ARCH_SPARC] = sparc_machines,
};
static const char *const *kvm_machines[VIR_ARCH_LAST] = {
@@ -106,7 +113,8 @@ static const char *qemu_default_ram_id[VIR_ARCH_LAST] = {
[VIR_ARCH_ARMV7L] = "vexpress.highmem",
[VIR_ARCH_PPC64] = "ppc_spapr.ram",
[VIR_ARCH_PPC] = "ppc_spapr.ram",
-[VIR_ARCH_S390X] = "s390.ram"
+[VIR_ARCH_S390X] = "s390.ram",
+[VIR_ARCH_SPARC] = "sun4m.ram",
};
char *
@@ -181,19 +189,21 @@ testQemuAddGuest(virCapsPtr caps,
NULL))
goto error;
-nmachines = g_strv_length((char **)kvm_machines[emu_arch]);
-machines = virCapabilitiesAllocMachines(kvm_machines[emu_arch],
-nmachines);
-if (machines == NULL)
-goto error;
+if (kvm_machines[emu_arch] != NULL) {
+nmachines = g_strv_length((char **)kvm_machines[emu_arch]);
+machines = virCapabilitiesAllocMachines(kvm_machines[emu_arch],
+nmachines);
+if (machines == NULL)
+goto error;
-if (!virCapabilitiesAddGuestDomain(guest,
- VIR_DOMAIN_VIRT_KVM,
- qemu_emulators[emu_arch],
- NULL,
- nmachines,
- machines))
-goto error;
+if (!virCapabilitiesAddGuestDomain(guest,
+ VIR_DOMAIN_VIRT_KVM,
+ qemu_emulators[emu_arch],
+ NULL,
+ nmachines,
+ machines))
+goto error;
+}
return 0;
@@ -363,18 +373,20 @@ int qemuTestCapsCacheInsert(virFileCachePtr cache,
defaultRAMid);
virQEMUCapsSet(tmpCaps, QEMU_CAPS_TCG);
}
-for (j = 0; kvm_machines[i][j] != NULL; j++) {
-virQEMUCapsAddMachine(tmpCaps,
- VIR_DOMAIN_VIRT_KVM,
- kvm_machines[i][j],
- NULL,
- NULL,
- 0,
- false,
+if (kvm_machines[i] != NULL) {
+for (j = 0; kvm_machines[i][j] != NULL; j++) {
+virQEMUCapsAddMachine(tmpCaps,
+ VIR_DOMAIN_VIRT_KVM,
+ kvm_machines[i][j],
+ NULL,
+ NULL,
+ 0,
+ false,
false,
- true,
- defaultRAMid);
-virQEMUCapsSet(tmpCaps, QEMU_CAPS_KVM);
+ true,
+ defaultRAMid);
+virQEMUCapsSet(tmpCaps, QEMU_CAPS_KVM);
+}
}
}
--
2.28.0
[libvirt PATCH 07/10] tests: add fake host CPU for sparc architecture
Signed-off-by: Daniel P. Berrangé
---
tests/testutilshostcpus.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/tests/testutilshostcpus.h b/tests/testutilshostcpus.h
index a54b181c09..91d9c153d9 100644
--- a/tests/testutilshostcpus.h
+++ b/tests/testutilshostcpus.h
@@ -130,6 +130,14 @@ static virCPUDef cpuS390Data = {
.threads = 1,
};
+static virCPUDef cpuSparcData = {
+.type = VIR_CPU_TYPE_HOST,
+.arch = VIR_ARCH_SPARC,
+.sockets = 1,
+.cores = 1,
+.threads = 1,
+};
+
static inline virCPUDefPtr
testUtilsHostCpusGetDefForModel(const char *model)
{
@@ -161,6 +169,8 @@ testUtilsHostCpusGetDefForArch(virArch arch)
return virCPUDefCopy();
else if (arch == VIR_ARCH_AARCH64)
return virCPUDefCopy();
+else if (arch == VIR_ARCH_SPARC)
+return virCPUDefCopy();
return NULL;
}
--
2.28.0
[libvirt PATCH 06/10] qemu: enable support for ESP SCSI controller family
The NCR53C90 is the built-in SCSI controller on all sparc machine types,
but not sparc64. Note that it has the fixed alias "scsi", which differs
from our normal naming convention of "scsi0".
The DC390 and AM53C974 are PCI SCSI controllers that can be added to any
PCI machine.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_alias.c | 6 ++
src/qemu/qemu_command.c| 18 +---
src/qemu/qemu_domain_address.c | 7 +++
src/qemu/qemu_validate.c | 38 ++
4 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 101ab5608f..dcb6c7156d 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -176,6 +176,12 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
controller->info.alias = g_strdup("usb");
return 0;
}
+} else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90 &&
+controller->idx == 0) {
+controller->info.alias = g_strdup("scsi");
+return 0;
+}
}
/* all other controllers use the default ${type}${index} naming
* scheme for alias/id.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d3de13c6ee..fbaacb8dd8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2471,11 +2471,15 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
virBufferAddLit(, "pvscsi");
break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974:
+virBufferAddLit(, "am53c974");
+break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390:
+virBufferAddLit(, "dc-390");
+break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90: /* It is built-in dev
*/
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported controller model: %s"),
virDomainControllerModelSCSITypeToString(def->model));
@@ -2713,6 +2717,14 @@ qemuBuildSkipController(const virDomainControllerDef
*controller,
controller->idx == 0 && qemuDomainHasBuiltinIDE(def))
return true;
+/* first ESP SCSI controller is implicit on certain machine types */
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+controller->idx == 0 &&
+controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90 &&
+qemuDomainHasBuiltinESP(def)) {
+return true;
+}
+
return false;
}
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f1fb532f39..2788dc7fb3 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -64,6 +64,8 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def,
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI))
return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+else if (qemuDomainHasBuiltinESP(def))
+return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90;
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to determine model for SCSI controller idx=%d"),
@@ -2249,6 +2251,11 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
cont->idx == 0)
continue;
+/* NCR53C90 SCSI controller is always a built-in device */
+if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+cont->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90)
+continue;
+
if (!virDeviceInfoPCIAddressIsWanted(>info))
continue;
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 2cd9ee8230..96636d1cff 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2998,13 +2998,31 @@ qemuValidateCheckSCSIControllerModel(virQEMUCapsPtr
qemuCaps,
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390:
-case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported controller model: %s"),
virDomainControllerModelSCSITypeToString(model));
return false;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_NCR53C90)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+
[libvirt PATCH 05/10] qemu: add capabilities for the three ESP family SCSI controllers
Probing for the NCR53C90 controller is a little unusual. The
qom-list-types QMP command returns a list of all types known to
the QEMU binary. It does not distinguish devices which are user
creatable from those which are built-in.
Any QEMU target that supports PCI will have the DC390 / AM53C974
devices because they are PCI based. Due to code dependencies
in QEMU though, existance of these two devices will also pull in
the NCR53C90 device (called just 'esp' in QEMU). The NCR53C90 is
not user-creatable and can only be used when built-in to the
machine type.
This is only the case on sparc machines, and certain mips64 and
m68k machines. IOW, we don't rely on qom-list-types as a guide
for existance of NCR53C90, as it shouldn't really exist in most
QEMU binaries.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_capabilities.c | 27 +++
src/qemu/qemu_capabilities.h | 5
.../caps_1.5.3.x86_64.xml | 2 ++
.../caps_1.6.0.x86_64.xml | 2 ++
.../caps_1.7.0.x86_64.xml | 2 ++
.../caps_2.1.1.x86_64.xml | 2 ++
.../caps_2.10.0.aarch64.xml | 2 ++
.../caps_2.10.0.ppc64.xml | 2 ++
.../caps_2.10.0.x86_64.xml| 2 ++
.../caps_2.11.0.x86_64.xml| 2 ++
.../caps_2.12.0.aarch64.xml | 2 ++
.../caps_2.12.0.ppc64.xml | 2 ++
.../caps_2.12.0.x86_64.xml| 2 ++
.../caps_2.4.0.x86_64.xml | 2 ++
.../caps_2.5.0.x86_64.xml | 2 ++
.../caps_2.6.0.aarch64.xml| 2 ++
.../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 2 ++
.../caps_2.6.0.x86_64.xml | 2 ++
.../caps_2.7.0.x86_64.xml | 2 ++
.../caps_2.8.0.x86_64.xml | 2 ++
.../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 2 ++
.../caps_2.9.0.x86_64.xml | 2 ++
.../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 ++
.../caps_3.0.0.x86_64.xml | 2 ++
.../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 ++
.../caps_3.1.0.x86_64.xml | 2 ++
.../caps_4.0.0.aarch64.xml| 2 ++
.../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 ++
.../caps_4.0.0.riscv32.xml| 2 ++
.../caps_4.0.0.riscv64.xml| 2 ++
.../caps_4.0.0.x86_64.xml | 2 ++
.../caps_4.1.0.x86_64.xml | 2 ++
.../caps_4.2.0.aarch64.xml| 2 ++
.../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 ++
.../caps_4.2.0.x86_64.xml | 2 ++
.../caps_5.0.0.aarch64.xml| 2 ++
.../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 ++
.../caps_5.0.0.riscv64.xml| 2 ++
.../caps_5.0.0.x86_64.xml | 2 ++
.../caps_5.1.0.x86_64.xml | 2 ++
.../caps_5.2.0.x86_64.xml | 2 ++
41 files changed, 110 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9f9f976754..c9e5a17919 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -604,6 +604,11 @@ VIR_ENUM_IMPL(virQEMUCaps,
"block-export-add",
"netdev.vhost-vdpa",
"fsdev.createmode",
+
+ /* 385 */
+ "ncr53c90",
+ "dc390",
+ "am53c974",
);
@@ -1306,6 +1311,20 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] =
{
{ "tcg-accel", QEMU_CAPS_TCG },
{ "pvscsi", QEMU_CAPS_SCSI_PVSCSI },
{ "spapr-tpm-proxy", QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY },
+/*
+ * We don't probe 'esp' directly, because it is often reported
+ * as present for all QEMU binaries, due to it being enabled
+ * for built as a dependancy of dc390/am53c974 PCI SCSI
+ * controllers.
+ *
+ * The base 'esp' device is only used as a biult-in device
+ * and is not user-creatable. So we turn this cap on later
+ * based on arch.
+ *
+ * { "esp", QEMU_CAPS_SCSI_NCR53C90 },
+ */
+{ "dc390", QEMU_CAPS_SCSI_DC390 },
+{ "am53c974", QEMU_CAPS_SCSI_AM53C974 },
};
@@ -5121,6 +5140,14 @@ virQEMUCapsInitProcessCaps(virQEMUCapsPtr qemuCaps)
virQEMUCapsGet(qemuCaps, QEMU_CAPS_SAVEVM_MONITOR_NODES))
virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKDEV);
+/* We can't probe "esp" as a type via virQEMUCapsObjectTypes
+ * array as it is only usable when builtin to the machine type
+ */
+if (qemuCaps->arch == VIR_ARCH_SPARC ||
+qemuCaps->arch == VIR_ARCH_M68K ||
+qemuCaps->arch == VIR_ARCH_MIPS)
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_SCSI_NCR53C90);
+
virQEMUCapsInitProcessCapsInterlock(qemuCaps);
}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
[libvirt PATCH 04/10] conf: add support for ESP SCSI controller family
The NCR53C90 is the built-in SCSI controller on all sparc machine types,
and some mips and m68k machine types.
The DC390 and AM53C974 are PCI SCSI controllers that can be added to any
PCI machine.
These are only interesting for emulating obsolete hardware platforms.
Signed-off-by: Daniel P. Berrangé
---
docs/formatdomain.rst | 3 ++-
docs/schemas/domaincommon.rng | 3 +++
src/conf/domain_conf.c | 8
src/conf/domain_conf.h | 3 +++
src/qemu/qemu_command.c| 3 +++
src/qemu/qemu_domain_address.c | 3 +++
src/qemu/qemu_validate.c | 6 ++
src/vbox/vbox_common.c | 3 +++
src/vmx/vmx.c | 3 +++
9 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index c2c23371b1..ff64996af2 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3426,7 +3426,8 @@ specific features, such as:
``scsi``
A ``scsi`` controller has an optional attribute ``model``, which is one of
'auto', 'buslogic', 'ibmvscsi', 'lsilogic', 'lsisas1068', 'lsisas1078',
- 'virtio-scsi', 'vmpvscsi', 'virtio-transitional', 'virtio-non-transitional'.
+ 'virtio-scsi', 'vmpvscsi', 'virtio-transitional', 'virtio-non-transitional',
+ 'ncr53c90' (as builtin implicit controller only), 'am53c974', 'dc390'.
See `Virtio transitional devices <#elementsVirtioTransitional>`__ for more
details.
``usb``
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f86a854863..1ccc9759ea 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2381,6 +2381,9 @@
lsisas1078
virtio-transitional
virtio-non-transitional
+ ncr53c90
+ dc390
+ am53c974
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 498a8b6ef0..abfde5b161 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -445,6 +445,9 @@ VIR_ENUM_IMPL(virDomainControllerModelSCSI,
"lsisas1078",
"virtio-transitional",
"virtio-non-transitional",
+ "ncr53c90",
+ "dc390",
+ "am53c974",
);
VIR_ENUM_IMPL(virDomainControllerModelISA,
VIR_DOMAIN_CONTROLLER_MODEL_ISA_LAST,
@@ -4971,6 +4974,11 @@ virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
reserved = 7;
break;
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974:
+max = 6;
+break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3e3d4bd002..96e6c34553 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -649,6 +649,9 @@ typedef enum {
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078,
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL,
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL,
+VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90,
+VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390,
+VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974,
VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST
} virDomainControllerModelSCSI;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 34b5746c1a..d3de13c6ee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2473,6 +2473,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
break;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DC390:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AM53C974:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported controller model: %s"),
virDomainControllerModelSCSITypeToString(def->model));
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index dd87915a97..f1fb532f39 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -637,6 +637,7 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
switch ((virDomainControllerModelSCSI) cont->model) {
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90:
return 0;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
@@ -652,6 +653,8 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case
[libvirt PATCH 03/10] qemu: add helper method for checking if ESP SCSI is builtin
The NCR53C90 ESP SCSI controller is only usable when built-in to the
machine type. This method will facilitate checking that restriction
across many places.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_domain.c | 17 +
src/qemu/qemu_domain.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e85ca80929..1a80aa4c69 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8289,6 +8289,23 @@ qemuDomainMachineHasBuiltinIDE(const char *machine,
}
+bool qemuDomainHasBuiltinESP(const virDomainDef *def)
+{
+/* These machines use ncr53c90 (ESP) SCSI controller built-in */
+if (def->os.arch == VIR_ARCH_SPARC) {
+return true;
+} else if (ARCH_IS_MIPS64(def->os.arch) &&
+ (STREQ(def->os.machine, "magnum") ||
+STREQ(def->os.machine, "pica61"))) {
+return true;
+} else if (def->os.arch == VIR_ARCH_M68K &&
+ STREQ(def->os.machine, "q800")) {
+return true;
+}
+return false;
+}
+
+
static bool
qemuDomainMachineNeedsFDC(const char *machine,
const virArch arch)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 8dbec6e33f..6b75b828c0 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -780,6 +780,7 @@ bool qemuDomainIsPSeries(const virDomainDef *def);
bool qemuDomainHasPCIRoot(const virDomainDef *def);
bool qemuDomainHasPCIeRoot(const virDomainDef *def);
bool qemuDomainHasBuiltinIDE(const virDomainDef *def);
+bool qemuDomainHasBuiltinESP(const virDomainDef *def);
bool qemuDomainNeedsFDC(const virDomainDef *def);
bool qemuDomainSupportsPCI(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps);
--
2.28.0
[libvirt PATCH 00/10] add support for ESP SCSI controller family
The sparc machines types all have an NCR53C90 SCSI Controller builtin. This is from the ESP family of SCSI controllers which includs two PCI variants. Wire up support for all these SCSI controllers, so we can at least launch a sparc VM with disks attached. There are likely other gaps in CLI coverage for sparc, so I don't claim this lets you have a fully working VM yet. Daniel P. Berrangé (10): util: add ARCH_IS_MIPS64 helper macro qemu: fix default devices on sparc machines qemu: add helper method for checking if ESP SCSI is builtin conf: add support for ESP SCSI controller family qemu: add capabilities for the three ESP family SCSI controllers qemu: enable support for ESP SCSI controller family tests: add fake host CPU for sparc architecture tests: add capabilities data files for sparc emulator target tests: define QEMU driver capabilities for sparc architecture tests: add minimal XML example for sparc VM docs/formatdomain.rst | 3 +- docs/schemas/domaincommon.rng | 3 + src/conf/domain_conf.c| 8 + src/conf/domain_conf.h| 3 + src/qemu/qemu_alias.c | 6 + src/qemu/qemu_capabilities.c |27 + src/qemu/qemu_capabilities.h | 5 + src/qemu/qemu_command.c |15 + src/qemu/qemu_domain.c|21 + src/qemu/qemu_domain.h| 1 + src/qemu/qemu_domain_address.c|10 + src/qemu/qemu_validate.c |36 + src/util/virarch.h| 3 + src/vbox/vbox_common.c| 3 + src/vmx/vmx.c | 3 + tests/domaincapsdata/qemu_5.1.0.sparc.xml | 101 + .../caps_1.5.3.x86_64.xml | 2 + .../caps_1.6.0.x86_64.xml | 2 + .../caps_1.7.0.x86_64.xml | 2 + .../caps_2.1.1.x86_64.xml | 2 + .../caps_2.10.0.aarch64.xml | 2 + .../caps_2.10.0.ppc64.xml | 2 + .../caps_2.10.0.x86_64.xml| 2 + .../caps_2.11.0.x86_64.xml| 2 + .../caps_2.12.0.aarch64.xml | 2 + .../caps_2.12.0.ppc64.xml | 2 + .../caps_2.12.0.x86_64.xml| 2 + .../caps_2.4.0.x86_64.xml | 2 + .../caps_2.5.0.x86_64.xml | 2 + .../caps_2.6.0.aarch64.xml| 2 + .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml | 2 + .../caps_2.6.0.x86_64.xml | 2 + .../caps_2.7.0.x86_64.xml | 2 + .../caps_2.8.0.x86_64.xml | 2 + .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml | 2 + .../caps_2.9.0.x86_64.xml | 2 + .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 + .../caps_3.0.0.x86_64.xml | 2 + .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 + .../caps_3.1.0.x86_64.xml | 2 + .../caps_4.0.0.aarch64.xml| 2 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 + .../caps_4.0.0.riscv32.xml| 2 + .../caps_4.0.0.riscv64.xml| 2 + .../caps_4.0.0.x86_64.xml | 2 + .../caps_4.1.0.x86_64.xml | 2 + .../caps_4.2.0.aarch64.xml| 2 + .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 + .../caps_4.2.0.x86_64.xml | 2 + .../caps_5.0.0.aarch64.xml| 2 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 + .../caps_5.0.0.riscv64.xml| 2 + .../caps_5.0.0.x86_64.xml | 2 + .../caps_5.1.0.sparc.replies | 17294 .../qemucapabilitiesdata/caps_5.1.0.sparc.xml | 133 + .../caps_5.1.0.x86_64.xml | 2 + .../caps_5.2.0.x86_64.xml | 2 + tests/qemucaps2xmloutdata/caps.sparc.xml |25 + tests/qemuxml2argvdata/sparc-minimal.args |34 + tests/qemuxml2argvdata/sparc-minimal.xml |21 + tests/qemuxml2argvtest.c | 3 + tests/testutilshostcpus.h |10 + tests/testutilsqemu.c |62 +- 63 files changed, 17882 insertions(+), 26 deletions(-) create mode 100644 tests/domaincapsdata/qemu_5.1.0.sparc.xml create mode 100644 tests/qemucapabilitiesdata/caps_5.1.0.sparc.replies create mode 100644 tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml create mode 100644 tests/qemucaps2xmloutdata/caps.sparc.xml create mode 100644 tests/qemuxml2argvdata/sparc-minimal.args create mode 100644 tests/qemuxml2argvdata/sparc-minimal.xml --
[libvirt PATCH 02/10] qemu: fix default devices on sparc machines
The sparc machines have little in common with sparc64 machines. No sparc machine type includes a PCI bus, so we should not be adding one to the XML. This further means that we should not be adding a memory balloon device, nor USB controller as these are both PCI based. Signed-off-by: Daniel P. Berrangé --- src/qemu/qemu_domain.c | 4 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2158080a56..e85ca80929 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3561,6 +3561,10 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, break; case VIR_ARCH_SPARC: +addDefaultUSB = false; +addDefaultMemballoon = false; +break; + case VIR_ARCH_SPARC64: addPCIRoot = true; break; -- 2.28.0
[libvirt PATCH 01/10] util: add ARCH_IS_MIPS64 helper macro
In most cases logic for MIPS64 and MIPS64EL will be identical.
Signed-off-by: Daniel P. Berrangé
---
src/util/virarch.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/util/virarch.h b/src/util/virarch.h
index 46b4609dd5..528f84f8a5 100644
--- a/src/util/virarch.h
+++ b/src/util/virarch.h
@@ -95,6 +95,9 @@ typedef enum {
#define ARCH_IS_S390(arch) ((arch) == VIR_ARCH_S390 ||\
(arch) == VIR_ARCH_S390X)
+#define ARCH_IS_MIPS64(arch) ((arch) == VIR_ARCH_MIPS64 ||\
+ (arch) == VIR_ARCH_MIPS64EL)
+
typedef enum {
VIR_ARCH_LITTLE_ENDIAN,
VIR_ARCH_BIG_ENDIAN,
--
2.28.0
Re: [libvirt PATCH] virt-host-validate: fix detection with cgroups v2
On 11/18/20 1:48 PM, Pavel Hrdina wrote: Using virtCgroupNewSelf() is not correct with cgroups v2 because the the virt-host-validate process is executed from from the same cgroup context as the terminal and usually not all controllers are enabled by default. To do a proper check we need to use the root cgroup to see what controllers are actually available. Libvirt or systemd ensures that all controllers are available for VMs as well. This still doesn't solve the devices controller with cgroups v2 where there is no controller as it was replaced by eBPF. Currently libvirt tries to query eBPF programs which usually works only for root as regular users will get permission denied for that operation. Fixes: https://gitlab.com/libvirt/libvirt/-/issues/94 Signed-off-by: Pavel Hrdina --- src/libvirt_private.syms | 1 + src/util/vircgroup.h | 4 src/util/vircgrouppriv.h | 4 tools/virt-host-validate-common.c | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask
On 11/18/20 2:52 PM, Erik Skultety wrote:
On Wed, Nov 18, 2020 at 01:06:07PM +0100, Michal Privoznik wrote:
The way our domain capabilities work currently, is that we have
virDomainCapsEnum struct which contains 'unsigned int values'
member which serves as a bitmask. More complicated structs are
composed from this struct, giving us whole virDomainCaps
eventually.
Whenever we want to report that a certain value is supported, the
'1 << value' bit is set in the corresponding unsigned int member.
This works as long as the resulting value after bitshift does not
overflow unsigned int. There is a check inside
virDomainCapsEnumSet() which ensures exactly this, but no caller
really checks whether virDomainCapsEnumSet() succeeded. Also,
checking at runtime is a bit too late.
Fortunately, we know the largest value we want to store in each
member, because each enum of ours ends with _LAST member.
Therefore, we can check at build time whether an overflow can
occur.
I'm wondering how does this build failure knowledge actually help us? Are we
going to start re-evaluating our approach to enums, splitting them somehow? I
don't think so. The standard only mandates the enum to be large enough so that
the constants fit into an int, but it's been a while since then and 32bit is
simply not going to cut it. The almighty internet also claims that compilers
(GCC specifically) automatically re-size the enum given the declaration, so if
you do 1 << 32, I would expect that the compiler should allocate a 64bit data
type, once we're past 64, well, no static assert is going to help us anyway, so
we need to start thinking about this more intensively.
I think you might have misunderstood. This is not about our enums, but
the way we store individual values in domCaps. If you have an enum, say
with video models, then in to express supported models in domCaps is to
set (1 << val) bit for each val that we find supported (in qemuCaps).
For instance:
1 << VIR_DOMAIN_VIDEO_TYPE_NONE
1 << VIR_DOMAIN_VIDEO_TYPE_VGA /* if qemuCaps have QEMU_CAPS_DEVICE_VGA */
1 << VIR_DOMAIN_VIDEO_TYPE_CIRRUS /* QEMU_CAPS_DEVICE_CIRRUS_VGA */
and so on. This bitmask is saved into 'unsigned int' currently. And what
this patch tries to ensure is that 'unsigned int' is enough for all
possible models known to libvirt currently. If it doesn't fit then we
will need to switch to a bigger type or use virBitmap. The reason why it
is not virBitmap currently is that virDomainCaps is complicated
structure and freeing all bitmaps through each member (which on itself
is another structure) is just too much code. Especially if 'unsigned
int' serves us good for now.
Additionally, since we're using compiler extension quite a bit already, I say
we make use of those if type expansion is not automatic (I think you have to
use it explicitly if you want to force a smaller type, like a short int).
In case I'm misunderstanding something, then I still think that the macro
definition should go to something like internal.h and be named in a consistent
fashion like VIR_ENUM_STATIC_ASSERT.
Moreover, GLib claims the macro can be used anywhere where the typedef is in
scope, thus I believe the right place to put these asserts is right after the
actual typedef rather than before a specific struct - doing this also avoid
duplication if several structures make us of e.g. virTristateBool.
I thought about this but figured it would harm tags generation, wouldn't
it? For instance consider following:
struct _virDomainCapsDeviceVideo {
virTristateBool supported;
VIR_DECLARE_MEMBER(modelType, VIR_DOMAIN_VIDEO_TYPE_LAST);
};
where VIR_DECLARE_MEMBER() would assert() that 1 <<
VIR_DOMAIN_VIDEO_TYPE_LAST fits into 'unsigned int' (or whatever type we
will use), and also declare 'modelType' member for the struct. I'm not
sure a tags generator would be capable of seeing that.
Michal
[PATCH libvirt v1] tests: add capabilities for QEMU 5.1.0 on s390x
Signed-off-by: Shalini Chellathurai Saroja --- The replies file is removed from this patch and is available in https://gitlab.com/shalinichellathurai/libvirt/-/commit/1c34c07c434560d7f44212ce0bbbc8bf92490622 tests/domaincapsdata/qemu_5.1.0.s390x.xml | 230 + .../caps_5.1.0.s390x.replies | 24617 .../qemucapabilitiesdata/caps_5.1.0.s390x.xml | 3278 ++ ...default-video-type-s390x.s390x-latest.args | 2 +- .../disk-error-policy-s390x.s390x-latest.args |12 +- .../fs9p-ccw.s390x-latest.args| 4 +- ...othreads-virtio-scsi-ccw.s390x-latest.args | 2 +- ...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 2 +- .../s390x-ccw-graphics.s390x-latest.args | 4 +- .../s390x-ccw-headless.s390x-latest.args | 4 +- .../vhost-vsock-ccw-auto.s390x-latest.args| 4 +- .../vhost-vsock-ccw.s390x-latest.args | 4 +- 12 files changed, 28144 insertions(+), 19 deletions(-) create mode 100644 tests/domaincapsdata/qemu_5.1.0.s390x.xml create mode 100644 tests/qemucapabilitiesdata/caps_5.1.0.s390x.replies create mode 100644 tests/qemucapabilitiesdata/caps_5.1.0.s390x.xml diff --git a/tests/domaincapsdata/qemu_5.1.0.s390x.xml b/tests/domaincapsdata/qemu_5.1.0.s390x.xml new file mode 100644 index ..42780349 --- /dev/null +++ b/tests/domaincapsdata/qemu_5.1.0.s390x.xml @@ -0,0 +1,230 @@ + + /usr/bin/qemu-system-s390x + kvm + s390-ccw-virtio-5.1 + s390x + + + + + + /usr/share/AAVMF/AAVMF_CODE.fd + /usr/share/AAVMF/AAVMF32_CODE.fd + /usr/share/OVMF/OVMF_CODE.fd + +rom +pflash + + +yes +no + + +no + + + + + + +off + + + + gen15a-base + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + z800-base + z890.2-base + z9EC.2 + z13.2 + z9BC-base + z990.5-base + z890.2 + z890 + z9BC + z13 + z196 + z13s + z990.3 + z13s-base + z9EC + gen15a + z14ZR1-base + z14.2-base + z900.3-base + z13.2-base + z196.2-base + zBC12-base + z9BC.2-base + z900.2-base + z9EC.3 + zEC12 + z900 + z114-base + zEC12-base + z10EC.2 + z10EC-base + z900.3 + z14ZR1 + z10BC + z10BC.2-base + z9BC.2 + z990.2 + z990 + z14 + gen15b-base + z990.4 + max + z10EC.2-base + gen15a-base + z800 + z10EC + zEC12.2 + z990.2-base + z900-base + z10BC.2 + z9EC-base + z9EC.3-base + z114 + z890.3 + z196-base + z9EC.2-base + z196.2 + z14.2 + z990-base + z900.2 + z890-base + z10EC.3 + z14-base + z990.4-base + z10EC.3-base + z10BC-base + z13-base + z990.3-base + zEC12.2-base + zBC12 + z890.3-base + z990.5 + gen15b + qemu + + + + + +disk +cdrom +floppy +lun + + +fdc +scsi +virtio + + +virtio +virtio-transitional +virtio-non-transitional + + + + +sdl +vnc +egl-headless + + + + +virtio +none + + + + +subsystem + + +default +mandatory +requisite +optional + + +pci +scsi + + + +default +vfio + + + + +virtio +virtio-transitional +virtio-non-transitional + + +random +egd +builtin + + + + + + + + + + + + diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.s390x.replies b/tests/qemucapabilitiesdata/caps_5.1.0.s390x.replies new file mode 100644 index ..ec3fd1f6 --- /dev/null +++ b/tests/qemucapabilitiesdata/caps_5.1.0.s390x.replies @@ -0,0 +1,24617 @@ [...] diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.s390x.xml b/tests/qemucapabilitiesdata/caps_5.1.0.s390x.xml new file mode 100644 index ..af2647aa --- /dev/null +++ b/tests/qemucapabilitiesdata/caps_5.1.0.s390x.xml @@ -0,0 +1,3278 @@ + + /usr/bin/qemu-system-s390x + 0 + 0 + 0 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
[libvirt PATCH] NEWS: restore backtick balance
Signed-off-by: Ján Tomko Reported-by: Michal Prívozník Fixes: db98d17709eeb13603730352a70f3817becd7372 --- Pushed. NEWS.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.rst b/NEWS.rst index 807f52..aa8a217eb6 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -33,7 +33,7 @@ v6.10.0 (unreleased) * hyperv: implement new APIs -The `virDomainGetMaxMemory()``, ``virDomainSetMaxMemory()``, +The ``virDomainGetMaxMemory()``, ``virDomainSetMaxMemory()``, ``virDomainGetSchedulerType()``, ``virDomainGetSchedulerParameters()``, ``virDomainGetSchedulerParametersFlags()``, ``virDomainGetVcpus()``, ``virDomainGetVcpusFlags()``, ``virDomainGetMaxVcpus()``, -- 2.26.2
Re: [PATCH v3 6/6] news: Document recent OpenSSH authorized key file mgmt APIs
On 11/18/20 3:31 PM, Peter Krempa wrote: On Wed, Nov 18, 2020 at 14:34:24 +0100, Michal Privoznik wrote: Signed-off-by: Michal Privoznik --- NEWS.rst | 7 +++ 1 file changed, 7 insertions(+) Reviewed-by: Peter Krempa I've pushed these. Thank you for all three rounds of review! Michal
[libvirt PATCH v2 9/9] cpu_map: Add missing feature fsrm
Signed-off-by: Tim Wiederhake --- src/cpu_map/x86_features.xml | 3 +++ tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-guest.xml | 1 + tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml | 1 + 3 files changed, 5 insertions(+) diff --git a/src/cpu_map/x86_features.xml b/src/cpu_map/x86_features.xml index a55f52b16c..b0bf22d916 100644 --- a/src/cpu_map/x86_features.xml +++ b/src/cpu_map/x86_features.xml @@ -339,6 +339,9 @@ + + + diff --git a/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-guest.xml b/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-guest.xml index 9b75ace710..3a71b28cfb 100644 --- a/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-guest.xml +++ b/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-guest.xml @@ -24,6 +24,7 @@ + diff --git a/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml b/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml index efbf9d363b..1582de0422 100644 --- a/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml +++ b/tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml @@ -25,6 +25,7 @@ + -- 2.26.2
[libvirt PATCH v2 8/9] cpu_map: sync_qemu_cpu_i386: Detect features missing in libvirt
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 17 +
1 file changed, 17 insertions(+)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 69114c5bb6..4743161b0c 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -5,6 +5,7 @@ import copy
import lark
import os
import re
+import xml.etree.ElementTree
def translate_vendor(name):
@@ -393,6 +394,22 @@ def main():
with open(name, "wt") as f:
output_model(f, model)
+features = set()
+for model in models:
+features.update(model["features"])
+
+try:
+filename = os.path.join(args.outdir, "x86_features.xml")
+dom = xml.etree.ElementTree.parse(filename)
+known = [x.attrib["name"] for x in dom.getroot().iter("feature")]
+unknown = [x for x in features if x not in known and x is not None]
+except Exception as e:
+unknown = []
+print("warning: Unable to read libvirt x86_features.xml: {}".format(e))
+
+for x in unknown:
+print("warning: Feature unknown to libvirt: {}".format(x))
+
if __name__ == "__main__":
main()
--
2.26.2
[libvirt PATCH v2 7/9] cpu_map: sync_qemu_cpu_i386: Add missing features
Signed-off-by: Tim Wiederhake --- src/cpu_map/sync_qemu_i386.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py index b27e5793ff..69114c5bb6 100755 --- a/src/cpu_map/sync_qemu_i386.py +++ b/src/cpu_map/sync_qemu_i386.py @@ -32,6 +32,7 @@ def translate_feature(name): "CPUID_7_0_EBX_AVX512DQ": "avx512dq", "CPUID_7_0_EBX_AVX512ER": "avx512er", "CPUID_7_0_EBX_AVX512F": "avx512f", +"CPUID_7_0_EBX_AVX512IFMA": "avx512ifma", "CPUID_7_0_EBX_AVX512PF": "avx512pf", "CPUID_7_0_EBX_AVX512VL": "avx512vl", "CPUID_7_0_EBX_BMI1": "bmi1", @@ -67,6 +68,7 @@ def translate_feature(name): "CPUID_7_0_EDX_AVX512_4FMAPS": "avx512-4fmaps", "CPUID_7_0_EDX_AVX512_4VNNIW": "avx512-4vnniw", "CPUID_7_0_EDX_CORE_CAPABILITY": "core-capability", +"CPUID_7_0_EDX_FSRM": "fsrm", "CPUID_7_0_EDX_SPEC_CTRL": "spec-ctrl", "CPUID_7_0_EDX_SPEC_CTRL_SSBD": "ssbd", "CPUID_7_0_EDX_STIBP": "stibp", -- 2.26.2
[libvirt PATCH v2 4/9] cpu_map: sync_qemu_cpu_i386: Do not ignore CPUID_EXT_MONITOR
Signed-off-by: Tim Wiederhake --- src/cpu_map/sync_qemu_i386.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py index 6a5a02dbd1..21981c58f1 100755 --- a/src/cpu_map/sync_qemu_i386.py +++ b/src/cpu_map/sync_qemu_i386.py @@ -108,6 +108,7 @@ def translate_feature(name): "CPUID_EXT_CX16": "cx16", "CPUID_EXT_F16C": "f16c", "CPUID_EXT_FMA": "fma", +"CPUID_EXT_MONITOR": "monitor", "CPUID_EXT_MOVBE": "movbe", "CPUID_EXT_PCID": "pcid", "CPUID_EXT_PCLMULQDQ": "pclmuldq", @@ -153,7 +154,6 @@ def translate_feature(name): "MSR_CORE_CAP_SPLIT_LOCK_DETECT": "split-lock-detect", # always disabled features -"CPUID_EXT_MONITOR": None, "0": None, # set to "no auto enable" by qemu -- 2.26.2
[libvirt PATCH v2 2/9] cpu_map: sync_qemu_cpu_i386: Factor out translation of vendors
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 82413d831e..fe95f26511 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -8,11 +8,6 @@ import re
T = {
-# translating qemu -> libvirt cpu vendor names
-"CPUID_VENDOR_AMD": "AMD",
-"CPUID_VENDOR_INTEL": "Intel",
-"CPUID_VENDOR_HYGON": "Hygon",
-
# translating qemu -> libvirt cpu feature names
"CPUID_6_EAX_ARAT": "arat",
"CPUID_7_0_EBX_ADX": "adx",
@@ -152,6 +147,20 @@ T = {
}
+def translate_vendor(name):
+T = {
+"CPUID_VENDOR_AMD": "AMD",
+"CPUID_VENDOR_INTEL": "Intel",
+"CPUID_VENDOR_HYGON": "Hygon",
+}
+
+if name in T:
+return T[name]
+
+print("warning: Unknown vendor '{}'".format(name))
+return name
+
+
def readline_cont(f):
"""Read one logical line from a file `f` i.e. continues lines that end in
a backslash."""
@@ -264,7 +273,7 @@ def expand_model(model):
result = {
"name": model.pop(".name"),
-"vendor": T[model.pop(".vendor")],
+"vendor": translate_vendor(model.pop(".vendor")),
"features": set(),
"extra": dict()}
--
2.26.2
[libvirt PATCH v2 6/9] cpu_map: sync_qemu_cpu_i386: Translate qemu feature pretty names
If a feature is added (or removed) in a QEMU CPU model version, we
get to see the QEMU pretty name for the feature, not the name of
the macro.
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 2219e67454..b27e5793ff 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -158,6 +158,7 @@ def translate_feature(name):
name == "0",
name.startswith("VMX_"),
name.startswith("MSR_VMX_"),
+name.startswith("vmx-"),
# set to "no auto enable" by qemu
name == "CPUID_EXT3_TOPOEXT",
@@ -170,6 +171,10 @@ def translate_feature(name):
if name in T:
return T[name]
+for v in T.values():
+if name.replace("-", "_") == v.replace("-", "_"):
+return v
+
print("warning: Unknown feature '{}'".format(name))
return name
--
2.26.2
[libvirt PATCH v2 5/9] cpu_map: sync_qemu_cpu_i386: Simplify ignore features
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 21981c58f1..2219e67454 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -152,14 +152,20 @@ def translate_feature(name):
"MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY": "skip-l1dfl-vmentry",
"MSR_ARCH_CAP_TAA_NO": "taa-no",
"MSR_CORE_CAP_SPLIT_LOCK_DETECT": "split-lock-detect",
+}
-# always disabled features
-"0": None,
+ignore = (
+name == "0",
+name.startswith("VMX_"),
+name.startswith("MSR_VMX_"),
# set to "no auto enable" by qemu
-"CPUID_EXT3_TOPOEXT": None,
-"MSR_VMX_BASIC_DUAL_MONITOR": None,
-}
+name == "CPUID_EXT3_TOPOEXT",
+name == "MSR_VMX_BASIC_DUAL_MONITOR",
+)
+
+if any(ignore):
+return None
if name in T:
return T[name]
@@ -291,8 +297,6 @@ def expand_model(model):
for k in [k for k in model if k.startswith(".features")]:
v = model.pop(k)
for feature in v.split():
-if feature.startswith("VMX_") or feature.startswith("MSR_VMX_"):
-continue
translated = translate_feature(feature)
if translated:
result["features"].add(translated)
--
2.26.2
[libvirt PATCH v2 3/9] cpu_map: sync_qemu_cpu_i386: Factor out translation of features
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 291 +-
1 file changed, 149 insertions(+), 142 deletions(-)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index fe95f26511..6a5a02dbd1 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -7,146 +7,6 @@ import os
import re
-T = {
-# translating qemu -> libvirt cpu feature names
-"CPUID_6_EAX_ARAT": "arat",
-"CPUID_7_0_EBX_ADX": "adx",
-"CPUID_7_0_EBX_AVX2": "avx2",
-"CPUID_7_0_EBX_AVX512BW": "avx512bw",
-"CPUID_7_0_EBX_AVX512CD": "avx512cd",
-"CPUID_7_0_EBX_AVX512DQ": "avx512dq",
-"CPUID_7_0_EBX_AVX512ER": "avx512er",
-"CPUID_7_0_EBX_AVX512F": "avx512f",
-"CPUID_7_0_EBX_AVX512PF": "avx512pf",
-"CPUID_7_0_EBX_AVX512VL": "avx512vl",
-"CPUID_7_0_EBX_BMI1": "bmi1",
-"CPUID_7_0_EBX_BMI2": "bmi2",
-"CPUID_7_0_EBX_CLFLUSHOPT": "clflushopt",
-"CPUID_7_0_EBX_CLWB": "clwb",
-"CPUID_7_0_EBX_ERMS": "erms",
-"CPUID_7_0_EBX_FSGSBASE": "fsgsbase",
-"CPUID_7_0_EBX_HLE": "hle",
-"CPUID_7_0_EBX_INVPCID": "invpcid",
-"CPUID_7_0_EBX_MPX": "mpx",
-"CPUID_7_0_EBX_RDSEED": "rdseed",
-"CPUID_7_0_EBX_RTM": "rtm",
-"CPUID_7_0_EBX_SHA_NI": "sha-ni",
-"CPUID_7_0_EBX_SMAP": "smap",
-"CPUID_7_0_EBX_SMEP": "smep",
-"CPUID_7_0_ECX_AVX512BITALG": "avx512bitalg",
-"CPUID_7_0_ECX_AVX512_VBMI2": "avx512vbmi2",
-"CPUID_7_0_ECX_AVX512_VBMI": "avx512vbmi",
-"CPUID_7_0_ECX_AVX512VNNI": "avx512vnni",
-"CPUID_7_0_ECX_AVX512_VPOPCNTDQ": "avx512-vpopcntdq",
-"CPUID_7_0_ECX_CLDEMOTE": "cldemote",
-"CPUID_7_0_ECX_GFNI": "gfni",
-"CPUID_7_0_ECX_LA57": "la57",
-"CPUID_7_0_ECX_MOVDIR64B": "movdir64b",
-"CPUID_7_0_ECX_MOVDIRI": "movdiri",
-"CPUID_7_0_ECX_PKU": "pku",
-"CPUID_7_0_ECX_RDPID": "rdpid",
-"CPUID_7_0_ECX_UMIP": "umip",
-"CPUID_7_0_ECX_VAES": "vaes",
-"CPUID_7_0_ECX_VPCLMULQDQ": "vpclmulqdq",
-"CPUID_7_0_EDX_ARCH_CAPABILITIES": "arch-capabilities",
-"CPUID_7_0_EDX_AVX512_4FMAPS": "avx512-4fmaps",
-"CPUID_7_0_EDX_AVX512_4VNNIW": "avx512-4vnniw",
-"CPUID_7_0_EDX_CORE_CAPABILITY": "core-capability",
-"CPUID_7_0_EDX_SPEC_CTRL": "spec-ctrl",
-"CPUID_7_0_EDX_SPEC_CTRL_SSBD": "ssbd",
-"CPUID_7_0_EDX_STIBP": "stibp",
-"CPUID_7_1_EAX_AVX512_BF16": "avx512-bf16",
-"CPUID_8000_0008_EBX_CLZERO": "clzero",
-"CPUID_8000_0008_EBX_IBPB": "ibpb",
-"CPUID_8000_0008_EBX_STIBP": "amd-stibp",
-"CPUID_8000_0008_EBX_WBNOINVD": "wbnoinvd",
-"CPUID_8000_0008_EBX_XSAVEERPTR": "xsaveerptr",
-"CPUID_ACPI": "acpi",
-"CPUID_APIC": "apic",
-"CPUID_CLFLUSH": "clflush",
-"CPUID_CMOV": "cmov",
-"CPUID_CX8": "cx8",
-"CPUID_DE": "de",
-"CPUID_EXT2_3DNOW": "3dnow",
-"CPUID_EXT2_3DNOWEXT": "3dnowext",
-"CPUID_EXT2_FFXSR": "fxsr_opt",
-"CPUID_EXT2_LM": "lm",
-"CPUID_EXT2_MMXEXT": "mmxext",
-"CPUID_EXT2_NX": "nx",
-"CPUID_EXT2_PDPE1GB": "pdpe1gb",
-"CPUID_EXT2_RDTSCP": "rdtscp",
-"CPUID_EXT2_SYSCALL": "syscall",
-"CPUID_EXT3_3DNOWPREFETCH": "3dnowprefetch",
-"CPUID_EXT3_ABM": "abm",
-"CPUID_EXT3_CR8LEG": "cr8legacy",
-"CPUID_EXT3_FMA4": "fma4",
-"CPUID_EXT3_LAHF_LM": "lahf_lm",
-"CPUID_EXT3_MISALIGNSSE": "misalignsse",
-"CPUID_EXT3_OSVW": "osvw",
-"CPUID_EXT3_PERFCORE": "perfctr_core",
-"CPUID_EXT3_SSE4A": "sse4a",
-"CPUID_EXT3_SVM": "svm",
-"CPUID_EXT3_TBM": "tbm",
-"CPUID_EXT3_XOP": "xop",
-"CPUID_EXT_AES": "aes",
-"CPUID_EXT_AVX": "avx",
-"CPUID_EXT_CX16": "cx16",
-"CPUID_EXT_F16C": "f16c",
-"CPUID_EXT_FMA": "fma",
-"CPUID_EXT_MOVBE": "movbe",
-"CPUID_EXT_PCID": "pcid",
-"CPUID_EXT_PCLMULQDQ": "pclmuldq",
-"CPUID_EXT_POPCNT": "popcnt",
-"CPUID_EXT_RDRAND": "rdrand",
-"CPUID_EXT_SSE3": "pni",
-"CPUID_EXT_SSE41": "sse4.1",
-"CPUID_EXT_SSE42": "sse4.2",
-"CPUID_EXT_SSSE3": "ssse3",
-"CPUID_EXT_TSC_DEADLINE_TIMER": "tsc-deadline",
-"CPUID_EXT_X2APIC": "x2apic",
-"CPUID_EXT_XSAVE": "xsave",
-"CPUID_FP87": "fpu",
-"CPUID_FXSR": "fxsr",
-"CPUID_MCA": "mca",
-"CPUID_MCE": "mce",
-"CPUID_MMX": "mmx",
-"CPUID_MSR": "msr",
-"CPUID_MTRR": "mtrr",
-"CPUID_PAE": "pae",
-"CPUID_PAT": "pat",
-"CPUID_PGE": "pge",
-"CPUID_PSE36": "pse36",
-"CPUID_PSE": "pse",
-"CPUID_SEP": "sep",
-"CPUID_SSE2": "sse2",
-"CPUID_SSE": "sse",
-"CPUID_SS": "ss",
-"CPUID_SVM_NPT": "npt",
-"CPUID_SVM_NRIPSAVE": "nrip-save",
-"CPUID_TSC": "tsc",
-"CPUID_VME": "vme",
-"CPUID_XSAVE_XGETBV1": "xgetbv1",
-"CPUID_XSAVE_XSAVEC": "xsavec",
-"CPUID_XSAVE_XSAVEOPT": "xsaveopt",
-"CPUID_XSAVE_XSAVES": "xsaves",
-"MSR_ARCH_CAP_IBRS_ALL": "ibrs-all",
-"MSR_ARCH_CAP_MDS_NO": "mds-no",
-"MSR_ARCH_CAP_PSCHANGE_MC_NO": "pschange-mc-no",
-"MSR_ARCH_CAP_RDCL_NO": "rdctl-no",
-
[libvirt PATCH v2 0/9] Add missing feature detection to sync tool in cpu_map
sync_qemu_i386.py in src/cpu_map is a tool to sync CPU models from qemu with libvirt. It currently has no provisions for detecting new features that are not implemented in libvirt yet. This series changes that. Currently missing x86 CPU models: * "Denverton" * "KnightsMill" * "Snowridge" Currently missing x86 CPU features: * "core-capability" * "fsrm" * "split-lock-detect" "fsrm" is added in this series. For the "Snowridge" CPU model and the "core-capability" and "split-lock-detect" features, see https://www.redhat.com/archives/libvir-list/2020-November/msg00924.html. V1: https://www.redhat.com/archives/libvir-list/2020-November/msg01002.html Tim Wiederhake (9): cpu_map: sync_qemu_cpu_i386: Translate features in model versions cpu_map: sync_qemu_cpu_i386: Factor out translation of vendors cpu_map: sync_qemu_cpu_i386: Factor out translation of features cpu_map: sync_qemu_cpu_i386: Do not ignore CPUID_EXT_MONITOR cpu_map: sync_qemu_cpu_i386: Simplify ignore features cpu_map: sync_qemu_cpu_i386: Translate qemu feature pretty names cpu_map: sync_qemu_cpu_i386: Add missing features cpu_map: sync_qemu_cpu_i386: Detect features missing in libvirt cpu_map: Add missing feature fsrm src/cpu_map/sync_qemu_i386.py | 340 ++ src/cpu_map/x86_features.xml | 3 + .../x86_64-cpuid-Ice-Lake-Server-guest.xml| 1 + .../x86_64-cpuid-Ice-Lake-Server-host.xml | 1 + 4 files changed, 198 insertions(+), 147 deletions(-) -- 2.26.2
[libvirt PATCH v2 1/9] cpu_map: sync_qemu_cpu_i386: Translate features in model versions
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 8deda869df..82413d831e 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -292,6 +292,8 @@ def expand_model(model):
props = version.pop(".props", dict())
for k, v in props:
+if k not in ("model-id", "stepping", "model"):
+k = T.get(k, k)
if v == "on":
result["features"].add(k)
elif v == "off" and k in result["features"]:
--
2.26.2
Re: [PATCH v3 6/6] news: Document recent OpenSSH authorized key file mgmt APIs
On Wed, Nov 18, 2020 at 14:34:24 +0100, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- > NEWS.rst | 7 +++ > 1 file changed, 7 insertions(+) Reviewed-by: Peter Krempa
Re: [PATCH v3 3/6] virsh: Expose OpenSSH authorized key file mgmt APIs
On Wed, Nov 18, 2020 at 14:34:21 +0100, Michal Privoznik wrote:
> The new virsh commands are:
>
> get-user-sshkeys
> set-user-sshkeys
>
> Signed-off-by: Michal Privoznik
> ---
> docs/manpages/virsh.rst | 38 ++
> tools/virsh-domain.c| 164
> 2 files changed, 202 insertions(+)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index bfd26e3120..543f62d429 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
[...]
> @@ -4004,6 +4019,29 @@ For QEMU/KVM, this requires the guest agent to be
> configured
> and running.
>
>
> +set-user-sshkeys
> +
> +
> +**Syntax:**
> +
> +::
> +
> + set-user-sshkeys domain user [--file FILE] [{--reset | --remove}]
> +
> +Append keys read from *FILE* into *user*'sSSH authorized keys file in the
> guest
s/sS/s S/
> +*domain*. In the *FILE* keys must be on separate lines and each line must
> +follow authorized keys format as defined by *sshd(8)*.
> +
> +If *--reset* is specified, then the guest authorized keys file content is
> +removed before appending new keys. As a special case, if *--reset* is
> provided
> +and no *FILE* was provided then no new keys are added and the authorized keys
> +file is cleared out.
> +
> +If *--remove* is specified, then instead of adding any new keys then keys
> read
> +from *FILE* are removed from the authorized keys file. It is not considered
> an
> +error if the key does not exist in the file.
Reviewed-by: Peter Krempa
[libvirt PATCH] qemu_conf: fix a typo in comment
Ceci n'est pas un objet. Signed-off-by: Ján Tomko Fixes: 7db61843b05a6e4295b1d2e27a3d86f162ef04a0 --- Pushed under the surreal rule. src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 80ff18204e..83de26ab56 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1391,7 +1391,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) * driver. If @refresh is true, the capabilities will be * rebuilt first * - * The caller must release the reference with virObjetUnref + * The caller must release the reference with virObjectUnref * * Returns: a reference to a virCapsPtr instance or NULL */ -- 2.26.2
Re: [PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask
On Wed, Nov 18, 2020 at 01:06:07PM +0100, Michal Privoznik wrote: > The way our domain capabilities work currently, is that we have > virDomainCapsEnum struct which contains 'unsigned int values' > member which serves as a bitmask. More complicated structs are > composed from this struct, giving us whole virDomainCaps > eventually. > > Whenever we want to report that a certain value is supported, the > '1 << value' bit is set in the corresponding unsigned int member. > This works as long as the resulting value after bitshift does not > overflow unsigned int. There is a check inside > virDomainCapsEnumSet() which ensures exactly this, but no caller > really checks whether virDomainCapsEnumSet() succeeded. Also, > checking at runtime is a bit too late. > > Fortunately, we know the largest value we want to store in each > member, because each enum of ours ends with _LAST member. > Therefore, we can check at build time whether an overflow can > occur. I'm wondering how does this build failure knowledge actually help us? Are we going to start re-evaluating our approach to enums, splitting them somehow? I don't think so. The standard only mandates the enum to be large enough so that the constants fit into an int, but it's been a while since then and 32bit is simply not going to cut it. The almighty internet also claims that compilers (GCC specifically) automatically re-size the enum given the declaration, so if you do 1 << 32, I would expect that the compiler should allocate a 64bit data type, once we're past 64, well, no static assert is going to help us anyway, so we need to start thinking about this more intensively. Additionally, since we're using compiler extension quite a bit already, I say we make use of those if type expansion is not automatic (I think you have to use it explicitly if you want to force a smaller type, like a short int). In case I'm misunderstanding something, then I still think that the macro definition should go to something like internal.h and be named in a consistent fashion like VIR_ENUM_STATIC_ASSERT. Moreover, GLib claims the macro can be used anywhere where the typedef is in scope, thus I believe the right place to put these asserts is right after the actual typedef rather than before a specific struct - doing this also avoid duplication if several structures make us of e.g. virTristateBool. Erik
[PATCH v3 4/6] qemu_agent: add qemuAgentSSH{Add, Remove, Get}AuthorizedKeys
From: Marc-André Lureau
In QEMU 5.2, the guest agent learned to manipulate a user
~/.ssh/authorized_keys. Bind the JSON API to libvirt.
https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
Signed-off-by: Marc-André Lureau
Signed-off-by: Michal Privoznik
Reviewed-by: Peter Krempa
---
src/qemu/qemu_agent.c | 141 ++
src/qemu/qemu_agent.h | 15 +
tests/qemuagenttest.c | 79 +++
3 files changed, 235 insertions(+)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 7fbb4a9431..230253d404 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2496,3 +2496,144 @@ qemuAgentSetResponseTimeout(qemuAgentPtr agent,
{
agent->timeout = timeout;
}
+
+/**
+ * qemuAgentSSHGetAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to get authorized keys for
+ * @keys: Array of authorized keys
+ *
+ * Fetch the public keys from @user's $HOME/.ssh/authorized_keys.
+ *
+ * Returns: number of keys returned on success,
+ * -1 otherwise (error is reported)
+ */
+int
+qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ char ***keys)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValuePtr data = NULL;
+size_t ndata;
+size_t i;
+char **keys_ret = NULL;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys",
+ "s:username", user,
+ NULL)))
+return -1;
+
+if (qemuAgentCommand(agent, cmd, , agent->timeout) < 0)
+return -1;
+
+if (!(data = virJSONValueObjectGetObject(reply, "return")) ||
+!(data = virJSONValueObjectGetArray(data, "keys"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("qemu agent didn't return an array of keys"));
+return -1;
+}
+
+ndata = virJSONValueArraySize(data);
+
+keys_ret = g_new0(char *, ndata + 1);
+
+for (i = 0; i < ndata; i++) {
+virJSONValuePtr entry = virJSONValueArrayGet(data, i);
+
+if (!entry) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("array element missing in
guest-ssh-get-authorized-keys return value"));
+goto error;
+}
+
+keys_ret[i] = g_strdup(virJSONValueGetString(entry));
+}
+
+*keys = g_steal_pointer(_ret);
+return ndata;
+
+ error:
+virStringListFreeCount(keys_ret, ndata);
+return -1;
+}
+
+
+/**
+ * qemuAgentSSHAddAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to add authorized keys for
+ * @keys: Array of authorized keys
+ * @nkeys: number of items in @keys array
+ * @reset: whether to truncate authorized keys file before writing
+ *
+ * Append SSH @keys into the @user's authorized keys file. If
+ * @reset is true then the file is truncated before write and
+ * thus contains only newly added @keys.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (error is reported)
+ */
+int
+qemuAgentSSHAddAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ const char **keys,
+ size_t nkeys,
+ bool reset)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) jkeys = NULL;
+
+jkeys = qemuAgentMakeStringsArray(keys, nkeys);
+if (jkeys == NULL)
+return -1;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-add-authorized-keys",
+ "s:username", user,
+ "a:keys", ,
+ "b:reset", reset,
+ NULL)))
+return -1;
+
+return qemuAgentCommand(agent, cmd, , agent->timeout);
+}
+
+
+/**
+ * qemuAgentSSHRemoveAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to remove authorized keys for
+ * @keys: Array of authorized keys
+ * @nkeys: number of items in @keys array
+ *
+ * Remove SSH @keys from the @user's authorized keys file. It's
+ * not considered an error when trying to remove a non-existent
+ * key.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (error is reported)
+ */
+int
+qemuAgentSSHRemoveAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ const char **keys,
+ size_t nkeys)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) jkeys = NULL;
+
+jkeys = qemuAgentMakeStringsArray(keys, nkeys);
+if (jkeys == NULL)
+return -1;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-remove-authorized-keys",
+ "s:username", user,
+ "a:keys", ,
+
[PATCH v3 6/6] news: Document recent OpenSSH authorized key file mgmt APIs
Signed-off-by: Michal Privoznik --- NEWS.rst | 7 +++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 16e75c5c3f..807f52 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -24,6 +24,13 @@ v6.10.0 (unreleased) * **New features** + * qemu: Implement OpenSSH authorized key file management APIs + +New APIs (``virDomainAuthorizedSSHKeysGet()`` and +``virDomainAuthorizedSSHKeysSet()``) and virsh commands +(``get-user-sshkeys`` and ``set-user-sshkeys``) are added to manage +authorized_keys SSH file for user. + * hyperv: implement new APIs The `virDomainGetMaxMemory()``, ``virDomainSetMaxMemory()``, -- 2.26.2
[PATCH v3 5/6] qemu: Implement OpenSSH authorized key file mgmt APIs
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1888537
Signed-off-by: Michal Privoznik
Reviewed-by: Peter Krempa
---
src/qemu/qemu_driver.c | 81 ++
1 file changed, 81 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ac38edf009..b69be1bedc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20094,6 +20094,85 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom,
}
+static int
+qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuAgentPtr agent;
+int rv = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0)
+return -1;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+return -1;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endagentjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+rv = qemuAgentSSHGetAuthorizedKeys(agent, user, keys);
+qemuDomainObjExitAgent(vm, agent);
+
+ endagentjob:
+qemuDomainObjEndAgentJob(vm);
+virDomainObjEndAPI();
+return rv;
+}
+
+
+static int
+qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
+ const char *user,
+ const char **keys,
+ int nkeys,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+g_autoptr(virDomainObj) vm = NULL;
+qemuAgentPtr agent;
+const bool append = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
+const bool remove = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
+int rv = -1;
+
+virCheckFlags(VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND |
+ VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainAuthorizedSshKeysSetEnsureACL(dom->conn, vm->def) < 0)
+return -1;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+return -1;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endagentjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+if (remove)
+rv = qemuAgentSSHRemoveAuthorizedKeys(agent, user, keys, nkeys);
+else
+rv = qemuAgentSSHAddAuthorizedKeys(agent, user, keys, nkeys, !append);
+qemuDomainObjExitAgent(vm, agent);
+
+ endagentjob:
+qemuDomainObjEndAgentJob(vm);
+virDomainObjEndAPI();
+return rv;
+}
+
+
static virHypervisorDriver qemuHypervisorDriver = {
.name = QEMU_DRIVER_NAME,
.connectURIProbe = qemuConnectURIProbe,
@@ -20333,6 +20412,8 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /*
5.10.0 */
.domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
.domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
+.domainAuthorizedSSHKeysGet = qemuDomainAuthorizedSSHKeysGet, /* 6.10.0 */
+.domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
};
--
2.26.2
[PATCH v3 3/6] virsh: Expose OpenSSH authorized key file mgmt APIs
The new virsh commands are:
get-user-sshkeys
set-user-sshkeys
Signed-off-by: Michal Privoznik
---
docs/manpages/virsh.rst | 38 ++
tools/virsh-domain.c| 164
2 files changed, 202 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..543f62d429 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -2636,6 +2636,21 @@ When *--timestamp* is used, a human-readable timestamp
will be printed
before the event.
+get-user-sshkeys
+
+
+**Syntax:**
+
+::
+
+ get-user-sshkeys domain user
+
+Print SSH authorized keys for given *user* in the guest *domain*. Please note,
+that an entry in the file has internal structure as defined by *sshd(8)* and
+virsh/libvirt does handle keys as opaque strings, i.e. does not interpret
+them.
+
+
guest-agent-timeout
---
@@ -4004,6 +4019,29 @@ For QEMU/KVM, this requires the guest agent to be
configured
and running.
+set-user-sshkeys
+
+
+**Syntax:**
+
+::
+
+ set-user-sshkeys domain user [--file FILE] [{--reset | --remove}]
+
+Append keys read from *FILE* into *user*'sSSH authorized keys file in the guest
+*domain*. In the *FILE* keys must be on separate lines and each line must
+follow authorized keys format as defined by *sshd(8)*.
+
+If *--reset* is specified, then the guest authorized keys file content is
+removed before appending new keys. As a special case, if *--reset* is provided
+and no *FILE* was provided then no new keys are added and the authorized keys
+file is cleared out.
+
+If *--remove* is specified, then instead of adding any new keys then keys read
+from *FILE* are removed from the authorized keys file. It is not considered an
+error if the key does not exist in the file.
+
+
setmaxmem
-
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..c999458d72 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14263,6 +14263,158 @@ cmdGuestInfo(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "get-user-sshkeys" command
+ */
+static const vshCmdInfo info_get_user_sshkeys[] = {
+{.name = "help",
+ .data = N_("list authorized SSH keys for given user (via agent)")
+},
+{.name = "desc",
+ .data = N_("Use the guest agent to query authorized SSH keys for given "
+"user")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_get_user_sshkeys[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+{.name = "user",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("user to list authorized keys for"),
+},
+{.name = NULL}
+};
+
+static bool
+cmdGetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *user;
+VIR_AUTOSTRINGLIST keys = NULL;
+int nkeys = 0;
+size_t i;
+const unsigned int flags = 0;
+bool ret = false;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "user", ) < 0)
+goto cleanup;
+
+nkeys = virDomainAuthorizedSSHKeysGet(dom, user, , flags);
+if (nkeys < 0)
+goto cleanup;
+
+for (i = 0; i < nkeys; i++) {
+vshPrint(ctl, "%s", keys[i]);
+}
+
+ret = true;
+ cleanup:
+virshDomainFree(dom);
+return ret;
+}
+
+
+/*
+ * "set-user-sshkeys" command
+ */
+static const vshCmdInfo info_set_user_sshkeys[] = {
+{.name = "help",
+ .data = N_("manipulate authorized SSH keys file for given user (via
agent)")
+},
+{.name = "desc",
+ .data = N_("Append, reset or remove specified key from the authorized "
+"keys file for given user")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_set_user_sshkeys[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+{.name = "user",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("user to set authorized keys for"),
+},
+{.name = "file",
+ .type = VSH_OT_STRING,
+ .help = N_("optional file to read keys from"),
+},
+{.name = "reset",
+ .type = VSH_OT_BOOL,
+ .help = N_("clear out authorized keys file before adding new keys"),
+},
+{.name = "remove",
+ .type = VSH_OT_BOOL,
+ .help = N_("remove keys from the authorized keys file"),
+},
+{.name = NULL}
+};
+
+static bool
+cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *user;
+const char *from;
+g_autofree char *buffer = NULL;
+VIR_AUTOSTRINGLIST keys = NULL;
+int nkeys = 0;
+unsigned int flags = 0;
+bool ret = false;
+
+VSH_REQUIRE_OPTION("remove", "file");
+VSH_EXCLUSIVE_OPTIONS("reset", "remove");
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "user", )
[PATCH v3 2/6] remote: Implement OpenSSH authorized key file mgmt APIs
Since both APIs accept/return an array of strings we can't have
client/server dispatch code generated. But implementation is
fairly trivial, although verbose.
Signed-off-by: Michal Privoznik
Reviewed-by: Peter Krempa
---
src/remote/remote_daemon_dispatch.c | 82 +++
src/remote/remote_driver.c | 87 +
src/remote/remote_protocol.x| 34 ++-
src/remote_protocol-structs | 22
4 files changed, 224 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index eb5f6ebb0c..46683aa4a7 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7381,3 +7381,85 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server
G_GNUC_UNUSED,
return rv;
}
+
+static int
+remoteDispatchDomainAuthorizedSshKeysGet(virNetServerPtr server G_GNUC_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessageErrorPtr rerr,
+
remote_domain_authorized_ssh_keys_get_args *args,
+
remote_domain_authorized_ssh_keys_get_ret *ret)
+{
+int rv = -1;
+virConnectPtr conn = remoteGetHypervisorConn(client);
+int nkeys = 0;
+char **keys = NULL;
+virDomainPtr dom = NULL;
+
+if (!conn)
+goto cleanup;
+
+if (!(dom = get_nonnull_domain(conn, args->dom)))
+goto cleanup;
+
+if ((nkeys = virDomainAuthorizedSSHKeysGet(dom, args->user,
+ , args->flags)) < 0)
+goto cleanup;
+
+if (nkeys > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Number of keys %d, which exceeds max liit: %d"),
+ nkeys, REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX);
+goto cleanup;
+}
+
+ret->keys.keys_val = g_steal_pointer();
+ret->keys.keys_len = nkeys;
+
+rv = nkeys;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+if (nkeys > 0)
+virStringListFreeCount(keys, nkeys);
+virObjectUnref(dom);
+
+return rv;
+}
+
+static int
+remoteDispatchDomainAuthorizedSshKeysSet(virNetServerPtr server G_GNUC_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessageErrorPtr rerr,
+
remote_domain_authorized_ssh_keys_set_args *args)
+{
+int rv = -1;
+virConnectPtr conn = remoteGetHypervisorConn(client);
+virDomainPtr dom = NULL;
+
+if (!conn)
+goto cleanup;
+
+if (!(dom = get_nonnull_domain(conn, args->dom)))
+goto cleanup;
+
+if (args->keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Number of keys %d, which exceeds max liit: %d"),
+ args->keys.keys_len,
REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX);
+goto cleanup;
+}
+
+rv = virDomainAuthorizedSSHKeysSet(dom, args->user,
+ (const char **) args->keys.keys_val,
+ args->keys.keys_len, args->flags);
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+virObjectUnref(dom);
+
+return rv;
+}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index dd5e8eeed2..6c0e7f7514 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8017,6 +8017,91 @@ remoteDomainGetGuestInfo(virDomainPtr dom,
return rv;
}
+static int
+remoteDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+int rv = -1;
+size_t i;
+struct private_data *priv = domain->conn->privateData;
+remote_domain_authorized_ssh_keys_get_args args;
+remote_domain_authorized_ssh_keys_get_ret ret;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(, domain);
+args.user = (char *) user;
+args.flags = flags;
+memset(, 0, sizeof(ret));
+
+if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET,
+ (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_args, (char
*),
+ (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_ret, (char
*)) == -1) {
+goto cleanup;
+}
+
+if (ret.keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_RPC, "%s",
+ _("remoteDomainAuthorizedSSHKeysGet: "
+ "returned number of keys exceeds
[PATCH v3 0/6] Introduce OpenSSH authorized key file mgmt APIs
v3 of:
https://www.redhat.com/archives/libvir-list/2020-November/msg00821.html
diff to v2:
- some patches are reviewed already, I'm sending them for completeness.
- worked in Peter's review of v2: virsh set-user-sshkeys semantics
change (append behaviour is the default), allocate N+1 keys to return a
string list in virDomainAuthorizedSSHKeysGet(), etc.
Marc-André Lureau (1):
qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5):
Introduce OpenSSH authorized key file mgmt APIs
remote: Implement OpenSSH authorized key file mgmt APIs
virsh: Expose OpenSSH authorized key file mgmt APIs
qemu: Implement OpenSSH authorized key file mgmt APIs
news: Document recent OpenSSH authorized key file mgmt APIs
NEWS.rst| 7 ++
docs/manpages/virsh.rst | 38 +++
include/libvirt/libvirt-domain.h| 17 +++
src/driver-hypervisor.h | 15 +++
src/libvirt-domain.c| 133 ++
src/libvirt_public.syms | 6 +
src/qemu/qemu_agent.c | 141
src/qemu/qemu_agent.h | 15 +++
src/qemu/qemu_driver.c | 81 ++
src/remote/remote_daemon_dispatch.c | 82 ++
src/remote/remote_driver.c | 87 +++
src/remote/remote_protocol.x| 34 +-
src/remote_protocol-structs | 22
tests/qemuagenttest.c | 79 ++
tools/virsh-domain.c| 164
15 files changed, 920 insertions(+), 1 deletion(-)
--
2.26.2
[PATCH v3 1/6] Introduce OpenSSH authorized key file mgmt APIs
When setting up a new guest or when a management software wants
to allow access to an existing guest the
virDomainSetUserPassword() API can be used, but that might be not
good enough if user want to ssh into the guest. Not only sshd has
to be configured to accept password authentication (which is
usually not the case for root), user have to type in their
password. Using SSH keys is more convenient. Therefore, two new
APIs are introduced:
virDomainAuthorizedSSHKeysGet() which lists authorized keys for
given user, and
virDomainAuthorizedSSHKeysSet() which modifies the authorized
keys file for given user (append, set or remove keys from the
file).
It's worth nothing that while authorized_keys file entries have
some structure (as defined by sshd(8)), expressing that structure
goes beyond libvirt's focus and thus "keys" are nothing but an
opaque string to libvirt.
Signed-off-by: Michal Privoznik
Reviewed-by: Peter Krempa
---
include/libvirt/libvirt-domain.h | 17
src/driver-hypervisor.h | 15
src/libvirt-domain.c | 133 +++
src/libvirt_public.syms | 6 ++
4 files changed, 171 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index e1095a193d..d81157ccaf 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5101,4 +5101,21 @@ int virDomainBackupBegin(virDomainPtr domain,
char *virDomainBackupGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+int virDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags);
+
+typedef enum {
+VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND = (1 << 0), /* don't truncate
file, just append */
+VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE = (1 << 1), /* remove keys,
instead of adding them */
+
+} virDomainAuthorizedSSHKeysSetFlags;
+
+int virDomainAuthorizedSSHKeysSet(virDomainPtr domain,
+ const char *user,
+ const char **keys,
+ int nkeys,
+ unsigned int flags);
+
#endif /* LIBVIRT_DOMAIN_H */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index bce023017d..5a5ea95c51 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1387,6 +1387,19 @@ typedef char *
(*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain,
unsigned int flags);
+typedef int
+(*virDrvDomainAuthorizedSSHKeysGet)(virDomainPtr domain,
+const char *user,
+char ***keys,
+unsigned int flags);
+
+typedef int
+(*virDrvDomainAuthorizedSSHKeysSet)(virDomainPtr domain,
+const char *user,
+const char **keys,
+int nkeys,
+unsigned int flags);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1650,4 +1663,6 @@ struct _virHypervisorDriver {
virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout;
virDrvDomainBackupBegin domainBackupBegin;
virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc;
+virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet;
+virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 3c5f55176a..63d4954e68 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12758,3 +12758,136 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,
virDispatchError(conn);
return NULL;
}
+
+
+/**
+ * virDomainAuthorizedSSHKeysGet:
+ * @domain: a domain object
+ * @user: user to list keys for
+ * @keys: pointer to a variable to store authorized keys
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * For given @user in @domain fetch list of public SSH authorized
+ * keys and store them into @keys array which is allocated upon
+ * successful return and is NULL terminated. The caller is
+ * responsible for freeing @keys when no longer needed.
+ *
+ * Keys are in OpenSSH format (see sshd(8)) but from libvirt's
+ * point of view are opaque strings, i.e. not interpreted.
+ *
+ * Please note that some hypervisors may require guest agent to
+ * be configured and running in order to be able to run this API.
+ *
+ * Returns: number of keys stored in @keys,
+ * -1 otherwise.
+ */
+int
+virDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+virConnectPtr conn;
+
+
[libvirt PATCH] virt-host-validate: fix detection with cgroups v2
Using virtCgroupNewSelf() is not correct with cgroups v2 because the
the virt-host-validate process is executed from from the same cgroup
context as the terminal and usually not all controllers are enabled
by default.
To do a proper check we need to use the root cgroup to see what
controllers are actually available. Libvirt or systemd ensures that
all controllers are available for VMs as well.
This still doesn't solve the devices controller with cgroups v2 where
there is no controller as it was replaced by eBPF. Currently libvirt
tries to query eBPF programs which usually works only for root as
regular users will get permission denied for that operation.
Fixes: https://gitlab.com/libvirt/libvirt/-/issues/94
Signed-off-by: Pavel Hrdina
---
src/libvirt_private.syms | 1 +
src/util/vircgroup.h | 4
src/util/vircgrouppriv.h | 4
tools/virt-host-validate-common.c | 2 +-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1d98f01334..79a23f34cb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1796,6 +1796,7 @@ virCgroupHasController;
virCgroupHasEmptyTasks;
virCgroupKillPainfully;
virCgroupKillRecursive;
+virCgroupNew;
virCgroupNewDetect;
virCgroupNewDetectMachine;
virCgroupNewDomainPartition;
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 78770f5d3b..f7eed983cc 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -60,6 +60,10 @@ typedef enum {
bool virCgroupAvailable(void);
+int virCgroupNew(const char *path,
+ int controllers,
+ virCgroupPtr *group);
+
int virCgroupNewSelf(virCgroupPtr *group)
ATTRIBUTE_NONNULL(1);
diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h
index baa84550f4..85ba5393e0 100644
--- a/src/util/vircgrouppriv.h
+++ b/src/util/vircgrouppriv.h
@@ -110,10 +110,6 @@ int virCgroupGetValueForBlkDev(const char *str,
const char *devPath,
char **value);
-int virCgroupNew(const char *path,
- int controllers,
- virCgroupPtr *group);
-
int virCgroupNewPartition(const char *path,
bool create,
int controllers,
diff --git a/tools/virt-host-validate-common.c
b/tools/virt-host-validate-common.c
index a10ac03293..fc43b2ddc8 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -293,7 +293,7 @@ int virHostValidateCGroupControllers(const char *hvname,
int ret = 0;
size_t i;
-if (virCgroupNewSelf() < 0)
+if (virCgroupNew("/", -1, ) < 0)
return -1;
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
--
2.26.2
Re: [PATCH] news: Mention Cooperlake cpu model in v6.4.0
On Fri, Nov 06, 2020 at 03:25:17PM +0100, Andrea Bolognani wrote: On Fri, 2020-11-06 at 15:10 +0100, Martin Kletzander wrote: On Wed, Oct 28, 2020 at 03:51:48PM +0800, Han Han wrote: > Signed-off-by: Han Han > --- > NEWS.rst | 2 ++ > 1 file changed, 2 insertions(+) So yes, this was added in 6.4.0, but I don't know whether we have any policy regarding news for older releases as that would create inconsistency and there is always going to be something we missed. Especially when lot of people do not update the news file. On the other hand it would appear in the news file from now on... I really don't know. Does anyone else have an opinion? @Dan? @Andrea? I think it can be useful to add this kind of information even after the fact, since we include the full release notes in release tarballs as well as publishing them on the website. There is basically no downside to this, even though of course it would be even better if we would manage to add this information *before* the release is out :) Reviewed-by: Andrea Bolognani For some reason I thought this was pushed already, but it wasn't. Anyway, I pushed it now. -- Andrea Bolognani / Red Hat / Virtualization signature.asc Description: PGP signature
Re: [libvirt PATCH 0/3] Add missing feature detection to sync tool in cpu_map
On Wed, 2020-11-18 at 12:19 +0100, Tim Wiederhake wrote: > sync_qemu_i386.py in src/cpu_map is a tool to sync CPU models from > qemu > with libvirt. It currently has no provisions for detecting new > features > that are not implemented in libvirt yet. This series changes that. > > See also > https://www.redhat.com/archives/libvir-list/2020-November/msg00271.html. > > libvirt is currently missing three x86 CPU models: Denverton, > KnightsMill, and Snowridge; and seven features: core-capability, > fsrm, > perfctr-core, split-lock-detect, vmx-eptp-switching, vmx-pml, and > vmx-rdseed-exit. > > For the Snowridge CPU model and the core-capability and split-lock- > detect > features, see also > https://www.redhat.com/archives/libvir-list/2020-November/msg00924.html. > > Tim Wiederhake (3): > cpu_map: sync_qemu_i386: Detect features missing in translation > table > cpu_map: sync_qemu_i386: Add features missing in translation table > cpu_map: sync_qemu_i386: Detect features missing in libvirt > > src/cpu_map/sync_qemu_i386.py | 28 > 1 file changed, 28 insertions(+) > > -- > 2.26.2 "Ohnosecond", Noun, /ˈoʊnoʊˌsɛkənd/: The fraction of time between making a mistake and realizing it. Disregard this series. There is a bug in the sync_qemu_i386.py script that needs adressing first. Will send out v2 later. Tim
Re: [libvirt][RFC PATCH] add a new 'default' option for attribute mode in numatune
On Wed, Nov 18, 2020 at 05:40:04PM +0800, Zhong, Luyao wrote:
On 11/12/2020 5:27 PM, Martin Kletzander wrote:
On Thu, Nov 12, 2020 at 02:19:06PM +0800, Zhong, Luyao wrote:
On 11/9/2020 7:21 PM, Martin Kletzander wrote:
On Sat, Nov 07, 2020 at 10:41:52AM +0800, Zhong, Luyao wrote:
On 11/4/2020 9:02 PM, Martin Kletzander wrote:
On Fri, Oct 16, 2020 at 10:38:51PM +0800, Zhong, Luyao wrote:
On 10/16/2020 9:32 PM, Zang, Rui wrote:
How about if “migratable” is set, “mode” should be ignored/omitted?
So any setting of “mode” will be rejected with an error
indicating an
invalid configuration.
We can say in the doc that “migratable” and “mode” shall not be set
together. So even the default value of “mode” is not taken.
If "mode" is not set, it's the same as setting "strict" value
('strict'
is the default value). It involves some code detail, it will be
translated to enumerated type, the value is 0 when mode not set or
set
to 'strict'. The code is in some fixed skeleton, so it's not easy to
modify.
Well I see it as it is "strict". It does not mean "strict cgroup
setting",
because cgroups are just one of the ways to enforce this. Look at it
this way:
mode can be:
- strict: only these nodes can be used for the memory
- preferred: there nodes should be preferred, but allocation should
not fail
- interleave: interleave the memory between these nodes
Due to the naming this maps to cgroup settings 1:1.
Sorry, I misspoke, this does not map to cgroup settings at all, in
cgroups you
can only set "strict" (by using cpuset.mems) and that's it. There is no
way to
set preferred or interleaved mapping, sorry.
memory policy is independent of cpuset.mems
Yes.
I quote here "Memory policies should not be confused with cpusets
(Documentation/admin-guide/cgroup-v1/cpusets.rst) which is an
administrative mechanism for restricting the nodes from which memory may
be allocated by a set of processes. Memory policies are a programming
interface that a NUMA-aware application can take advantage of.
Pay attention to this part:
When both cpusets and policies are applied to a task, the restrictions
of the
cpuset takes priority.
See Memory Policies and cpusets below for more details."[1]
So using cpuset.mems does not mean set "strict" memory policy if I
understand it correctly, we can set cpuset.mems with any memory policy.
That's not how I understand that. Sure, it's independent of memory
policy, but
if you do not specify memory policy (which keeps it as "default") and set
cpuset.mems, then the process will only be permitted to allocate memory
on NUMA
nodes specified in the file.
yes it's not conflict with what I was saying, it's one kind of combinations.
For instance, we can also set cpuset.mems to "1,2" and use mbind() set
memory policy to MPOL_PREFERRED and preferred node is "1", that means we
will allocate pages from the node 1 first then fall back to other
nodes(only node 2 under this case since cpuset.mems restrict the memory
nodes) if the preferred nodes is low on free memory. If the prefered
node is "0", we will not allocate pages from node 0 since cpuset.mems
takes priority.
Do you mean cpuset.mems + MPOL_DEFAULT policy == MPOL_BIND policy? They
might be functionally similar if not specific policies implemented in
kernel. But I don't think they are exactly the same.
It's not the same, but has similar outcome. If you do numa_set_membind() or
set_mempolicy(MPOL_BIND) on nodes 2-3, then the memory allocations will only be
done on those nodes. If there is no space on them the allocation will fail. If
you write 2-3 into cpuset.mems, then similar thing happens: allocations will
only be done on the mentioned nodes and will fail if there is not enough space.
If you are talking about mbind() in qemu, that is done *after* the allocation,
but *before* any write to that. If that is anonymous memory or a private
file-backed memory, then writes that allocate will be allocated on the specified
nodes. If you write to cpuset.mems *after* the allocations were done, then they
might be moved to those nodes based on cpuset.memory_migrate and couple of other
things.
Because default policy indicate that we are using policy implemented in
kernel(transparent to process), just like the memory
tiering(https://lwn.net/Articles/802544/) case I stated before. So
cpuset.mems + MPOL_DEFAULT policy is not MPOL_BIND policy under this case.
Yes, there are some other minor details between mpol_default and mpol_bind. Are
you referring to the fact that mpol_default prefers same node as the thread is
running on whereas mpol_bind allocates on the node with the lowest ID from the
set, no matter what node the thread requesting the allocation is running on,
etc.?
If yes, then sure, I completely agree that someone might want to use cpuset.mems
with mpol_default. That's what we're both trying to achieve here, right? I
think the misunderstanding here comes from the fact we both understand what
libvirt XML is
[PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask
The way our domain capabilities work currently, is that we have
virDomainCapsEnum struct which contains 'unsigned int values'
member which serves as a bitmask. More complicated structs are
composed from this struct, giving us whole virDomainCaps
eventually.
Whenever we want to report that a certain value is supported, the
'1 << value' bit is set in the corresponding unsigned int member.
This works as long as the resulting value after bitshift does not
overflow unsigned int. There is a check inside
virDomainCapsEnumSet() which ensures exactly this, but no caller
really checks whether virDomainCapsEnumSet() succeeded. Also,
checking at runtime is a bit too late.
Fortunately, we know the largest value we want to store in each
member, because each enum of ours ends with _LAST member.
Therefore, we can check at build time whether an overflow can
occur.
Signed-off-by: Michal Privoznik
---
src/conf/domain_capabilities.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index f177af1744..b22d40abb2 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -36,6 +36,9 @@ struct _virDomainCapsEnum {
unsigned int values; /* Bitmask of values supported in the corresponding
enum */
};
+#define STATIC_ASSERT_ENUM(last) \
+G_STATIC_ASSERT(last <= sizeof(unsigned int) * CHAR_BIT)
+
typedef struct _virDomainCapsStringValues virDomainCapsStringValues;
typedef virDomainCapsStringValues *virDomainCapsStringValuesPtr;
struct _virDomainCapsStringValues {
@@ -43,6 +46,8 @@ struct _virDomainCapsStringValues {
size_t nvalues; /* number of strings */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_LOADER_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_TRISTATE_BOOL_LAST);
typedef struct _virDomainCapsLoader virDomainCapsLoader;
typedef virDomainCapsLoader *virDomainCapsLoaderPtr;
struct _virDomainCapsLoader {
@@ -53,6 +58,7 @@ struct _virDomainCapsLoader {
virDomainCapsEnum secure; /* Info about secure:virTristateBool */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST);
typedef struct _virDomainCapsOS virDomainCapsOS;
typedef virDomainCapsOS *virDomainCapsOSPtr;
struct _virDomainCapsOS {
@@ -61,6 +67,9 @@ struct _virDomainCapsOS {
virDomainCapsLoader loader; /* Info about virDomainLoaderDef */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_DEVICE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_BUS_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_MODEL_LAST);
typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk;
typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr;
struct _virDomainCapsDeviceDisk {
@@ -71,6 +80,7 @@ struct _virDomainCapsDeviceDisk {
/* add new fields here */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_GRAPHICS_TYPE_LAST);
typedef struct _virDomainCapsDeviceGraphics virDomainCapsDeviceGraphics;
typedef virDomainCapsDeviceGraphics *virDomainCapsDeviceGraphicsPtr;
struct _virDomainCapsDeviceGraphics {
@@ -78,6 +88,7 @@ struct _virDomainCapsDeviceGraphics {
virDomainCapsEnum type; /* virDomainGraphicsType */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
typedef struct _virDomainCapsDeviceVideo virDomainCapsDeviceVideo;
typedef virDomainCapsDeviceVideo *virDomainCapsDeviceVideoPtr;
struct _virDomainCapsDeviceVideo {
@@ -85,6 +96,11 @@ struct _virDomainCapsDeviceVideo {
virDomainCapsEnum modelType; /* virDomainVideoType */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_MODE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_STARTUP_POLICY_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST);
typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
struct _virDomainCapsDeviceHostdev {
@@ -97,6 +113,8 @@ struct _virDomainCapsDeviceHostdev {
/* add new fields here */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_RNG_MODEL_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_RNG_BACKEND_LAST);
typedef struct _virDomainCapsDeviceRNG virDomainCapsDeviceRNG;
typedef virDomainCapsDeviceRNG *virDomainCapsDeviceRNGPtr;
struct _virDomainCapsDeviceRNG {
@@ -105,6 +123,7 @@ struct _virDomainCapsDeviceRNG {
virDomainCapsEnum backendModel; /* virDomainRNGBackend */
};
+STATIC_ASSERT_ENUM(VIR_GIC_VERSION_LAST);
typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC;
typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr;
struct _virDomainCapsFeatureGIC {
--
2.26.2
Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'
On 11/18/20 4:44 AM, Peter Krempa wrote: On Mon, Nov 16, 2020 at 20:43:09 +0100, Andrea Bolognani wrote: On Fri, 2020-11-13 at 16:23 -0300, Daniel Henrique Barboza wrote: On 11/13/20 10:51 AM, Andrea Bolognani wrote: I only skimmed the remaining patches and didn't dig into the code as [...] In general, changing guest ABI between boots is not something that we want to happen. I have trouble keeping all the details of memory alignment inside my head and I can't quite spend the time necessary to swap them back in right now, so please apologies if I'm being vague and of course correct me if I'm wrong... Having Peter in the thread will also surely help with that :) Either you have high hopes or my sarcasm detector is failing. Just as one data point: the 'PostParse' callback happen _always_. Even when migrating the VM. My patch according to the commit message is specifically avoiding the alignment on migration. Thus the code can't be moved to the post parse callback. Not without considering the PARSE_ABI_UPDATE flag, and even then I'm not sure if it's worth the trouble like I mentioned earlier. The outcome documented in the NEWS just mentions that it's to update the live XML. Neither of the commit messages of this series mentions that there is an issue with migration so the series needs to be re-evaluated in that light. This is correct. I got a lot of stuff wrong in the v1. v2 will be way shorter in scope but it will not mess up with existing migration/snapshot mechanics. My aim will be to fix the existing alignment calculation for pseries DIMMs. Thanks, DHB
[libvirt PATCH 3/3] cpu_map: sync_qemu_i386: Detect features missing in libvirt
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 10 ++
1 file changed, 10 insertions(+)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index 684fc96dc0..8844aa00cd 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -5,6 +5,7 @@ import copy
import lark
import os
import re
+import xml.etree.ElementTree
T = {
@@ -382,6 +383,15 @@ def main():
"Features not in the translation table:",
", ".join(sorted(untranslated)))
+filename = os.path.join(args.outdir, "x86_features.xml")
+DOMTree = xml.etree.ElementTree.parse(filename)
+known = [x.attrib["name"] for x in DOMTree.getroot().iter("feature")]
+unknown = [x for x in features if x not in known]
+if unknown:
+print(
+"Features not in libvirt:",
+", ".join(sorted(unknown)))
+
if __name__ == "__main__":
main()
--
2.26.2
[libvirt PATCH 2/3] cpu_map: sync_qemu_i386: Add features missing in translation table
Signed-off-by: Tim Wiederhake
---
src/cpu_map/sync_qemu_i386.py | 8
1 file changed, 8 insertions(+)
diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py
index fb4eea101b..684fc96dc0 100755
--- a/src/cpu_map/sync_qemu_i386.py
+++ b/src/cpu_map/sync_qemu_i386.py
@@ -142,6 +142,14 @@ T = {
"MSR_ARCH_CAP_TAA_NO": "taa-no",
"MSR_CORE_CAP_SPLIT_LOCK_DETECT": "split-lock-detect",
+# identically named features
+"avx512ifma": "avx512ifma",
+"fsrm": "fsrm",
+"perfctr-core": "perfctr-core",
+"vmx-eptp-switching": "vmx-eptp-switching",
+"vmx-pml": "vmx-pml",
+"vmx-rdseed-exit": "vmx-rdseed-exit",
+
# always disabled features
"CPUID_EXT_MONITOR": None,
"0": None,
--
2.26.2
[libvirt PATCH 0/3] Add missing feature detection to sync tool in cpu_map
sync_qemu_i386.py in src/cpu_map is a tool to sync CPU models from qemu with libvirt. It currently has no provisions for detecting new features that are not implemented in libvirt yet. This series changes that. See also https://www.redhat.com/archives/libvir-list/2020-November/msg00271.html. libvirt is currently missing three x86 CPU models: Denverton, KnightsMill, and Snowridge; and seven features: core-capability, fsrm, perfctr-core, split-lock-detect, vmx-eptp-switching, vmx-pml, and vmx-rdseed-exit. For the Snowridge CPU model and the core-capability and split-lock-detect features, see also https://www.redhat.com/archives/libvir-list/2020-November/msg00924.html. Tim Wiederhake (3): cpu_map: sync_qemu_i386: Detect features missing in translation table cpu_map: sync_qemu_i386: Add features missing in translation table cpu_map: sync_qemu_i386: Detect features missing in libvirt src/cpu_map/sync_qemu_i386.py | 28 1 file changed, 28 insertions(+) -- 2.26.2
[libvirt PATCH 1/3] cpu_map: sync_qemu_i386: Detect features missing in translation table
Signed-off-by: Tim Wiederhake --- src/cpu_map/sync_qemu_i386.py | 10 ++ 1 file changed, 10 insertions(+) diff --git a/src/cpu_map/sync_qemu_i386.py b/src/cpu_map/sync_qemu_i386.py index 8deda869df..fb4eea101b 100755 --- a/src/cpu_map/sync_qemu_i386.py +++ b/src/cpu_map/sync_qemu_i386.py @@ -364,6 +364,16 @@ def main(): with open(name, "wt") as f: output_model(f, model) +features = set() +for model in models: +features.update(model["features"]) + +untranslated = [x for x in features if x not in T.values()] +if untranslated: +print( +"Features not in the translation table:", +", ".join(sorted(untranslated))) + if __name__ == "__main__": main() -- 2.26.2
Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'
On 11/16/20 4:43 PM, Andrea Bolognani wrote: On Fri, 2020-11-13 at 16:23 -0300, Daniel Henrique Barboza wrote: On 11/13/20 10:51 AM, Andrea Bolognani wrote: I only skimmed the remaining patches and didn't dig into the code as much, or as recently, as you have, but from a high-level perspective I don't see why you wouldn't be able to simply move the existing rounding logic from the command line generator to PostParse? It's not like the former has access to additional information that the latter can't get to, right? I was looking into the code and I think that might have the wrong idea here. Apparently we're not aligning memory during migration or snapshot restore. This specific line in qemu_command.c got my attention: -- qemuBuildCommandLine() -- if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0) return NULL; -- I investigated the history behind this logic and found the following commit: commit c7d7ba85a6242d789ba3f4dae313e950fbb638c5 Author: Peter Krempa Date: Thu Sep 17 08:14:05 2015 +0200 qemu: command: Align memory sizes only on fresh starts When we are starting a qemu process for an incomming migration or snapshot reloading we should not modify the memory sizes in the domain since we could potentially change the guest ABI that was tediously checked before. Additionally the function now updates the initial memory size according to the NUMA node size, which should not happen if we are restoring state. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1252685 - This means that the changes made in this series will not break migration, since the alignment of 'initialmem' is not being triggered in those cases. Which is good. However, you also brought up in an earlier reply that these changes might break "the guest ABI across guest boots (if libvirt is upgraded in between them)". This can't be helped I think - an older ppc64 guest with 4GiB of 'currentMemory' in the XML, that is ending up having 4.256GiB (extra 256MiB) due to the way alignment was being done, will have exactly 4GiB of 'initialmem' after these changes. My point is that we're giving the exact memory the guest is demanding, as intended by the domain XML, in a fresh guest start. This might be considered an ABI break probably, but why would one complain that Libvirt is now giving precisely what was asked for? Avoiding granting extra 256MBs of mem for domains seems worth it, given that we're not impacting live domains or migration. In general, changing guest ABI between boots is not something that we want to happen. I have trouble keeping all the details of memory alignment inside my head and I can't quite spend the time necessary to swap them back in right now, so please apologies if I'm being vague and of course correct me if I'm wrong... Having Peter in the thread will also surely help with that :) The aim of this series should *not* be to change the calculations that happen when aligning memory, but only to reflect them back to the domain XML where they can be queried: so for example if the input 4 500 results in the command line -m 4352m -object memory-backend-file,size=512m (the exact numbers are not relevant), then what we want is for the XML to be updated at define time so that it reads I investigate it more and I think I got a few premises wrong here: - I pointed out that the alignment changes aren't being reflected. This is wrong. I figured it out when launching real guests and seeing that the live XML was being updated properly. This diminishes the urgency of this patch series considerably, and I apologize for the confusion. - The reason I claimed that is because I got misled by my own misunderstanding of what qemuxml2xmltest.c does: it checks the result XML only up to PostParse changes, and that's it. Any further change in the XML aren't being reflected by the tests. qemuxml2argvtest.c goes as far as building the command line, and I got the wrong idea that both tests reflected the same stages of a VM launch. For what I'm doing here, qemuxml2xml isn't helpful. All this out of the way: 4864 512 (again, the numbers are almost certainly wrong) and we want *that* XML to generate the same QEMU command line as before. If this can't be achieved, or there are other side effects to it that I haven't considered, then we're better off leaving the current behavior alone (documenting the heck out of it if necessary) instead of changing it in ways that would alter guest ABI between boots. This can be achieve, sort of. I can't just move the alignment to PostParse, even without changing how it is calculated, because I would break the premises it's based on today (not be executed on migration or snapshot). To do that I would need to gate the code into a VIR_DOMAIN_DEF_PARSE_ABI_UPDATE parse flag. But I'm not sure if this
Re: Libvirt NVME support
On Wed, Nov 18, 2020 at 09:57:14 +, Thanos Makatos wrote: > > As a separate question, is there any performance benefit of emulating a > > NVMe controller compared to e.g. virtio-scsi? > > We haven't measured that yet; I would expect it to be slight faster and/or > more > CPU efficient but wouldn't be surprised if it isn't. The main benefit of using > NVMe is that we don't have to install VirtIO drivers in the guest. Okay, I'm not sold on the drivers bit but that is definitely not a problem in regards of adding support for emulating NVME controllers to libvirt. As a starting point a trivial way to model this in the XML will be: And then add the storage into it as: The 'drive' address here maps the disk to the controller. This example uses unit= as the way to specify the namespace ID. Both 'bus' and 'target' must be 0. You can theoretically also add your own address type if 'drive' doesn't look right. This model will have problems with hotplug/unplug if the NVMe spec doesn't actually allow hotplug of a single namespace into a controller as libvirt's hotplug APIs only deal with one element at time. We theoretically could work this around by allowing hotplug of disks which correspond to the namespace used while the controller was not attached yet, and the attach of the controller then attaches both the backends and the controller. This is a bit hacky though. Another obvious solution is to disallow hotplug of the namespaces and thus also the controller.
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On Wed, Nov 18, 2020 at 10:38 AM Daniel P. Berrangé wrote: > > On Tue, Nov 17, 2020 at 09:11:48PM -0500, Neal Gompa wrote: > > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt > > wrote: > > > > > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik > > > wrote: > > > > > > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote: > > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a > > > > > wrapper > > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use > > > > > for quite a while anymore, but required to work for compatibility e.g. > > > > > when migrating in old guests. > > > > > > > > > > For years this was a symlink kvm-spice->kvm and therefore covered > > > > > apparmor-wise by the existing entry: > > > > > /usr/bin/kvm rmix, > > > > > But due to a recent change [1] in qemu packaging this now is no > > > > > symlink, > > > > > but a wrapper on its own and therefore needs an own entry that allows > > > > > it > > > > > to be executed. > > > > > > > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 > > > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > > --- > > > > > src/security/apparmor/libvirt-qemu | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > Reviewed-by: Michal Privoznik > > > > > > Thank you Michal, > > > it also passed fine through my tests (as backport to 6.8 and 6.9). > > > We are not in any freeze, review has happened, tests LGTM - pushed to git. > > > > > > > Hold up, why was this merged? Did anyone validate whether this would > > break the other AppArmor user (SUSE)? > > > > Unlike SELinux, AppArmor functionality is quite fragmented between > > Ubuntu and SUSE distributions (the two major users of AppArmor), and > > there did not seem to be any indication that this AppArmor patch was > > validated with openSUSE before merging. My personal experience with > > AppArmor across the two distribution families is that it's really easy > > to make profiles that work for Ubuntu but fail on SUSE because of the > > disparity of functionality. I also don't see Jim Fehlig stepping in to > > indicate that this worked for him. > > > > I haven't had a chance to test this myself, but I am immediately > > suspicious of a change that references a commit based on Debian > > packaging of QEMU. > > Historically the AppArmor policy in libvirt has been exclusively > maintained and tested by the Debian and Ubuntu maintainers. We have > never considered SUSE in any changes made to it. Ack to what Daniel wrote. In addition neither are other - be it Suse or 3rd party - changes gated on Debian/Ubuntu testing them. If I fail to catch the changes on the ML-discussion as part of staring at my inbox, then the testing for us happens whenever we merge a new upstream version. The general rule of thumb that we not-strictly followed in recent times aret: - logical changes e.g. to virt-aa-helper will have a build time self-test associated - labelling changes (related to hot add for example) are usually up for discussion a bit longer and tested by the author in the context that the issues were found - rule allow-additions (like the one here) are discussed and added if there are no security concerns I don't remember we've made anything more restrictive recently, that would probably be somewhere between the latter two points above. The duration also depends on the complexity - in this particular case as Michal already stated there isn't a high chance of breaking something with it. OTOH I'm fine to work out something more official/established if you want to define something - but keep in mind that many of us do this as a fraction of a part of their duties. Due to that sometimes human/machine time isn't available for short round trip times which are needed for a formal gating process. > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| > -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
RE: Libvirt NVME support
> As a separate question, is there any performance benefit of emulating a > NVMe controller compared to e.g. virtio-scsi? We haven't measured that yet; I would expect it to be slight faster and/or more CPU efficient but wouldn't be surprised if it isn't. The main benefit of using NVMe is that we don't have to install VirtIO drivers in the guest.
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On 11/18/20 3:11 AM, Neal Gompa wrote: On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt wrote: On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik wrote: On 11/16/20 1:26 PM, Christian Ehrhardt wrote: 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper around qemu-system-x86_64 enabling kvm acceleration. This isn't in use for quite a while anymore, but required to work for compatibility e.g. when migrating in old guests. For years this was a symlink kvm-spice->kvm and therefore covered apparmor-wise by the existing entry: /usr/bin/kvm rmix, But due to a recent change [1] in qemu packaging this now is no symlink, but a wrapper on its own and therefore needs an own entry that allows it to be executed. [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 Signed-off-by: Christian Ehrhardt --- src/security/apparmor/libvirt-qemu | 1 + 1 file changed, 1 insertion(+) Reviewed-by: Michal Privoznik Thank you Michal, it also passed fine through my tests (as backport to 6.8 and 6.9). We are not in any freeze, review has happened, tests LGTM - pushed to git. Hold up, why was this merged? Did anyone validate whether this would break the other AppArmor user (SUSE)? Unlike SELinux, AppArmor functionality is quite fragmented between Ubuntu and SUSE distributions (the two major users of AppArmor), and there did not seem to be any indication that this AppArmor patch was validated with openSUSE before merging. My personal experience with AppArmor across the two distribution families is that it's really easy to make profiles that work for Ubuntu but fail on SUSE because of the disparity of functionality. I also don't see Jim Fehlig stepping in to indicate that this worked for him. I haven't had a chance to test this myself, but I am immediately suspicious of a change that references a commit based on Debian packaging of QEMU. Maybe I'm misunderstanding something, but does this have a potential of breaking something? It's only allowing one binary more that can be executed. Michal
Re: [libvirt][RFC PATCH] add a new 'default' option for attribute mode in numatune
On 11/12/2020 5:27 PM, Martin Kletzander wrote:
On Thu, Nov 12, 2020 at 02:19:06PM +0800, Zhong, Luyao wrote:
On 11/9/2020 7:21 PM, Martin Kletzander wrote:
On Sat, Nov 07, 2020 at 10:41:52AM +0800, Zhong, Luyao wrote:
On 11/4/2020 9:02 PM, Martin Kletzander wrote:
On Fri, Oct 16, 2020 at 10:38:51PM +0800, Zhong, Luyao wrote:
On 10/16/2020 9:32 PM, Zang, Rui wrote:
How about if “migratable” is set, “mode” should be ignored/omitted?
So any setting of “mode” will be rejected with an error
indicating an
invalid configuration.
We can say in the doc that “migratable” and “mode” shall not be set
together. So even the default value of “mode” is not taken.
If "mode" is not set, it's the same as setting "strict" value
('strict'
is the default value). It involves some code detail, it will be
translated to enumerated type, the value is 0 when mode not set or
set
to 'strict'. The code is in some fixed skeleton, so it's not easy to
modify.
Well I see it as it is "strict". It does not mean "strict cgroup
setting",
because cgroups are just one of the ways to enforce this. Look at it
this way:
mode can be:
- strict: only these nodes can be used for the memory
- preferred: there nodes should be preferred, but allocation should
not fail
- interleave: interleave the memory between these nodes
Due to the naming this maps to cgroup settings 1:1.
Sorry, I misspoke, this does not map to cgroup settings at all, in
cgroups you
can only set "strict" (by using cpuset.mems) and that's it. There is no
way to
set preferred or interleaved mapping, sorry.
memory policy is independent of cpuset.mems
Yes.
I quote here "Memory policies should not be confused with cpusets
(Documentation/admin-guide/cgroup-v1/cpusets.rst) which is an
administrative mechanism for restricting the nodes from which memory may
be allocated by a set of processes. Memory policies are a programming
interface that a NUMA-aware application can take advantage of.
Pay attention to this part:
When both cpusets and policies are applied to a task, the restrictions
of the
cpuset takes priority.
See Memory Policies and cpusets below for more details."[1]
So using cpuset.mems does not mean set "strict" memory policy if I
understand it correctly, we can set cpuset.mems with any memory policy.
That's not how I understand that. Sure, it's independent of memory
policy, but
if you do not specify memory policy (which keeps it as "default") and set
cpuset.mems, then the process will only be permitted to allocate memory
on NUMA
nodes specified in the file.
yes it's not conflict with what I was saying, it's one kind of combinations.
For instance, we can also set cpuset.mems to "1,2" and use mbind() set
memory policy to MPOL_PREFERRED and preferred node is "1", that means we
will allocate pages from the node 1 first then fall back to other
nodes(only node 2 under this case since cpuset.mems restrict the memory
nodes) if the preferred nodes is low on free memory. If the prefered
node is "0", we will not allocate pages from node 0 since cpuset.mems
takes priority.
Do you mean cpuset.mems + MPOL_DEFAULT policy == MPOL_BIND policy? They
might be functionally similar if not specific policies implemented in
kernel. But I don't think they are exactly the same.
Because default policy indicate that we are using policy implemented in
kernel(transparent to process), just like the memory
tiering(https://lwn.net/Articles/802544/) case I stated before. So
cpuset.mems + MPOL_DEFAULT policy is not MPOL_BIND policy under this case.
Regards,
Luyao
[1]https://www.infradead.org/~mchehab/kernel_docs/admin-guide/mm/numa_memory_policy.html
But now we have another way of enforcing this, using qemu cmdline
option. The
names actually map 1:1 to those as well:
https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/machine.json#L901
So my idea was that we would add a movable/migratable/whatever
attribute
that
would tell us which way for enforcing we use because there does not
seem
to be
"one size fits all" solution. Am I misunderstanding this discussion?
Please
correct me if I am. Thank you.
Actually I need a default memory policy(memory policy is 'hard coded'
into the kernel) support, I thought "migratable" was enough to indicate
So I am getting on your track, yes. What you mean is basically
MPOL_DEFAULT and
that's where the naming probably comes from, right? Anyway, what we're
trying
to do is not restrict us from other options, even if they are only
possible in
the future. So instead of adding "default" which would actually mean
"strict"
(because you still use cpuset.mems) which would restrict us from
potentially
being able to migrate with a different policy than "strict" (even
though it
might not make sense for "preferred", for example) and it's also a bit
confusing
as I mentioned above, using "cpuset.mems" does not mean "strict" memory
policy.
for users, I suggested we add "migratable" which
[PATCH for-5.2] docs: Fix some typos (found by codespell)
Fix also a similar typo in a code comment. Signed-off-by: Stefan Weil --- docs/can.txt | 8 docs/interop/vhost-user.rst | 2 +- docs/replay.txt | 2 +- docs/specs/ppc-spapr-numa.rst | 2 +- docs/system/deprecated.rst| 4 ++-- docs/tools/virtiofsd.rst | 2 +- hw/vfio/igd.c | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/can.txt b/docs/can.txt index 5838f6620c..0d310237df 100644 --- a/docs/can.txt +++ b/docs/can.txt @@ -19,7 +19,7 @@ interface to implement because such device can be easily connected to systems with different CPU architectures (x86, PowerPC, Arm, etc.). In 2020, CTU CAN FD controller model has been added as part -of the bachelor theses of Jan Charvat. This controller is complete +of the bachelor thesis of Jan Charvat. This controller is complete open-source/design/hardware solution. The core designer of the project is Ondrej Ille, the financial support has been provided by CTU, and more companies including Volkswagen subsidiaries. @@ -31,7 +31,7 @@ testing lead to goal change to provide environment which provides complete emulated environment for testing and RTEMS GSoC slot has been donated to work on CAN hardware emulation on QEMU. -Examples how to use CAN emulation for SJA1000 based borads +Examples how to use CAN emulation for SJA1000 based boards == When QEMU with CAN PCI support is compiled then one of the next @@ -106,8 +106,8 @@ This open-source core provides CAN FD support. CAN FD drames are delivered even to the host systems when SocketCAN interface is found CAN FD capable. -The PCIe borad emulation is provided for now (the device identifier is -ctucan_pci). The defauld build defines two CTU CAN FD cores +The PCIe board emulation is provided for now (the device identifier is +ctucan_pci). The default build defines two CTU CAN FD cores on the board. Example how to connect the canbus0-bus (virtual wire) to the host diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 988f154144..72b2e8c7ba 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -513,7 +513,7 @@ descriptor table (split virtqueue) or descriptor ring (packed virtqueue). However, it can't work when we process descriptors out-of-order because some entries which store the information of inflight descriptors in available ring (split virtqueue) or descriptor -ring (packed virtqueue) might be overrided by new entries. To solve +ring (packed virtqueue) might be overridden by new entries. To solve this problem, slave need to allocate an extra buffer to store this information of inflight descriptors and share it with master for persistent. ``VHOST_USER_GET_INFLIGHT_FD`` and diff --git a/docs/replay.txt b/docs/replay.txt index 87a64ae068..5b008ca491 100644 --- a/docs/replay.txt +++ b/docs/replay.txt @@ -328,7 +328,7 @@ between the snapshots. Each of the passes include the following steps: 1. loading the snapshot 2. replaying to examine the breakpoints 3. if breakpoint or watchpoint was met -- loading the snaphot again +- loading the snapshot again - replaying to the required breakpoint 4. else - proceeding to the p.1 with the earlier snapshot diff --git a/docs/specs/ppc-spapr-numa.rst b/docs/specs/ppc-spapr-numa.rst index 5fca2bdd8e..ffa687dc89 100644 --- a/docs/specs/ppc-spapr-numa.rst +++ b/docs/specs/ppc-spapr-numa.rst @@ -198,7 +198,7 @@ This is how it is being done: * user distance 121 and beyond will be interpreted as 160 * user distance 10 stays 10 -The reasoning behind this aproximation is to avoid any round up to the local +The reasoning behind this approximation is to avoid any round up to the local distance (10), keeping it exclusive to the 4th NUMA level (which is still exclusive to the node_id). All other ranges were chosen under the developer discretion of what would be (somewhat) sensible considering the user input. diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 32a0e620db..63e9db1463 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -465,7 +465,7 @@ default configuration. The CPU model runnability guarantee won't apply anymore to existing CPU models. Management software that needs runnability -guarantees must resolve the CPU model aliases using te +guarantees must resolve the CPU model aliases using the ``alias-of`` field returned by the ``query-cpu-definitions`` QMP command. @@ -637,7 +637,7 @@ Splitting RAM by default between NUMA nodes had the same issues as ``mem`` parameter with the difference that the role of the user plays QEMU using implicit generic or board specific splitting rule. Use ``memdev`` with *memory-backend-ram* backend or ``mem`` (if -it's supported by used machine type) to define mapping explictly instead. +it's supported by used machine type) to define mapping
Re: [libvirt PATCH 05/16] docs: add manpage for virtbhyved
On Tue, Nov 17, 2020 at 03:01:56PM -0500, Ryan Moeller wrote:
> On Tue, Nov 17, 2020 at 12:03 PM Daniel P. Berrangé
> wrote:
> >
> > This is an adaptation of the libvirtd manpage.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > docs/manpages/index.rst | 7 ++
> > docs/manpages/meson.build| 1 +
> > docs/manpages/virtbhyved.rst | 215 +++
> > 3 files changed, 223 insertions(+)
> > create mode 100644 docs/manpages/virtbhyved.rst
> >
> > diff --git a/docs/manpages/index.rst b/docs/manpages/index.rst
> > index 6a2a1e065d..da835d62ec 100644
> > --- a/docs/manpages/index.rst
> > +++ b/docs/manpages/index.rst
> > @@ -12,6 +12,13 @@ These daemons provide functionality across multiple
> > libvirt drivers
> > * `virtlogd(8) `__ - libvirt log management daemon
> > * `virtproxyd(8) `__ - libvirt proxy daemon
> >
> > +Modular Driver daemons
> > +==
> > +
> > +These daemons provide functionality to a single libvirt driver
> > +
> > +* `virtbhyved(8) `__ - libvirt bhyve management daemon
> > +
> > Tools
> > =
> >
> > diff --git a/docs/manpages/meson.build b/docs/manpages/meson.build
> > index 7d5a81ecd5..7c03cb74cf 100644
> > --- a/docs/manpages/meson.build
> > +++ b/docs/manpages/meson.build
> > @@ -22,6 +22,7 @@ docs_man_files = [
> >
> >{ 'name': 'libvirtd', 'section': '8', 'install':
> > conf.has('WITH_LIBVIRTD') },
> >{ 'name': 'virt-sanlock-cleanup', 'section': '8', 'install':
> > conf.has('WITH_SANLOCK') },
> > + { 'name': 'virtbhyved', 'section': '8', 'install':
> > conf.has('WITH_BHYVE') },
> >{ 'name': 'virtlockd', 'section': '8', 'install':
> > conf.has('WITH_LIBVIRTD') },
> >{ 'name': 'virtlogd', 'section': '8', 'install':
> > conf.has('WITH_LIBVIRTD') },
> >{ 'name': 'virtproxyd', 'section': '8', 'install':
> > conf.has('WITH_LIBVIRTD') },
> > diff --git a/docs/manpages/virtbhyved.rst b/docs/manpages/virtbhyved.rst
> > new file mode 100644
> > index 00..4d1d36b161
> > --- /dev/null
> > +++ b/docs/manpages/virtbhyved.rst
> > @@ -0,0 +1,215 @@
> > +==
> > +virtbhyved
> > +==
> > +
> > +---
> > +libvirt bhyve management daemon
> > +---
> > +
> > +:Manual section: 8
> > +:Manual group: Virtualization Support
> > +
> > +.. contents::
> > +
> > +SYNOPSIS
> > +
> > +
> > +``virtbhyved`` [*OPTION*]...
> > +
> > +
> > +DESCRIPTION
> > +===
> > +
> > +The ``virtbhyved`` program is a server side daemon component of the libvirt
> > +virtualization management system.
> > +
> > +It is one of a collection of modular daemons that replace functionality
> > +previously provided by the monolithic ``libvirtd`` daemon.
> > +
> > +This daemon runs on virtualization hosts to provide management for bhyve
> > virtual
> > +machines.
> > +
> > +The ``virtbhyved`` daemon only listens for requests on a local Unix domain
> > +socket. Remote off-host access and backwards compatibility with legacy
> > +clients expecting ``libvirtd`` is provided by the ``virtproxy`` daemon.
> > +
> > +Restarting ``virtbhyved`` does not interrupt running guests. Guests
> > continue to
> > +operate and changes in their state will generally be picked up
> > automatically
> > +during startup. None the less it is recommended to avoid restarting with
> > +running guests whenever practical.
> > +
> > +
> > +SYSTEM SOCKET ACTIVATION
> > +
> > +
> > +The ``virtbhyved`` daemon is capable of starting in two modes.
> > +
> > +In the traditional mode, it will create and listen on UNIX sockets itself.
> > +
> > +In socket activation mode, it will rely on systemd to create and listen
> > +on the UNIX sockets and pass them as pre-opened file descriptors. In this
> > +mode most of the socket related config options in
> > +``/etc/libvirt/virtbhyved.conf`` will no longer have any effect.
> > +
> > +Socket activation mode is generally the default when running on a host
> > +OS that uses systemd. To revert to the traditional mode, all the socket
> > +unit files must be masked:
> > +
> > +::
> > +
> > + $ systemctl mask virtbhyved.socket virtbhyved-ro.socket \
> > + virtbhyved-admin.socket
> > +
>
> I don't think any OS that supports bhyve has systemd.
Heh true, this is a good example of why I didn't try to generate the
manpages from a single common template. I'll cull this stuff out.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] apparmor: allow kvm-spice compat wrapper
On Tue, Nov 17, 2020 at 09:11:48PM -0500, Neal Gompa wrote: > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt > wrote: > > > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik > > wrote: > > > > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote: > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a > > > > wrapper > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use > > > > for quite a while anymore, but required to work for compatibility e.g. > > > > when migrating in old guests. > > > > > > > > For years this was a symlink kvm-spice->kvm and therefore covered > > > > apparmor-wise by the existing entry: > > > > /usr/bin/kvm rmix, > > > > But due to a recent change [1] in qemu packaging this now is no symlink, > > > > but a wrapper on its own and therefore needs an own entry that allows it > > > > to be executed. > > > > > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3 > > > > > > > > Signed-off-by: Christian Ehrhardt > > > > --- > > > > src/security/apparmor/libvirt-qemu | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > Reviewed-by: Michal Privoznik > > > > Thank you Michal, > > it also passed fine through my tests (as backport to 6.8 and 6.9). > > We are not in any freeze, review has happened, tests LGTM - pushed to git. > > > > Hold up, why was this merged? Did anyone validate whether this would > break the other AppArmor user (SUSE)? > > Unlike SELinux, AppArmor functionality is quite fragmented between > Ubuntu and SUSE distributions (the two major users of AppArmor), and > there did not seem to be any indication that this AppArmor patch was > validated with openSUSE before merging. My personal experience with > AppArmor across the two distribution families is that it's really easy > to make profiles that work for Ubuntu but fail on SUSE because of the > disparity of functionality. I also don't see Jim Fehlig stepping in to > indicate that this worked for him. > > I haven't had a chance to test this myself, but I am immediately > suspicious of a change that references a commit based on Debian > packaging of QEMU. Historically the AppArmor policy in libvirt has been exclusively maintained and tested by the Debian and Ubuntu maintainers. We have never considered SUSE in any changes made to it. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] add phytium FT-2000+ and Tengyun-S2500 support on arm architecture.
On Wed, 2020-11-18 at 09:06 +0800, yangshaoju...@163.com wrote: > From: Shaojun Yang > > Signed-off-by: Shaojun Yang > --- > src/cpu_map/arm_Phytium.xml | 10 ++ > src/cpu_map/arm_vendors.xml | 1 + > src/cpu_map/index.xml | 3 +++ > 3 files changed, 14 insertions(+) > create mode 100644 src/cpu_map/arm_Phytium.xml You only addressed a small part of my feedback. Please read https://www.redhat.com/archives/libvir-list/2020-November/msg00946.html again and address *all* of the points I brought up. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
On Wed, Nov 18, 2020 at 09:41:44 +0100, Michal Privoznik wrote:
> On 11/18/20 9:32 AM, Peter Krempa wrote:
> > On Wed, Nov 18, 2020 at 09:12:26 +0100, Michal Privoznik wrote:
> > > On 11/18/20 8:59 AM, Peter Krempa wrote:
> > > > On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote:
> > > > > Similarly to previous commits, we can utilize domCaps to check if
> > > > > graphics type is supported.
> > > > >
> > > > > Signed-off-by: Michal Privoznik
> > > > > ---
> > > > >src/qemu/qemu_capabilities.c | 2 +-
> > > > >src/qemu/qemu_capabilities.h | 3 +++
> > > > >src/qemu/qemu_validate.c | 40
> > > > >
> > > > >3 files changed, 17 insertions(+), 28 deletions(-)
> > > >
> > > > [...]
> > > >
> > > > > @@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const
> > > > > virDomainGraphicsDef *graphics,
> > > > >}
> > > > >break;
> > > > > +
> > > > > +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> > > > >case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> > > > >case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> > > > > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > > > - _("unsupported graphics type '%s'"),
> > > > > -
> > > > > virDomainGraphicsTypeToString(graphics->type));
> > > > > -return -1;
> > > > >case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> > > > > -default:
> > > > > -return -1;
> > > > > +break;
> > > >
> > > > Removing 'default: ' is not necessary once you use proper type for the
> > > > variable in the switch statement, which is our usual approach.
> > >
> > > I guess I don't need to typecast ->type since it's already stored as
> > > virDomainGraphicsType in the struct.
> > >
> > > >
> > > > The default and _LAST case should use virReportEnumRangeError.
> > > >
> > > >
> > >
> > > I'm not sure it's adding useful code. In this very patch I'm adding a
> > > check
> > > that rejects unknown values for ->type and thus I don't see a way how the
> > > control could hit any of these 'case', or 'default'.
> >
> > You are right it won't .
> >
> > A drawback of using the capability code is that
> > it relies on converting the type to a bit array stored in "unsigned int"
> > without any overflow safeguards. Once a device model or type enum
> > reaches 33 entries, the code will start failing without any obvious
> > sign.
> >
> > virDomainCapsEnumSet which is internally used by the
> > VIR_DOMAIN_CAPS_ENUM_SET
> > which is used by virQEMUCapsFillDomainDeviceGraphicsCaps to fill the
> > bitmap catches overflow but the callers neglect to propagate it.
> >
>
> Alright, so how about I post a follow up patch that checks for retval of
> virDomainCapsEnumSet() and merge this patch as is? Would that work for you?
Yes, merge these as they are.
Regarding the fix for the domain caps stuff, I prefer if you add compile
time checks that each declaration of virDomainCapsEnum guards that the
corresponding enum's _LAST value is less than 32. It's not really a
runtime problem, nor anything user can fix in their configuration.
Re: [PATCH] virsh: Added attach-disk support for network disk
On Mon, Nov 16, 2020 at 18:03:24 -0600, Ryan Gahagan wrote:
I'd like to reiterate that you should avoid top-posts on technical
lists. Even if you copy the context into the top post. Please always
respond inline.
> > > +
> > > /* Make XML of disk */
> > > virBufferAsprintf(, " > >isFile ? "file" : "block");
> >
> > I didn't notice this previously. You obviously need " > if defining a network disk.
>
> We're confused on what the exact semantics of the disk type we should use
> is, particularly if the driver or stype field has unexpected values.. We've
> drafted up some example code to show our interpretation of when "network"
> should be used over "file" or "block", but it is by no means final and we
> want your opinion on it. Here's what we've got:
>
> typedef enum {
> VIRSH_SOURCE_TYPE_FILE,
> VIRSH_SOURCE_TYPE_BLOCK,
> VIRSH_SOURCE_TYPE_NETWORK, VIRSH_SOURCE_TYPE_UNKNOWN,
> } virshAttachDiskSourceType;
>
> ...
> virshAttachDiskSourceType disk_type = VIRSH_SOURCE_TYPE_UNKNOWN;
> if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap")))
> { /* Disk type declared to be file */ disk_type = VIRSH_SOURCE_TYPE_FILE; }
> else { if (source && !stat(source, )) { /* Found a file at path source,
> test file type */ if (S_ISREG(st.st_mode)) disk_type =
> VIRSH_SOURCE_TYPE_FILE; else if (S_ISBLK(st.st_mode)) disk_type =
> VIRSH_SOURCE_TYPE_BLOCK; else disk_type = VIRSH_SOURCE_TYPE_NETWORK; } else
> { /* Either file not found or not specified, default network */ disk_type =
> VIRSH_SOURCE_TYPE_NETWORK; } } } else if (STREQ(stype, "file")) { disk_type
> = VIRSH_SOURCE_TYPE_FILE; } else if (STREQ(stype, "block")) { disk_type =
> VIRSH_SOURCE_TYPE_BLOCK; } else if (STREQ(stype, "network")) { disk_type =
> VIRSH_SOURCE_TYPE_NETWORK; } else { vshError(ctl, _("Unknown source type:
> '%s'"), stype); goto cleanup; }
This got horribly mangled by your client. I suggest you use plain-text
mode for maling list posts. Let me reformat it for now, but next time
please make sure it's readable as it takes a lot of effort.
> virshAttachDiskSourceType disk_type = VIRSH_SOURCE_TYPE_UNKNOWN;
> if (!stype) {
> if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) {
> /* Disk type declared to be file */
> disk_type = VIRSH_SOURCE_TYPE_FILE;
> } else {
> if (source && !stat(source, )) {
> /* Found a file at path source, test file type */
> if (S_ISREG(st.st_mode))
> disk_type = VIRSH_SOURCE_TYPE_FILE;
> else if (S_ISBLK(st.st_mode))
> disk_type = VIRSH_SOURCE_TYPE_BLOCK;
> else
>disk_type = VIRSH_SOURCE_TYPE_NETWORK;
So this is very wrong. This should not be based on stat(). Firstly, virsh
may not run on the host the VM is running on (remote connection). That
is a bug in the current code too. As noted I'll fix it after your patch
as I want to reduce the number of backs-and-forths with this patch.
You can simply assume that the user want's a network disk when the
protocol is specified using the flag you've added. Since that is
mandatory for a network disk it removes any kind of guessing.
> } else {
>/* Either file not found or not specified, default network */
>disk_type = VIRSH_SOURCE_TYPE_NETWORK;
>}
> }
> } else if (STREQ(stype, "file")) {
> disk_type = VIRSH_SOURCE_TYPE_FILE;
> } else if (STREQ(stype, "block")) {
>disk_type = VIRSH_SOURCE_TYPE_BLOCK;
> } else if (STREQ(stype, "network")) {
>disk_type = VIRSH_SOURCE_TYPE_NETWORK;
You also want to check that the protocol is specified when the stype is
explicitly set to network.
> } else {
> vshError(ctl, _("Unknown source type: '%s'"), stype);
>goto cleanup;
> }
[...]
> Let us know if this looks right or what you think should be used instead.
>
> > > +/* Using a local disk; source is file or dev */
> > > +virBufferAsprintf(, " %s='%s'",
> > +isFile ? "file" : "dev", source);
> >
> > This is still misaligned.
> >
> > > +virBufferAddLit(, "/>\n");
> > > +}
> > > +}
> > > +
> > > virBufferAsprintf(, " > > if (targetbus)
> > > virBufferAsprintf(, " bus='%s'", targetbus);
> >
> > Unfortunately this function is very old and would need to be refactored,
> > the XML formatting is definitely not to our modern standards.
>
> We're not entirely sure what you mean by this. What part of the code looks
No. The old code is wrong and uses old formatter functions. I don't want
to refactor it before we get over this patch as you'd have to change it
significantly.
> misaligned, and what function would need to be refactored? Your example
> output at least looks aligned properly:
Yes, the XML is aligned properly. I meant that the C code you moved in
[3] is not aligned properly. See below as your quote doesn't include
that bit.
> > $
Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
On 11/18/20 9:32 AM, Peter Krempa wrote:
On Wed, Nov 18, 2020 at 09:12:26 +0100, Michal Privoznik wrote:
On 11/18/20 8:59 AM, Peter Krempa wrote:
On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote:
Similarly to previous commits, we can utilize domCaps to check if
graphics type is supported.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_capabilities.h | 3 +++
src/qemu/qemu_validate.c | 40
3 files changed, 17 insertions(+), 28 deletions(-)
[...]
@@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const
virDomainGraphicsDef *graphics,
}
break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unsupported graphics type '%s'"),
- virDomainGraphicsTypeToString(graphics->type));
-return -1;
case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
-default:
-return -1;
+break;
Removing 'default: ' is not necessary once you use proper type for the
variable in the switch statement, which is our usual approach.
I guess I don't need to typecast ->type since it's already stored as
virDomainGraphicsType in the struct.
The default and _LAST case should use virReportEnumRangeError.
I'm not sure it's adding useful code. In this very patch I'm adding a check
that rejects unknown values for ->type and thus I don't see a way how the
control could hit any of these 'case', or 'default'.
You are right it won't .
A drawback of using the capability code is that
it relies on converting the type to a bit array stored in "unsigned int"
without any overflow safeguards. Once a device model or type enum
reaches 33 entries, the code will start failing without any obvious
sign.
virDomainCapsEnumSet which is internally used by the VIR_DOMAIN_CAPS_ENUM_SET
which is used by virQEMUCapsFillDomainDeviceGraphicsCaps to fill the
bitmap catches overflow but the callers neglect to propagate it.
Alright, so how about I post a follow up patch that checks for retval of
virDomainCapsEnumSet() and merge this patch as is? Would that work for you?
Michal
Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
On Wed, Nov 18, 2020 at 09:12:26 +0100, Michal Privoznik wrote:
> On 11/18/20 8:59 AM, Peter Krempa wrote:
> > On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote:
> > > Similarly to previous commits, we can utilize domCaps to check if
> > > graphics type is supported.
> > >
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/qemu/qemu_capabilities.c | 2 +-
> > > src/qemu/qemu_capabilities.h | 3 +++
> > > src/qemu/qemu_validate.c | 40
> > > 3 files changed, 17 insertions(+), 28 deletions(-)
> >
> > [...]
> >
> > > @@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const
> > > virDomainGraphicsDef *graphics,
> > > }
> > > break;
> > > +
> > > +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> > > case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> > > case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> > > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > - _("unsupported graphics type '%s'"),
> > > - virDomainGraphicsTypeToString(graphics->type));
> > > -return -1;
> > > case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> > > -default:
> > > -return -1;
> > > +break;
> >
> > Removing 'default: ' is not necessary once you use proper type for the
> > variable in the switch statement, which is our usual approach.
>
> I guess I don't need to typecast ->type since it's already stored as
> virDomainGraphicsType in the struct.
>
> >
> > The default and _LAST case should use virReportEnumRangeError.
> >
> >
>
> I'm not sure it's adding useful code. In this very patch I'm adding a check
> that rejects unknown values for ->type and thus I don't see a way how the
> control could hit any of these 'case', or 'default'.
You are right it won't .
A drawback of using the capability code is that
it relies on converting the type to a bit array stored in "unsigned int"
without any overflow safeguards. Once a device model or type enum
reaches 33 entries, the code will start failing without any obvious
sign.
virDomainCapsEnumSet which is internally used by the VIR_DOMAIN_CAPS_ENUM_SET
which is used by virQEMUCapsFillDomainDeviceGraphicsCaps to fill the
bitmap catches overflow but the callers neglect to propagate it.
Re: [PATCH for-5.2] docs: Fix some typos (found by codespell)
On 17/11/20 20:34, Stefan Weil wrote: Fix also a similar typo in a code comment. Signed-off-by: Stefan Weil --- docs/can.txt | 8 docs/interop/vhost-user.rst | 2 +- docs/replay.txt | 2 +- docs/specs/ppc-spapr-numa.rst | 2 +- docs/system/deprecated.rst| 4 ++-- docs/tools/virtiofsd.rst | 2 +- hw/vfio/igd.c | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/can.txt b/docs/can.txt index 5838f6620c..0d310237df 100644 --- a/docs/can.txt +++ b/docs/can.txt @@ -19,7 +19,7 @@ interface to implement because such device can be easily connected to systems with different CPU architectures (x86, PowerPC, Arm, etc.). In 2020, CTU CAN FD controller model has been added as part -of the bachelor theses of Jan Charvat. This controller is complete +of the bachelor thesis of Jan Charvat. This controller is complete open-source/design/hardware solution. The core designer of the project is Ondrej Ille, the financial support has been provided by CTU, and more companies including Volkswagen subsidiaries. @@ -31,7 +31,7 @@ testing lead to goal change to provide environment which provides complete emulated environment for testing and RTEMS GSoC slot has been donated to work on CAN hardware emulation on QEMU. -Examples how to use CAN emulation for SJA1000 based borads +Examples how to use CAN emulation for SJA1000 based boards == When QEMU with CAN PCI support is compiled then one of the next @@ -106,8 +106,8 @@ This open-source core provides CAN FD support. CAN FD drames are delivered even to the host systems when SocketCAN interface is found CAN FD capable. -The PCIe borad emulation is provided for now (the device identifier is -ctucan_pci). The defauld build defines two CTU CAN FD cores +The PCIe board emulation is provided for now (the device identifier is +ctucan_pci). The default build defines two CTU CAN FD cores on the board. Example how to connect the canbus0-bus (virtual wire) to the host diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 988f154144..72b2e8c7ba 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -513,7 +513,7 @@ descriptor table (split virtqueue) or descriptor ring (packed virtqueue). However, it can't work when we process descriptors out-of-order because some entries which store the information of inflight descriptors in available ring (split virtqueue) or descriptor -ring (packed virtqueue) might be overrided by new entries. To solve +ring (packed virtqueue) might be overridden by new entries. To solve this problem, slave need to allocate an extra buffer to store this information of inflight descriptors and share it with master for persistent. ``VHOST_USER_GET_INFLIGHT_FD`` and diff --git a/docs/replay.txt b/docs/replay.txt index 87a64ae068..5b008ca491 100644 --- a/docs/replay.txt +++ b/docs/replay.txt @@ -328,7 +328,7 @@ between the snapshots. Each of the passes include the following steps: 1. loading the snapshot 2. replaying to examine the breakpoints 3. if breakpoint or watchpoint was met -- loading the snaphot again +- loading the snapshot again - replaying to the required breakpoint 4. else - proceeding to the p.1 with the earlier snapshot diff --git a/docs/specs/ppc-spapr-numa.rst b/docs/specs/ppc-spapr-numa.rst index 5fca2bdd8e..ffa687dc89 100644 --- a/docs/specs/ppc-spapr-numa.rst +++ b/docs/specs/ppc-spapr-numa.rst @@ -198,7 +198,7 @@ This is how it is being done: * user distance 121 and beyond will be interpreted as 160 * user distance 10 stays 10 -The reasoning behind this aproximation is to avoid any round up to the local +The reasoning behind this approximation is to avoid any round up to the local distance (10), keeping it exclusive to the 4th NUMA level (which is still exclusive to the node_id). All other ranges were chosen under the developer discretion of what would be (somewhat) sensible considering the user input. diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 32a0e620db..63e9db1463 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -465,7 +465,7 @@ default configuration. The CPU model runnability guarantee won't apply anymore to existing CPU models. Management software that needs runnability -guarantees must resolve the CPU model aliases using te +guarantees must resolve the CPU model aliases using the ``alias-of`` field returned by the ``query-cpu-definitions`` QMP command. @@ -637,7 +637,7 @@ Splitting RAM by default between NUMA nodes had the same issues as ``mem`` parameter with the difference that the role of the user plays QEMU using implicit generic or board specific splitting rule. Use ``memdev`` with *memory-backend-ram* backend or ``mem`` (if -it's supported by used machine
Re: Libvirt NVME support
On Mon, Nov 16, 2020 at 23:01:00 +, Suraj Kasi wrote: > Hi Peter, > > Just wanted to follow up. As Thanos mentioned that we want a virtual NVMe > controller in the guest for which the support doesn't yet exist in libvirt. > Is it something that would be accepted if we were to implement it? Sure. Preferably post your proposed design of the XML as a RFC patch on the list so that the design can be discussed without wasting any development work first. As a separate question, is there any performance benefit of emulating a NVMe controller compared to e.g. virtio-scsi?
Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
On 11/18/20 8:59 AM, Peter Krempa wrote:
On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote:
Similarly to previous commits, we can utilize domCaps to check if
graphics type is supported.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_capabilities.h | 3 +++
src/qemu/qemu_validate.c | 40
3 files changed, 17 insertions(+), 28 deletions(-)
[...]
@@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const
virDomainGraphicsDef *graphics,
}
break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unsupported graphics type '%s'"),
- virDomainGraphicsTypeToString(graphics->type));
-return -1;
case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
-default:
-return -1;
+break;
Removing 'default: ' is not necessary once you use proper type for the
variable in the switch statement, which is our usual approach.
I guess I don't need to typecast ->type since it's already stored as
virDomainGraphicsType in the struct.
The default and _LAST case should use virReportEnumRangeError.
I'm not sure it's adding useful code. In this very patch I'm adding a
check that rejects unknown values for ->type and thus I don't see a way
how the control could hit any of these 'case', or 'default'.
Sorry,
Michal
Re: [PATCH 5/5] qemu_validate: Deduplicate code for graphics type check
On Tue, Nov 17, 2020 at 12:28:27 +0100, Michal Privoznik wrote:
> Similarly to previous commits, we can utilize domCaps to check if
> graphics type is supported.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_capabilities.c | 2 +-
> src/qemu/qemu_capabilities.h | 3 +++
> src/qemu/qemu_validate.c | 40
> 3 files changed, 17 insertions(+), 28 deletions(-)
[...]
> @@ -3903,15 +3892,12 @@ qemuValidateDomainDeviceDefGraphics(const
> virDomainGraphicsDef *graphics,
> }
>
> break;
> +
> +case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("unsupported graphics type '%s'"),
> - virDomainGraphicsTypeToString(graphics->type));
> -return -1;
> case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
> -default:
> -return -1;
> +break;
Removing 'default: ' is not necessary once you use proper type for the
variable in the switch statement, which is our usual approach.
The default and _LAST case should use virReportEnumRangeError.
