[Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Christian Borntraeger]

We want to configure several things in KVM that go beyond what
ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
and we need to do more complex actions) can provide. Instead of
adding several s390 specific ioctls, lets provide a configuration
and control device that encapsulates different commands into
groups of the same area (MEMORY, CPU, ..)

We also provide an initial nameless base group, with a simple first
user to set the guest name. We need that name in the kernel for
the emulation of STSI (which provides the guest name to the guest)
but we need to implement the emulation in supervisor mode, as it
also provides the underlying levels of hipervisors.

Currently we have the following GROUPS and ATTRs pending, which
configure some memory management related function or allow to set
the guest facilities, cpuids etc:

#define KVM_DEV_CONFIG_GROUP            0
#define KVM_DEV_CONFIG_NAME             0

#define KVM_DEV_CONFIG_GROUP_MEM        1
#define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
#define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
#define KVM_DEV_CONFIG_MEM_CLR_PAGES    2

#define KVM_DEV_CONFIG_GROUP_CPU        2
#define KVM_DEV_CONFIG_CPU_TYPE         0
#define KVM_DEV_CONFIG_CPU_FAC          1
#define KVM_DEV_CONFIG_CPU_FAC_MASK     2
#define KVM_DEV_CONFIG_CPU_IBC          3
#define KVM_DEV_CONFIG_CPU_IBC_RANGE    4



In addition other groups like 
#define KVM_DEV_CONFIG_GROUP_CRYPTO 
are under consideration to configure crypto acceleration.

Unless there is a major concern, I will add this to the next
s390 PULL requests for KVM.

Christian

[Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Christian Borntraeger]

From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
configuration and control attributes of particular vm.
The device is created by KVM_CREATE_DEVICE ioctl.
The attributes may be retrieved and stored by calling
KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/devices/s390_config.txt |  13 +++
 arch/s390/include/asm/kvm_host.h                  |   6 +
 arch/s390/include/uapi/asm/kvm.h                  |  11 ++
 arch/s390/kvm/Makefile                            |   2 +-
 arch/s390/kvm/config.c                            | 133 ++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c                          |   7 ++
 arch/s390/kvm/kvm-s390.h                          |   4 +
 arch/s390/kvm/priv.c                              |   6 +-
 include/linux/kvm_host.h                          |   1 +
 include/uapi/linux/kvm.h                          |   1 +
 virt/kvm/kvm_main.c                               |   3 +
 11 files changed, 185 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/s390_config.txt
 create mode 100644 arch/s390/kvm/config.c

diff --git a/Documentation/virtual/kvm/devices/s390_config.txt b/Documentation/virtual/kvm/devices/s390_config.txt
new file mode 100644
index 0000000..f9bc251
--- /dev/null
+++ b/Documentation/virtual/kvm/devices/s390_config.txt
@@ -0,0 +1,13 @@
+S390 Config Device
+==================
+
+The s390 config device is used for storage and retrieval of various kinds
+of configuration related information which should be shared between kvm
+and userspace.
+
+Groups:
+KVM_DEV_CONFIG_GROUP
+
+Attributes for KVM_DEV_CONFIG_GROUP:
+KVM_DEV_CONFIG_NAME - The name for this vm instance. A string of at
+			   most 128 characters.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 154b600..da93b6e 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -277,6 +277,11 @@ struct s390_io_adapter {
 #define MAX_S390_IO_ADAPTERS ((MAX_ISC + 1) * 8)
 #define MAX_S390_ADAPTER_MAPS 256
 
+struct kvm_s390_config {
+	struct mutex lock;
+	struct kvm_s390_attr_name name;
+};
+
 struct kvm_arch{
 	struct sca_block *sca;
 	debug_info_t *dbf;
@@ -286,6 +291,7 @@ struct kvm_arch{
 	int css_support;
 	int use_irqchip;
 	struct s390_io_adapter *adapters[MAX_S390_IO_ADAPTERS];
+	struct kvm_s390_config *cfg;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index c003c6a..d53dd45 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -79,6 +79,17 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+
+
+/* config device specific GROUPS */
+
+#define KVM_DEV_CONFIG_GROUP		0
+#define KVM_DEV_CONFIG_NAME		0
+
+struct kvm_s390_attr_name {
+	char name[128];
+};
+
 #define KVM_SYNC_PREFIX (1UL << 0)
 #define KVM_SYNC_GPRS   (1UL << 1)
 #define KVM_SYNC_ACRS   (1UL << 2)
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index d3adb37..79f130b 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -11,5 +11,5 @@ common-objs = $(KVM)/kvm_main.o $(KVM)/eventfd.o  $(KVM)/async_pf.o $(KVM)/irqch
 
 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
 
-kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o
+kvm-objs := $(common-objs) kvm-s390.o intercept.o interrupt.o priv.o sigp.o diag.o config.o
 obj-$(CONFIG_KVM) += kvm.o
diff --git a/arch/s390/kvm/config.c b/arch/s390/kvm/config.c
new file mode 100644
index 0000000..f887116
--- /dev/null
+++ b/arch/s390/kvm/config.c
@@ -0,0 +1,133 @@
+/*
+ * handling CONFIG kvm device
+ *
+ * Copyright IBM Corp. 2014
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2 only)
+ * as published by the Free Software Foundation.
+ *
+ *    Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
+ *               Christian Borntraeger <borntraeger@de.ibm.com>
+ */
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <asm/uaccess.h>
+#include "kvm-s390.h"
+
+static const struct kvm_s390_config config_defaults = {
+	.name = { "KVMguest" },
+};
+
+/*
+ * This is called from architecture code, since we access config even if
+ * userspace does not create a config device.
+ */
+struct kvm_s390_config *kvm_s390_config_init(void)
+{
+	struct kvm_s390_config *config;
+
+	config = kmalloc(sizeof(struct kvm_s390_config), GFP_KERNEL);
+	if (config) {
+		memcpy(config, &config_defaults, sizeof(*config));
+		mutex_init(&config->lock);
+	}
+
+	return config;
+}
+
+void kvm_s390_config_free(struct kvm_s390_config *config)
+{
+	kfree(config);
+}
+
+/*
+ * kvm accesses the config datastructure even if userspace did not create a
+ * config kvm device. So we only do kvm device related cleanups in the
+ * create and destroy callbacks.
+ */
+static int config_create(struct kvm_device *dev, u32 attr)
+{
+	return 0;
+}
+
+static void config_destroy(struct kvm_device *dev)
+{
+	kfree(dev);
+}
+
+static int config_set(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	struct kvm_s390_config *config = dev->kvm->arch.cfg;
+	int ret;
+
+	switch (attr->attr) {
+	case KVM_DEV_CONFIG_NAME:
+		mutex_lock(&config->lock);
+		ret = copy_from_user(&config->name, (void __user *) attr->addr,
+				     sizeof(config->name));
+		mutex_unlock(&config->lock);
+		return ret;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int config_get(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	struct kvm_s390_config *config = dev->kvm->arch.cfg;
+	int ret;
+
+	switch (attr->attr) {
+	case KVM_DEV_CONFIG_NAME:
+		mutex_lock(&config->lock);
+		ret = copy_to_user((void __user *) attr->addr, &config->name,
+				   sizeof(config->name));
+		mutex_unlock(&config->lock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int config_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_CONFIG_GROUP:
+		return config_set(dev, attr);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int config_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_CONFIG_GROUP:
+		return config_get(dev, attr);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int config_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_CONFIG_GROUP:
+		switch (attr->attr) {
+		case KVM_DEV_CONFIG_NAME:
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+struct kvm_device_ops kvm_s390_config_ops = {
+	.name = "kvm_s390_config",
+	.create = config_create,
+	.destroy = config_destroy,
+	.get_attr = config_get_attr,
+	.set_attr = config_set_attr,
+	.has_attr = config_has_attr,
+};
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b3ecb8f..4504015 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -307,8 +307,14 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 	kvm->arch.css_support = 0;
 	kvm->arch.use_irqchip = 0;
+	kvm->arch.cfg = kvm_s390_config_init();
+	if (!kvm->arch.cfg)
+		goto out_gmap;
 
 	return 0;
+out_gmap:
+	if (!kvm_is_ucontrol(kvm))
+		gmap_free(kvm->arch.gmap);
 out_nogmap:
 	debug_unregister(kvm->arch.dbf);
 out_nodbf:
@@ -371,6 +377,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	if (!kvm_is_ucontrol(kvm))
 		gmap_free(kvm->arch.gmap);
 	kvm_s390_destroy_adapters(kvm);
+	kvm_s390_config_free(kvm->arch.cfg);
 }
 
 /* Section: vcpu related */
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 3c1e227..50185a3 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -168,4 +168,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
 int psw_extint_disabled(struct kvm_vcpu *vcpu);
 void kvm_s390_destroy_adapters(struct kvm *kvm);
 
+/* implemented in config.c */
+struct kvm_s390_config *kvm_s390_config_init(void);
+void kvm_s390_config_free(struct kvm_s390_config *config);
+
 #endif
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 476e9e2..4e5fba5 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/kvm.h>
+#include <linux/kvm_host.h>
 #include <linux/gfp.h>
 #include <linux/errno.h>
 #include <linux/compat.h>
@@ -414,7 +415,10 @@ static void handle_stsi_3_2_2(struct kvm_vcpu *vcpu, struct sysinfo_3_2_2 *mem)
 	mem->vm[0].cpus_standby = 0;
 	mem->vm[0].cpus_reserved = 0;
 	mem->vm[0].caf = 1000;
-	memcpy(mem->vm[0].name, "KVMguest", 8);
+
+	mutex_lock(&vcpu->kvm->arch.cfg->lock);
+	memcpy(mem->vm[0].name, &vcpu->kvm->arch.cfg->name, 8);
+	mutex_unlock(&vcpu->kvm->arch.cfg->lock);
 	ASCEBC(mem->vm[0].name, 8);
 	memcpy(mem->vm[0].cpi, "KVM/Linux       ", 16);
 	ASCEBC(mem->vm[0].cpi, 16);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7d21cf9..5c5c39c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1080,6 +1080,7 @@ extern struct kvm_device_ops kvm_xics_ops;
 extern struct kvm_device_ops kvm_vfio_ops;
 extern struct kvm_device_ops kvm_arm_vgic_v2_ops;
 extern struct kvm_device_ops kvm_flic_ops;
+extern struct kvm_device_ops kvm_s390_config_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a8f4ee5..d0e738f 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -934,6 +934,7 @@ struct kvm_device_attr {
 #define   KVM_DEV_VFIO_GROUP_DEL			2
 #define KVM_DEV_TYPE_ARM_VGIC_V2	5
 #define KVM_DEV_TYPE_FLIC		6
+#define KVM_DEV_TYPE_S390_CONFIG	7
 
 /*
  * ioctls for VM fds
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5fd4cf8..a9f909a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2285,6 +2285,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 	case KVM_DEV_TYPE_FLIC:
 		ops = &kvm_flic_ops;
 		break;
+	case KVM_DEV_TYPE_S390_CONFIG:
+		ops = &kvm_s390_config_ops;
+		break;
 #endif
 	default:
 		return -ENODEV;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Alexander Graf]

On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>
> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
> configuration and control attributes of particular vm.
> The device is created by KVM_CREATE_DEVICE ioctl.
> The attributes may be retrieved and stored by calling
> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

I don't think a device is particularly the best fit. A device can 
usually be instantiated multiple times. The configuration device can 
only be created once. A device also gets created by user space which 
enables it to receive the fd to drive it. Your device has to be created 
during VM creation.

I think VM configuration is common enough to just make this a separate 
interface.


Alex

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Christian Borntraeger]

On 01/04/14 16:58, Alexander Graf wrote:
> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>
>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>> configuration and control attributes of particular vm.
>> The device is created by KVM_CREATE_DEVICE ioctl.
>> The attributes may be retrieved and stored by calling
>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> I don't think a device is particularly the best fit. A device can usually be instantiated multiple times. The configuration device can only be created once. A device also gets created by user space which enables it to receive the fd to drive it. Your device has to be created during VM creation.

I remember some discussion a year or 2 ago, and IIRC a config device
was actually your idea ;-) (The other idea that we had, was ONE_REG for the VM)

> 
> I think VM configuration is common enough to just make this a separate interface.

So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
Seems like an easy enough change. Would you reuse the kvm_attr structure for that?

> 
> 
> Alex
> 

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Alexander Graf]

On 04/01/2014 05:04 PM, Christian Borntraeger wrote:
> On 01/04/14 16:58, Alexander Graf wrote:
>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>
>>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>>> configuration and control attributes of particular vm.
>>> The device is created by KVM_CREATE_DEVICE ioctl.
>>> The attributes may be retrieved and stored by calling
>>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>>
>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> I don't think a device is particularly the best fit. A device can usually be instantiated multiple times. The configuration device can only be created once. A device also gets created by user space which enables it to receive the fd to drive it. Your device has to be created during VM creation.
> I remember some discussion a year or 2 ago, and IIRC a config device
> was actually your idea ;-) (The other idea that we had, was ONE_REG for the VM)

Omg, really? :o

A device would make sense for a specific "system information" 
instruction trap that we handle in-kernel for whatever reason (usually 
because it's performance critical) and some mandatory say to make sure 
user space always creates it. And some checks to make sure it can't get 
created twice.

Speaking of which, why don't we just forward STSI to user space with an 
ENABLE_CAP and handle all of this there? It's not performance critical 
at all, right?

>> I think VM configuration is common enough to just make this a separate interface.
> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
> Seems like an easy enough change. Would you reuse the kvm_attr structure for that?

Yeah, reuse whatever we can. Basically just remove the device 
boilerplate - I don't think it's impressively useful for a non-device.


Alex

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Christian Borntraeger]

On 01/04/14 17:12, Alexander Graf wrote:
> On 04/01/2014 05:04 PM, Christian Borntraeger wrote:
>> On 01/04/14 16:58, Alexander Graf wrote:
>>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>>
>>>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>>>> configuration and control attributes of particular vm.
>>>> The device is created by KVM_CREATE_DEVICE ioctl.
>>>> The attributes may be retrieved and stored by calling
>>>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>>>
>>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> I don't think a device is particularly the best fit. A device can usually be instantiated multiple times. The configuration device can only be created once. A device also gets created by user space which enables it to receive the fd to drive it. Your device has to be created during VM creation.
>> I remember some discussion a year or 2 ago, and IIRC a config device
>> was actually your idea ;-) (The other idea that we had, was ONE_REG for the VM)
> 
> Omg, really? :o
> 
> A device would make sense for a specific "system information" instruction trap that we handle in-kernel for whatever reason (usually because it's performance critical) and some mandatory say to make sure user space always creates it. And some checks to make sure it can't get created twice.

Well the device created twice problem  is also true for all existing KVM devices. 
The only missing piece is the check in the config device, no?

> 
> Speaking of which, why don't we just forward STSI to user space with an ENABLE_CAP and handle all of this there? It's not performance critical at all, right?

No, performance is not critical.
The thing is, that we definitely need the kernel to handle parts of STSI, as we have to provide information from the upper hipervisor (LPAR or zVM). This information is only available in kernel space. So in essence we could only forward a small subset of STSI, namely stsi3_2_2. But we still have to call stsi_3_2_2 in the kernel,
as 3_2_2 does contain the list of hipervisors underneath us (KVM under z/VM). 

So then only thing that we could do is to forward STSI_3_2_2 to qemu when a capability is set and after the kernel has filled in the upper layers. 
QEMU then has to modify the page that the kernel touched and go back. Would work, but needs a capability and preferably an own exit. An new ioctl or
a subcode of an ioctl (attr/group whatever) seems easier.

> 
>>> I think VM configuration is common enough to just make this a separate interface.
>> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
>> Seems like an easy enough change. Would you reuse the kvm_attr structure for that?
> 
> Yeah, reuse whatever we can. Basically just remove the device boilerplate - I don't think it's impressively useful for a non-device.

See above, name is just a simple first user.  
The thing is, that we have to have the ioctl either define a proper namespace (unique groups attrs) or to make it s390 specific. The device approach does help us here. 

I personally dont mind which way to go, as long as Paolo is fine with the approach, and nobody complains about the functions being non-QOM.

Christian

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Alexander Graf]

On 01.04.2014, at 21:19, Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/04/14 17:12, Alexander Graf wrote:
>> On 04/01/2014 05:04 PM, Christian Borntraeger wrote:
>>> On 01/04/14 16:58, Alexander Graf wrote:
>>>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>>>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>>> 
>>>>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>>>>> configuration and control attributes of particular vm.
>>>>> The device is created by KVM_CREATE_DEVICE ioctl.
>>>>> The attributes may be retrieved and stored by calling
>>>>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>>>> 
>>>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> I don't think a device is particularly the best fit. A device can usually be instantiated multiple times. The configuration device can only be created once. A device also gets created by user space which enables it to receive the fd to drive it. Your device has to be created during VM creation.
>>> I remember some discussion a year or 2 ago, and IIRC a config device
>>> was actually your idea ;-) (The other idea that we had, was ONE_REG for the VM)
>> 
>> Omg, really? :o
>> 
>> A device would make sense for a specific "system information" instruction trap that we handle in-kernel for whatever reason (usually because it's performance critical) and some mandatory say to make sure user space always creates it. And some checks to make sure it can't get created twice.
> 
> Well the device created twice problem  is also true for all existing KVM devices. 
> The only missing piece is the check in the config device, no?
> 
>> 
>> Speaking of which, why don't we just forward STSI to user space with an ENABLE_CAP and handle all of this there? It's not performance critical at all, right?
> 
> No, performance is not critical.
> The thing is, that we definitely need the kernel to handle parts of STSI, as we have to provide information from the upper hipervisor (LPAR or zVM). This information is only available in kernel space. So in essence we could only forward a small subset of STSI, namely stsi3_2_2. But we still have to call stsi_3_2_2 in the kernel,
> as 3_2_2 does contain the list of hipervisors underneath us (KVM under z/VM). 
> 
> So then only thing that we could do is to forward STSI_3_2_2 to qemu when a capability is set and after the kernel has filled in the upper layers. 
> QEMU then has to modify the page that the kernel touched and go back. Would work, but needs a capability and preferably an own exit. An new ioctl or
> a subcode of an ioctl (attr/group whatever) seems easier.

I would consider these 2 orthogonal bits of information. User space wants to get information about its underlying hypervisors regardless of KVM, no? So we should have some interface to bubble STSI information of the current system to user space either way.

QEMU could use that and add a few bits of its own. That way we could handle all of STSI in QEMU and get out of the business of defining complicated interfaces.

> 
>> 
>>>> I think VM configuration is common enough to just make this a separate interface.
>>> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
>>> Seems like an easy enough change. Would you reuse the kvm_attr structure for that?
>> 
>> Yeah, reuse whatever we can. Basically just remove the device boilerplate - I don't think it's impressively useful for a non-device.
> 
> See above, name is just a simple first user.  
> The thing is, that we have to have the ioctl either define a proper namespace (unique groups attrs) or to make it s390 specific. The device approach does help us here. 

If you like the device approach, make sure to create it on VM creation and only implement a specific ioctl to fetch its fd. We don't create the configuration information pseudo device after VM creation - it's always there :).

> I personally dont mind which way to go, as long as Paolo is fine with the approach, and nobody complains about the functions being non-QOM.

I think the most obvious and straight forward way would be to deal with all of STSI in user space. Make it a separate exit type similar to hypercalls and don't worry about QOM'ification of anything. This thing is on the same level as CPUID really - just VM wide :).


Alex

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Christian Borntraeger]

On 01/04/14 21:36, Alexander Graf wrote:
[...]
>>> Speaking of which, why don't we just forward STSI to user space with an ENABLE_CAP and handle all of this there? It's not performance critical at all, right?
>>
>> No, performance is not critical.
>> The thing is, that we definitely need the kernel to handle parts of STSI, as we have to provide information from the upper hipervisor (LPAR or zVM). This information is only available in kernel space. So in essence we could only forward a small subset of STSI, namely stsi3_2_2. But we still have to call stsi_3_2_2 in the kernel,
>> as 3_2_2 does contain the list of hipervisors underneath us (KVM under z/VM). 
>>
>> So then only thing that we could do is to forward STSI_3_2_2 to qemu when a capability is set and after the kernel has filled in the upper layers. 
>> QEMU then has to modify the page that the kernel touched and go back. Would work, but needs a capability and preferably an own exit. An new ioctl or
>> a subcode of an ioctl (attr/group whatever) seems easier.
> 
> I would consider these 2 orthogonal bits of information. User space wants to get information about its underlying hypervisors regardless of KVM, no? So we should have some interface to bubble STSI information of the current system to user space either way.

There is /proc/sysinfo and it provides most (but not all) of the STSI information. But we dont want to go that path, really. Providing STSI as
binary blob is also complex and error-prone.
Think about it again: Instead of having a small qemu->kernel interface for specific subsets of stsi, you suggest a kernel->qemu interface
for the complete set of STSI, so that qemu can then emulate it? (We talk about 20 pages in the principles of operation). T
 
> QEMU could use that and add a few bits of its own. That way we could handle all of STSI in QEMU and get out of the business of defining complicated interfaces.

This also misses my main point: The name is just the first user. I want to have some interface that allows me to do other things:
enable CMMA, reset the CMMA memory usage state on reset, zap the page tables on clear reset, set the cpu facilities (some parts need to be in kernel so that we can block specific operations), enable s390 specific settings, whatever. 

The usual approach was create new KVM ioctls, each for one feature + capability. This sounded like overkill.
So please dont focus on stsi, I need some interface that I can use for other pending patches. And if that interface is generic enough, we might also use that for name (or not depending on the stsi discussion). I am not married to the device idea, anything that works out fine is ok with me.

>>>>> I think VM configuration is common enough to just make this a separate interface.
>>>> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
>>>> Seems like an easy enough change. Would you reuse the kvm_attr structure for that?
>>>
>>> Yeah, reuse whatever we can. Basically just remove the device boilerplate - I don't think it's impressively useful for a non-device.
>>
>> See above, name is just a simple first user.  
>> The thing is, that we have to have the ioctl either define a proper namespace (unique groups attrs) or to make it s390 specific. The device approach does help us here. 
> 
> If you like the device approach, make sure to create it on VM creation and only implement a specific ioctl to fetch its fd. We don't create the configuration information pseudo device after VM creation - it's always there :).
> 
>> I personally dont mind which way to go, as long as Paolo is fine with the approach, and nobody complains about the functions being non-QOM.
> 
> I think the most obvious and straight forward way would be to deal with all of STSI in user space. Make it a separate exit type similar to hypercalls and don't worry about QOM'ification of anything. This thing is on the same level as CPUID really - just VM wide :).

For 3_2_2 this might be possible solution, but not for the other codes. But as I said, the name is not my main problem here.

Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Alexander Graf]

On 01.04.2014, at 21:19, Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/04/14 17:12, Alexander Graf wrote:
>> On 04/01/2014 05:04 PM, Christian Borntraeger wrote:
>>> On 01/04/14 16:58, Alexander Graf wrote:
>>>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>>>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>>> 
>>>>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>>>>> configuration and control attributes of particular vm.
>>>>> The device is created by KVM_CREATE_DEVICE ioctl.
>>>>> The attributes may be retrieved and stored by calling
>>>>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>>>> 
>>>>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> I don't think a device is particularly the best fit. A device can usually be instantiated multiple times. The configuration device can only be created once. A device also gets created by user space which enables it to receive the fd to drive it. Your device has to be created during VM creation.
>>> I remember some discussion a year or 2 ago, and IIRC a config device
>>> was actually your idea ;-) (The other idea that we had, was ONE_REG for the VM)
>> 
>> Omg, really? :o
>> 
>> A device would make sense for a specific "system information" instruction trap that we handle in-kernel for whatever reason (usually because it's performance critical) and some mandatory say to make sure user space always creates it. And some checks to make sure it can't get created twice.
> 
> Well the device created twice problem  is also true for all existing KVM devices. 
> The only missing piece is the check in the config device, no?
> 
>> 
>> Speaking of which, why don't we just forward STSI to user space with an ENABLE_CAP and handle all of this there? It's not performance critical at all, right?
> 
> No, performance is not critical.
> The thing is, that we definitely need the kernel to handle parts of STSI, as we have to provide information from the upper hipervisor (LPAR or zVM). This information is only available in kernel space. So in essence we could only forward a small subset of STSI, namely stsi3_2_2. But we still have to call stsi_3_2_2 in the kernel,
> as 3_2_2 does contain the list of hipervisors underneath us (KVM under z/VM). 
> 
> So then only thing that we could do is to forward STSI_3_2_2 to qemu when a capability is set and after the kernel has filled in the upper layers. 
> QEMU then has to modify the page that the kernel touched and go back. Would work, but needs a capability and preferably an own exit. An new ioctl or
> a subcode of an ioctl (attr/group whatever) seems easier.

I would consider these 2 orthogonal bits of information. User space wants to get information about its underlying hypervisors regardless of KVM, no? So we should have some interface to bubble STSI information of the current system to user space either way.

QEMU could use that and add a few bits of its own. That way we could handle all of STSI in QEMU and get out of the business of defining complicated interfaces.

> 
>> 
>>>> I think VM configuration is common enough to just make this a separate interface.
>>> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, instead?
>>> Seems like an easy enough change. Would you reuse the kvm_attr structure for that?
>> 
>> Yeah, reuse whatever we can. Basically just remove the device boilerplate - I don't think it's impressively useful for a non-device.
> 
> See above, name is just a simple first user.  
> The thing is, that we have to have the ioctl either define a proper namespace (unique groups attrs) or to make it s390 specific. The device approach does help us here. 

If you like the device approach, make sure to create it on VM creation and only implement a specific ioctl to fetch its fd. We don't create the configuration information pseudo device after VM creation - it's always there :).

> I personally dont mind which way to go, as long as Paolo is fine with the approach, and nobody complains about the functions being non-QOM.

I think the most obvious and straight forward way would be to deal with all of STSI in user space. Make it a separate exit type similar to hypercalls and don't worry about QOM'ification of anything. This thing is on the same level as CPUID really - just VM wide :).


Alex

[Qemu-devel] [PATCH/RFC] s390x/kvm: implement and use QEMU config device for s390

[Christian Borntraeger]

From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>

The following patch adds Qemu CONFIG device,
which interacts with kvm CONFIG device by calling KVM_CREATE_DEVICE
(to create the device in kernel), KVM_GET_DEVICE_ATTR and
KVM_SET_DEVICE_ATTR (to get and set the particular attributes of
KVM CONFIG device respectively).
This patch uses the Qemu CONFIG device to copy the
qemu_name (if specified) into the kvm CONFIG device.

Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 default-configs/s390x-softmmu.mak |   1 +
 hw/s390x/Makefile.objs            |   1 +
 hw/s390x/config.c                 | 113 ++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c        |   6 ++
 hw/s390x/s390-virtio.c            |  24 ++++++++
 hw/s390x/s390-virtio.h            |   1 +
 include/hw/s390x/config.h         |  51 +++++++++++++++++
 linux-headers/asm-s390/kvm.h      |  11 ++++
 linux-headers/linux/kvm.h         |   1 +
 trace-events                      |   4 ++
 10 files changed, 213 insertions(+)
 create mode 100644 hw/s390x/config.c
 create mode 100644 include/hw/s390x/config.h

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index d843dc0..a07697d 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -1,3 +1,4 @@
 CONFIG_VIRTIO=y
 CONFIG_SCLPCONSOLE=y
 CONFIG_S390_FLIC=$(CONFIG_KVM)
+CONFIG_S390_CONFIG=$(CONFIG_KVM)
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 1ba6c3a..dc03e1d 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -8,3 +8,4 @@ obj-y += ipl.o
 obj-y += css.o
 obj-y += s390-virtio-ccw.o
 obj-y += virtio-ccw.o
+obj-$(CONFIG_S390_CONFIG) += config.o
diff --git a/hw/s390x/config.c b/hw/s390x/config.c
new file mode 100644
index 0000000..f70e880
--- /dev/null
+++ b/hw/s390x/config.c
@@ -0,0 +1,113 @@
+/*
+ * S390 configuration and control device
+ *
+ * Copyright 2014 IBM Corp.
+ * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include <sys/ioctl.h>
+#include "hw/s390x/config.h"
+#include "trace.h"
+
+S390ConfigState *get_config_state(void)
+{
+    return S390_CONFIG(object_resolve_path("/machine/s390-config", NULL));
+}
+
+static int config_set(S390ConfigState *config, struct kvm_device_attr *attr)
+{
+    int rc;
+
+    rc = ioctl(config->fd, KVM_SET_DEVICE_ATTR, attr);
+    return rc < 0 ? -errno : rc;
+}
+
+/*
+static int config_get(S390ConfigState *config, struct kvm_device_attr *attr)
+{
+    int rc;
+
+    rc = ioctl(config->fd, KVM_GET_DEVICE_ATTR, attr);
+    return rc < 0 ? -errno : rc;
+}
+*/
+
+static int config_set_name(S390ConfigState *config, char *name)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_DEV_CONFIG_GROUP,
+        .attr  = KVM_DEV_CONFIG_NAME,
+        .addr  = (uint64_t)name,
+    };
+
+    if (!name ||
+        (qemu_strnlen(name, S390_MACHINE_NAME_SIZE)
+         >= S390_MACHINE_NAME_SIZE)) {
+        return -EINVAL;
+    }
+
+    return config_set(config, &attr);
+}
+
+static void s390_config_realize(DeviceState *dev, Error **errp)
+{
+    S390ConfigState *cfg = S390_CONFIG(dev);
+    struct kvm_create_device cd = {0};
+    int ret;
+
+    if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
+        trace_config_no_device_api(errno);
+        return;
+    }
+
+    cd.type = KVM_DEV_TYPE_S390_CONFIG;
+    ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
+    if (ret < 0) {
+        trace_config_create_device(errno);
+        return;
+    }
+    cfg->fd = cd.fd;
+}
+
+void s390_config_dev_init(void)
+{
+    DeviceState *dev;
+
+    if (kvm_enabled()) {
+        dev  = qdev_create(NULL, TYPE_S390_CONFIG);
+
+        object_property_add_child(qdev_get_machine(),
+                                  TYPE_S390_CONFIG,
+                                  OBJECT(dev), NULL);
+        qdev_init_nofail(dev);
+    }
+}
+
+static void s390_config_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    S390ConfigClass *scc = S390_CONFIG_CLASS(oc);
+
+    dc->realize = s390_config_realize;
+    scc->set_name = config_set_name;
+}
+
+static const TypeInfo s390_config_info = {
+    .name          = TYPE_S390_CONFIG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(S390ConfigState),
+    .class_init    = s390_config_class_init,
+    .class_size    = sizeof(S390ConfigClass),
+};
+
+
+static void s390_config_register_types(void)
+{
+    type_register_static(&s390_config_info);
+}
+
+type_init(s390_config_register_types)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0d4f6ae..1e4b9ad 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -14,9 +14,11 @@
 #include "s390-virtio.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
+#include "hw/s390x/config.h"
 #include "ioinst.h"
 #include "css.h"
 #include "virtio-ccw.h"
+#include "qemu/error-report.h"
 
 void io_subsystem_reset(void)
 {
@@ -106,6 +108,10 @@ static void ccw_init(QEMUMachineInitArgs *args)
                       args->initrd_filename, "s390-ccw.img");
     s390_flic_init();
 
+    s390_config_dev_init();
+
+    s390_set_vm_name();
+
     /* register hypercalls */
     virtio_ccw_register_hcalls();
 
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index aef2003..7a296fd 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -39,6 +39,8 @@
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/s390-virtio.h"
 
+#include "hw/s390x/config.h"
+
 //#define DEBUG_S390
 
 #ifdef DEBUG_S390
@@ -223,6 +225,24 @@ void s390_create_virtio_net(BusState *bus, const char *name)
     }
 }
 
+void s390_set_vm_name(void)
+{
+    S390ConfigState *config = get_config_state();
+    char machine_name[S390_MACHINE_NAME_SIZE];
+    S390ConfigClass *scc;
+
+    if (!config || !qemu_name) {
+        return;
+    }
+    strncpy(machine_name, qemu_name, sizeof(machine_name));
+
+    scc = S390_CONFIG_GET_CLASS(config);
+
+    if (scc->set_name(config, machine_name)) {
+        error_report("Setting machine name failed: %m");
+    }
+}
+
 /* PC hardware initialisation */
 static void s390_init(QEMUMachineInitArgs *args)
 {
@@ -252,6 +272,10 @@ static void s390_init(QEMUMachineInitArgs *args)
                       args->initrd_filename, ZIPL_FILENAME);
     s390_flic_init();
 
+    s390_config_dev_init();
+
+    s390_set_vm_name();
+
     /* register hypercalls */
     s390_virtio_register_hcalls();
 
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index 5c405e7..deca18e 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -26,4 +26,5 @@ void s390_init_ipl_dev(const char *kernel_filename,
                        const char *initrd_filename,
                        const char *firmware);
 void s390_create_virtio_net(BusState *bus, const char *name);
+void s390_set_vm_name(void);
 #endif
diff --git a/include/hw/s390x/config.h b/include/hw/s390x/config.h
new file mode 100644
index 0000000..14d9b02
--- /dev/null
+++ b/include/hw/s390x/config.h
@@ -0,0 +1,51 @@
+/*
+ * S390 configuration and control device
+ *
+ * Copyright 2014 IBM Corp.
+ * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef __KVM_S390_CONFIG_H
+#define __KVM_S390_CONFIG_H
+
+#include "hw/sysbus.h"
+#include "migration/qemu-file.h"
+
+#define TYPE_S390_CONFIG "s390-config"
+#define S390_CONFIG(obj) \
+    OBJECT_CHECK(S390ConfigState, (obj), TYPE_S390_CONFIG)
+#define S390_CONFIG_CLASS(class) \
+     OBJECT_CLASS_CHECK(S390ConfigClass, (class), TYPE_S390_CONFIG)
+#define S390_CONFIG_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(S390ConfigClass, (obj), TYPE_S390_CONFIG)
+
+typedef struct S390ConfigState {
+    SysBusDevice parent_obj;
+    uint32_t fd;
+} S390ConfigState;
+
+typedef struct S390ConfigClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+    int (*set_name)(S390ConfigState *config, char *name);
+} S390ConfigClass;
+
+#ifdef CONFIG_KVM
+void s390_config_dev_init(void);
+S390ConfigState *get_config_state(void);
+#define S390_MACHINE_NAME_SIZE sizeof(struct kvm_s390_attr_name)
+#else
+static inline void s390_config_dev_init(void) { }
+static inline S390ConfigState *get_config_state(void)
+{
+    return NULL;
+}
+#define S390_MACHINE_NAME_SIZE 128
+#endif
+
+#endif /* __KVM_S390_CONFIG_H */
diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index cb4c1eb..432ffa5 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -57,6 +57,17 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+
+
+/* config device specific GROUPS */
+
+#define KVM_DEV_CONFIG_GROUP		0
+#define KVM_DEV_CONFIG_NAME		0
+
+struct kvm_s390_attr_name {
+	char name[128];
+};
+
 #define KVM_SYNC_PREFIX (1UL << 0)
 #define KVM_SYNC_GPRS   (1UL << 1)
 #define KVM_SYNC_ACRS   (1UL << 2)
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e27a4b3..59d1ec3 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -921,6 +921,7 @@ struct kvm_device_attr {
 #define   KVM_DEV_VFIO_GROUP_DEL			2
 #define KVM_DEV_TYPE_ARM_VGIC_V2	5
 #define KVM_DEV_TYPE_FLIC		6
+#define KVM_DEV_TYPE_S390_CONFIG	7
 
 /*
  * ioctls for VM fds
diff --git a/trace-events b/trace-events
index 3df3f32..6ccd619 100644
--- a/trace-events
+++ b/trace-events
@@ -1185,6 +1185,10 @@ css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc %x)"
 virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command %x"
 virtio_ccw_new_device(int cssid, int ssid, int schid, int devno, const char *devno_mode) "VIRTIO-CCW: add subchannel %x.%x.%04x, devno %04x (%s)"
 
+# hw/s390x/config.c
+config_no_device_api(int err) "config_dev: no Device Control API support %d"
+config_create_device(int err) "config_dev: create device failed %d"
+
 # hw/intc/s390_flic.c
 flic_create_device(int err) "flic: create device failed %d"
 flic_no_device_api(int err) "flic: no Device Contral API support %d"
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH/RFC] s390x/kvm: implement and use QEMU config device for s390

[Paolo Bonzini]

Il 01/04/2014 16:47, Christian Borntraeger ha scritto:
> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>
> The following patch adds Qemu CONFIG device,
> which interacts with kvm CONFIG device by calling KVM_CREATE_DEVICE
> (to create the device in kernel), KVM_GET_DEVICE_ATTR and
> KVM_SET_DEVICE_ATTR (to get and set the particular attributes of
> KVM CONFIG device respectively).
> This patch uses the Qemu CONFIG device to copy the
> qemu_name (if specified) into the kvm CONFIG device.
>
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

On the QEMU side we'll soon have per-machine-type options in -machine. 
s390 can then use that.

Paolo

> ---
>  default-configs/s390x-softmmu.mak |   1 +
>  hw/s390x/Makefile.objs            |   1 +
>  hw/s390x/config.c                 | 113 ++++++++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c        |   6 ++
>  hw/s390x/s390-virtio.c            |  24 ++++++++
>  hw/s390x/s390-virtio.h            |   1 +
>  include/hw/s390x/config.h         |  51 +++++++++++++++++
>  linux-headers/asm-s390/kvm.h      |  11 ++++
>  linux-headers/linux/kvm.h         |   1 +
>  trace-events                      |   4 ++
>  10 files changed, 213 insertions(+)
>  create mode 100644 hw/s390x/config.c
>  create mode 100644 include/hw/s390x/config.h
>
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index d843dc0..a07697d 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -1,3 +1,4 @@
>  CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_S390_FLIC=$(CONFIG_KVM)
> +CONFIG_S390_CONFIG=$(CONFIG_KVM)
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 1ba6c3a..dc03e1d 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -8,3 +8,4 @@ obj-y += ipl.o
>  obj-y += css.o
>  obj-y += s390-virtio-ccw.o
>  obj-y += virtio-ccw.o
> +obj-$(CONFIG_S390_CONFIG) += config.o
> diff --git a/hw/s390x/config.c b/hw/s390x/config.c
> new file mode 100644
> index 0000000..f70e880
> --- /dev/null
> +++ b/hw/s390x/config.c
> @@ -0,0 +1,113 @@
> +/*
> + * S390 configuration and control device
> + *
> + * Copyright 2014 IBM Corp.
> + * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include <sys/ioctl.h>
> +#include "hw/s390x/config.h"
> +#include "trace.h"
> +
> +S390ConfigState *get_config_state(void)
> +{
> +    return S390_CONFIG(object_resolve_path("/machine/s390-config", NULL));
> +}
> +
> +static int config_set(S390ConfigState *config, struct kvm_device_attr *attr)
> +{
> +    int rc;
> +
> +    rc = ioctl(config->fd, KVM_SET_DEVICE_ATTR, attr);
> +    return rc < 0 ? -errno : rc;
> +}
> +
> +/*
> +static int config_get(S390ConfigState *config, struct kvm_device_attr *attr)
> +{
> +    int rc;
> +
> +    rc = ioctl(config->fd, KVM_GET_DEVICE_ATTR, attr);
> +    return rc < 0 ? -errno : rc;
> +}
> +*/
> +
> +static int config_set_name(S390ConfigState *config, char *name)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_DEV_CONFIG_GROUP,
> +        .attr  = KVM_DEV_CONFIG_NAME,
> +        .addr  = (uint64_t)name,
> +    };
> +
> +    if (!name ||
> +        (qemu_strnlen(name, S390_MACHINE_NAME_SIZE)
> +         >= S390_MACHINE_NAME_SIZE)) {
> +        return -EINVAL;
> +    }
> +
> +    return config_set(config, &attr);
> +}
> +
> +static void s390_config_realize(DeviceState *dev, Error **errp)
> +{
> +    S390ConfigState *cfg = S390_CONFIG(dev);
> +    struct kvm_create_device cd = {0};
> +    int ret;
> +
> +    if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
> +        trace_config_no_device_api(errno);
> +        return;
> +    }
> +
> +    cd.type = KVM_DEV_TYPE_S390_CONFIG;
> +    ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
> +    if (ret < 0) {
> +        trace_config_create_device(errno);
> +        return;
> +    }
> +    cfg->fd = cd.fd;
> +}
> +
> +void s390_config_dev_init(void)
> +{
> +    DeviceState *dev;
> +
> +    if (kvm_enabled()) {
> +        dev  = qdev_create(NULL, TYPE_S390_CONFIG);
> +
> +        object_property_add_child(qdev_get_machine(),
> +                                  TYPE_S390_CONFIG,
> +                                  OBJECT(dev), NULL);
> +        qdev_init_nofail(dev);
> +    }
> +}
> +
> +static void s390_config_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    S390ConfigClass *scc = S390_CONFIG_CLASS(oc);
> +
> +    dc->realize = s390_config_realize;
> +    scc->set_name = config_set_name;
> +}
> +
> +static const TypeInfo s390_config_info = {
> +    .name          = TYPE_S390_CONFIG,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(S390ConfigState),
> +    .class_init    = s390_config_class_init,
> +    .class_size    = sizeof(S390ConfigClass),
> +};
> +
> +
> +static void s390_config_register_types(void)
> +{
> +    type_register_static(&s390_config_info);
> +}
> +
> +type_init(s390_config_register_types)
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0d4f6ae..1e4b9ad 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -14,9 +14,11 @@
>  #include "s390-virtio.h"
>  #include "hw/s390x/sclp.h"
>  #include "hw/s390x/s390_flic.h"
> +#include "hw/s390x/config.h"
>  #include "ioinst.h"
>  #include "css.h"
>  #include "virtio-ccw.h"
> +#include "qemu/error-report.h"
>
>  void io_subsystem_reset(void)
>  {
> @@ -106,6 +108,10 @@ static void ccw_init(QEMUMachineInitArgs *args)
>                        args->initrd_filename, "s390-ccw.img");
>      s390_flic_init();
>
> +    s390_config_dev_init();
> +
> +    s390_set_vm_name();
> +
>      /* register hypercalls */
>      virtio_ccw_register_hcalls();
>
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index aef2003..7a296fd 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -39,6 +39,8 @@
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/s390-virtio.h"
>
> +#include "hw/s390x/config.h"
> +
>  //#define DEBUG_S390
>
>  #ifdef DEBUG_S390
> @@ -223,6 +225,24 @@ void s390_create_virtio_net(BusState *bus, const char *name)
>      }
>  }
>
> +void s390_set_vm_name(void)
> +{
> +    S390ConfigState *config = get_config_state();
> +    char machine_name[S390_MACHINE_NAME_SIZE];
> +    S390ConfigClass *scc;
> +
> +    if (!config || !qemu_name) {
> +        return;
> +    }
> +    strncpy(machine_name, qemu_name, sizeof(machine_name));
> +
> +    scc = S390_CONFIG_GET_CLASS(config);
> +
> +    if (scc->set_name(config, machine_name)) {
> +        error_report("Setting machine name failed: %m");
> +    }
> +}
> +
>  /* PC hardware initialisation */
>  static void s390_init(QEMUMachineInitArgs *args)
>  {
> @@ -252,6 +272,10 @@ static void s390_init(QEMUMachineInitArgs *args)
>                        args->initrd_filename, ZIPL_FILENAME);
>      s390_flic_init();
>
> +    s390_config_dev_init();
> +
> +    s390_set_vm_name();
> +
>      /* register hypercalls */
>      s390_virtio_register_hcalls();
>
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index 5c405e7..deca18e 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -26,4 +26,5 @@ void s390_init_ipl_dev(const char *kernel_filename,
>                         const char *initrd_filename,
>                         const char *firmware);
>  void s390_create_virtio_net(BusState *bus, const char *name);
> +void s390_set_vm_name(void);
>  #endif
> diff --git a/include/hw/s390x/config.h b/include/hw/s390x/config.h
> new file mode 100644
> index 0000000..14d9b02
> --- /dev/null
> +++ b/include/hw/s390x/config.h
> @@ -0,0 +1,51 @@
> +/*
> + * S390 configuration and control device
> + *
> + * Copyright 2014 IBM Corp.
> + * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef __KVM_S390_CONFIG_H
> +#define __KVM_S390_CONFIG_H
> +
> +#include "hw/sysbus.h"
> +#include "migration/qemu-file.h"
> +
> +#define TYPE_S390_CONFIG "s390-config"
> +#define S390_CONFIG(obj) \
> +    OBJECT_CHECK(S390ConfigState, (obj), TYPE_S390_CONFIG)
> +#define S390_CONFIG_CLASS(class) \
> +     OBJECT_CLASS_CHECK(S390ConfigClass, (class), TYPE_S390_CONFIG)
> +#define S390_CONFIG_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(S390ConfigClass, (obj), TYPE_S390_CONFIG)
> +
> +typedef struct S390ConfigState {
> +    SysBusDevice parent_obj;
> +    uint32_t fd;
> +} S390ConfigState;
> +
> +typedef struct S390ConfigClass {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +    /*< public >*/
> +    int (*set_name)(S390ConfigState *config, char *name);
> +} S390ConfigClass;
> +
> +#ifdef CONFIG_KVM
> +void s390_config_dev_init(void);
> +S390ConfigState *get_config_state(void);
> +#define S390_MACHINE_NAME_SIZE sizeof(struct kvm_s390_attr_name)
> +#else
> +static inline void s390_config_dev_init(void) { }
> +static inline S390ConfigState *get_config_state(void)
> +{
> +    return NULL;
> +}
> +#define S390_MACHINE_NAME_SIZE 128
> +#endif
> +
> +#endif /* __KVM_S390_CONFIG_H */
> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> index cb4c1eb..432ffa5 100644
> --- a/linux-headers/asm-s390/kvm.h
> +++ b/linux-headers/asm-s390/kvm.h
> @@ -57,6 +57,17 @@ struct kvm_debug_exit_arch {
>  struct kvm_guest_debug_arch {
>  };
>
> +
> +
> +/* config device specific GROUPS */
> +
> +#define KVM_DEV_CONFIG_GROUP		0
> +#define KVM_DEV_CONFIG_NAME		0
> +
> +struct kvm_s390_attr_name {
> +	char name[128];
> +};
> +
>  #define KVM_SYNC_PREFIX (1UL << 0)
>  #define KVM_SYNC_GPRS   (1UL << 1)
>  #define KVM_SYNC_ACRS   (1UL << 2)
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index e27a4b3..59d1ec3 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -921,6 +921,7 @@ struct kvm_device_attr {
>  #define   KVM_DEV_VFIO_GROUP_DEL			2
>  #define KVM_DEV_TYPE_ARM_VGIC_V2	5
>  #define KVM_DEV_TYPE_FLIC		6
> +#define KVM_DEV_TYPE_S390_CONFIG	7
>
>  /*
>   * ioctls for VM fds
> diff --git a/trace-events b/trace-events
> index 3df3f32..6ccd619 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1185,6 +1185,10 @@ css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc %x)"
>  virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command %x"
>  virtio_ccw_new_device(int cssid, int ssid, int schid, int devno, const char *devno_mode) "VIRTIO-CCW: add subchannel %x.%x.%04x, devno %04x (%s)"
>
> +# hw/s390x/config.c
> +config_no_device_api(int err) "config_dev: no Device Control API support %d"
> +config_create_device(int err) "config_dev: create device failed %d"
> +
>  # hw/intc/s390_flic.c
>  flic_create_device(int err) "flic: create device failed %d"
>  flic_no_device_api(int err) "flic: no Device Contral API support %d"
>

Re: [Qemu-devel] [PATCH/RFC] s390x/kvm: implement and use QEMU config device for s390

[Christian Borntraeger]

On 01/04/14 17:38, Paolo Bonzini wrote:
> Il 01/04/2014 16:47, Christian Borntraeger ha scritto:
>> From: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>>
>> The following patch adds Qemu CONFIG device,
>> which interacts with kvm CONFIG device by calling KVM_CREATE_DEVICE
>> (to create the device in kernel), KVM_GET_DEVICE_ATTR and
>> KVM_SET_DEVICE_ATTR (to get and set the particular attributes of
>> KVM CONFIG device respectively).
>> This patch uses the Qemu CONFIG device to copy the
>> qemu_name (if specified) into the kvm CONFIG device.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> On the QEMU side we'll soon have per-machine-type options in -machine. s390 can then use that.

Well we need some calls in the kernel at runtime (e.g. during reset), so we need more than
static options. (But we can use those for several things). This device was mostly meant as an achor for "doesnt fit into something else and would require a new
ioctl. Lets encapsulate everything in a way that follows the everything is a device approach with QOM and all these things".

Is there a branch for the per-machine-type things?

Christian


> 
> Paolo
> 
>> ---
>>  default-configs/s390x-softmmu.mak |   1 +
>>  hw/s390x/Makefile.objs            |   1 +
>>  hw/s390x/config.c                 | 113 ++++++++++++++++++++++++++++++++++++++
>>  hw/s390x/s390-virtio-ccw.c        |   6 ++
>>  hw/s390x/s390-virtio.c            |  24 ++++++++
>>  hw/s390x/s390-virtio.h            |   1 +
>>  include/hw/s390x/config.h         |  51 +++++++++++++++++
>>  linux-headers/asm-s390/kvm.h      |  11 ++++
>>  linux-headers/linux/kvm.h         |   1 +
>>  trace-events                      |   4 ++
>>  10 files changed, 213 insertions(+)
>>  create mode 100644 hw/s390x/config.c
>>  create mode 100644 include/hw/s390x/config.h
>>
>> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
>> index d843dc0..a07697d 100644
>> --- a/default-configs/s390x-softmmu.mak
>> +++ b/default-configs/s390x-softmmu.mak
>> @@ -1,3 +1,4 @@
>>  CONFIG_VIRTIO=y
>>  CONFIG_SCLPCONSOLE=y
>>  CONFIG_S390_FLIC=$(CONFIG_KVM)
>> +CONFIG_S390_CONFIG=$(CONFIG_KVM)
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index 1ba6c3a..dc03e1d 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -8,3 +8,4 @@ obj-y += ipl.o
>>  obj-y += css.o
>>  obj-y += s390-virtio-ccw.o
>>  obj-y += virtio-ccw.o
>> +obj-$(CONFIG_S390_CONFIG) += config.o
>> diff --git a/hw/s390x/config.c b/hw/s390x/config.c
>> new file mode 100644
>> index 0000000..f70e880
>> --- /dev/null
>> +++ b/hw/s390x/config.c
>> @@ -0,0 +1,113 @@
>> +/*
>> + * S390 configuration and control device
>> + *
>> + * Copyright 2014 IBM Corp.
>> + * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include <sys/ioctl.h>
>> +#include "hw/s390x/config.h"
>> +#include "trace.h"
>> +
>> +S390ConfigState *get_config_state(void)
>> +{
>> +    return S390_CONFIG(object_resolve_path("/machine/s390-config", NULL));
>> +}
>> +
>> +static int config_set(S390ConfigState *config, struct kvm_device_attr *attr)
>> +{
>> +    int rc;
>> +
>> +    rc = ioctl(config->fd, KVM_SET_DEVICE_ATTR, attr);
>> +    return rc < 0 ? -errno : rc;
>> +}
>> +
>> +/*
>> +static int config_get(S390ConfigState *config, struct kvm_device_attr *attr)
>> +{
>> +    int rc;
>> +
>> +    rc = ioctl(config->fd, KVM_GET_DEVICE_ATTR, attr);
>> +    return rc < 0 ? -errno : rc;
>> +}
>> +*/
>> +
>> +static int config_set_name(S390ConfigState *config, char *name)
>> +{
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_DEV_CONFIG_GROUP,
>> +        .attr  = KVM_DEV_CONFIG_NAME,
>> +        .addr  = (uint64_t)name,
>> +    };
>> +
>> +    if (!name ||
>> +        (qemu_strnlen(name, S390_MACHINE_NAME_SIZE)
>> +         >= S390_MACHINE_NAME_SIZE)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    return config_set(config, &attr);
>> +}
>> +
>> +static void s390_config_realize(DeviceState *dev, Error **errp)
>> +{
>> +    S390ConfigState *cfg = S390_CONFIG(dev);
>> +    struct kvm_create_device cd = {0};
>> +    int ret;
>> +
>> +    if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>> +        trace_config_no_device_api(errno);
>> +        return;
>> +    }
>> +
>> +    cd.type = KVM_DEV_TYPE_S390_CONFIG;
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
>> +    if (ret < 0) {
>> +        trace_config_create_device(errno);
>> +        return;
>> +    }
>> +    cfg->fd = cd.fd;
>> +}
>> +
>> +void s390_config_dev_init(void)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (kvm_enabled()) {
>> +        dev  = qdev_create(NULL, TYPE_S390_CONFIG);
>> +
>> +        object_property_add_child(qdev_get_machine(),
>> +                                  TYPE_S390_CONFIG,
>> +                                  OBJECT(dev), NULL);
>> +        qdev_init_nofail(dev);
>> +    }
>> +}
>> +
>> +static void s390_config_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    S390ConfigClass *scc = S390_CONFIG_CLASS(oc);
>> +
>> +    dc->realize = s390_config_realize;
>> +    scc->set_name = config_set_name;
>> +}
>> +
>> +static const TypeInfo s390_config_info = {
>> +    .name          = TYPE_S390_CONFIG,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(S390ConfigState),
>> +    .class_init    = s390_config_class_init,
>> +    .class_size    = sizeof(S390ConfigClass),
>> +};
>> +
>> +
>> +static void s390_config_register_types(void)
>> +{
>> +    type_register_static(&s390_config_info);
>> +}
>> +
>> +type_init(s390_config_register_types)
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 0d4f6ae..1e4b9ad 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -14,9 +14,11 @@
>>  #include "s390-virtio.h"
>>  #include "hw/s390x/sclp.h"
>>  #include "hw/s390x/s390_flic.h"
>> +#include "hw/s390x/config.h"
>>  #include "ioinst.h"
>>  #include "css.h"
>>  #include "virtio-ccw.h"
>> +#include "qemu/error-report.h"
>>
>>  void io_subsystem_reset(void)
>>  {
>> @@ -106,6 +108,10 @@ static void ccw_init(QEMUMachineInitArgs *args)
>>                        args->initrd_filename, "s390-ccw.img");
>>      s390_flic_init();
>>
>> +    s390_config_dev_init();
>> +
>> +    s390_set_vm_name();
>> +
>>      /* register hypercalls */
>>      virtio_ccw_register_hcalls();
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index aef2003..7a296fd 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -39,6 +39,8 @@
>>  #include "hw/s390x/s390_flic.h"
>>  #include "hw/s390x/s390-virtio.h"
>>
>> +#include "hw/s390x/config.h"
>> +
>>  //#define DEBUG_S390
>>
>>  #ifdef DEBUG_S390
>> @@ -223,6 +225,24 @@ void s390_create_virtio_net(BusState *bus, const char *name)
>>      }
>>  }
>>
>> +void s390_set_vm_name(void)
>> +{
>> +    S390ConfigState *config = get_config_state();
>> +    char machine_name[S390_MACHINE_NAME_SIZE];
>> +    S390ConfigClass *scc;
>> +
>> +    if (!config || !qemu_name) {
>> +        return;
>> +    }
>> +    strncpy(machine_name, qemu_name, sizeof(machine_name));
>> +
>> +    scc = S390_CONFIG_GET_CLASS(config);
>> +
>> +    if (scc->set_name(config, machine_name)) {
>> +        error_report("Setting machine name failed: %m");
>> +    }
>> +}
>> +
>>  /* PC hardware initialisation */
>>  static void s390_init(QEMUMachineInitArgs *args)
>>  {
>> @@ -252,6 +272,10 @@ static void s390_init(QEMUMachineInitArgs *args)
>>                        args->initrd_filename, ZIPL_FILENAME);
>>      s390_flic_init();
>>
>> +    s390_config_dev_init();
>> +
>> +    s390_set_vm_name();
>> +
>>      /* register hypercalls */
>>      s390_virtio_register_hcalls();
>>
>> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
>> index 5c405e7..deca18e 100644
>> --- a/hw/s390x/s390-virtio.h
>> +++ b/hw/s390x/s390-virtio.h
>> @@ -26,4 +26,5 @@ void s390_init_ipl_dev(const char *kernel_filename,
>>                         const char *initrd_filename,
>>                         const char *firmware);
>>  void s390_create_virtio_net(BusState *bus, const char *name);
>> +void s390_set_vm_name(void);
>>  #endif
>> diff --git a/include/hw/s390x/config.h b/include/hw/s390x/config.h
>> new file mode 100644
>> index 0000000..14d9b02
>> --- /dev/null
>> +++ b/include/hw/s390x/config.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * S390 configuration and control device
>> + *
>> + * Copyright 2014 IBM Corp.
>> + * Author(s): Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#ifndef __KVM_S390_CONFIG_H
>> +#define __KVM_S390_CONFIG_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "migration/qemu-file.h"
>> +
>> +#define TYPE_S390_CONFIG "s390-config"
>> +#define S390_CONFIG(obj) \
>> +    OBJECT_CHECK(S390ConfigState, (obj), TYPE_S390_CONFIG)
>> +#define S390_CONFIG_CLASS(class) \
>> +     OBJECT_CLASS_CHECK(S390ConfigClass, (class), TYPE_S390_CONFIG)
>> +#define S390_CONFIG_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(S390ConfigClass, (obj), TYPE_S390_CONFIG)
>> +
>> +typedef struct S390ConfigState {
>> +    SysBusDevice parent_obj;
>> +    uint32_t fd;
>> +} S390ConfigState;
>> +
>> +typedef struct S390ConfigClass {
>> +    /*< private >*/
>> +    SysBusDeviceClass parent_class;
>> +    /*< public >*/
>> +    int (*set_name)(S390ConfigState *config, char *name);
>> +} S390ConfigClass;
>> +
>> +#ifdef CONFIG_KVM
>> +void s390_config_dev_init(void);
>> +S390ConfigState *get_config_state(void);
>> +#define S390_MACHINE_NAME_SIZE sizeof(struct kvm_s390_attr_name)
>> +#else
>> +static inline void s390_config_dev_init(void) { }
>> +static inline S390ConfigState *get_config_state(void)
>> +{
>> +    return NULL;
>> +}
>> +#define S390_MACHINE_NAME_SIZE 128
>> +#endif
>> +
>> +#endif /* __KVM_S390_CONFIG_H */
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index cb4c1eb..432ffa5 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -57,6 +57,17 @@ struct kvm_debug_exit_arch {
>>  struct kvm_guest_debug_arch {
>>  };
>>
>> +
>> +
>> +/* config device specific GROUPS */
>> +
>> +#define KVM_DEV_CONFIG_GROUP        0
>> +#define KVM_DEV_CONFIG_NAME        0
>> +
>> +struct kvm_s390_attr_name {
>> +    char name[128];
>> +};
>> +
>>  #define KVM_SYNC_PREFIX (1UL << 0)
>>  #define KVM_SYNC_GPRS   (1UL << 1)
>>  #define KVM_SYNC_ACRS   (1UL << 2)
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index e27a4b3..59d1ec3 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -921,6 +921,7 @@ struct kvm_device_attr {
>>  #define   KVM_DEV_VFIO_GROUP_DEL            2
>>  #define KVM_DEV_TYPE_ARM_VGIC_V2    5
>>  #define KVM_DEV_TYPE_FLIC        6
>> +#define KVM_DEV_TYPE_S390_CONFIG    7
>>
>>  /*
>>   * ioctls for VM fds
>> diff --git a/trace-events b/trace-events
>> index 3df3f32..6ccd619 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1185,6 +1185,10 @@ css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc %x)"
>>  virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command %x"
>>  virtio_ccw_new_device(int cssid, int ssid, int schid, int devno, const char *devno_mode) "VIRTIO-CCW: add subchannel %x.%x.%04x, devno %04x (%s)"
>>
>> +# hw/s390x/config.c
>> +config_no_device_api(int err) "config_dev: no Device Control API support %d"
>> +config_create_device(int err) "config_dev: create device failed %d"
>> +
>>  # hw/intc/s390_flic.c
>>  flic_create_device(int err) "flic: create device failed %d"
>>  flic_no_device_api(int err) "flic: no Device Contral API support %d"
>>
> 

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Alexander Graf]

On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
> We want to configure several things in KVM that go beyond what
> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
> and we need to do more complex actions) can provide. Instead of
> adding several s390 specific ioctls, lets provide a configuration
> and control device that encapsulates different commands into
> groups of the same area (MEMORY, CPU, ..)
>
> We also provide an initial nameless base group, with a simple first
> user to set the guest name. We need that name in the kernel for
> the emulation of STSI (which provides the guest name to the guest)
> but we need to implement the emulation in supervisor mode, as it
> also provides the underlying levels of hipervisors.
>
> Currently we have the following GROUPS and ATTRs pending, which
> configure some memory management related function or allow to set
> the guest facilities, cpuids etc:
>
> #define KVM_DEV_CONFIG_GROUP            0
> #define KVM_DEV_CONFIG_NAME             0
>
> #define KVM_DEV_CONFIG_GROUP_MEM        1
> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
>
> #define KVM_DEV_CONFIG_GROUP_CPU        2
> #define KVM_DEV_CONFIG_CPU_TYPE         0
> #define KVM_DEV_CONFIG_CPU_FAC          1
> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
> #define KVM_DEV_CONFIG_CPU_IBC          3
> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4

Why would CPU specific information be set in the VM?


Alex

>
>
>
> In addition other groups like
> #define KVM_DEV_CONFIG_GROUP_CRYPTO
> are under consideration to configure crypto acceleration.
>
> Unless there is a major concern, I will add this to the next
> s390 PULL requests for KVM.
>
> Christian
>

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Christian Borntraeger]

On 01/04/14 16:59, Alexander Graf wrote:
> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>> We want to configure several things in KVM that go beyond what
>> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
>> and we need to do more complex actions) can provide. Instead of
>> adding several s390 specific ioctls, lets provide a configuration
>> and control device that encapsulates different commands into
>> groups of the same area (MEMORY, CPU, ..)
>>
>> We also provide an initial nameless base group, with a simple first
>> user to set the guest name. We need that name in the kernel for
>> the emulation of STSI (which provides the guest name to the guest)
>> but we need to implement the emulation in supervisor mode, as it
>> also provides the underlying levels of hipervisors.
>>
>> Currently we have the following GROUPS and ATTRs pending, which
>> configure some memory management related function or allow to set
>> the guest facilities, cpuids etc:
>>
>> #define KVM_DEV_CONFIG_GROUP            0
>> #define KVM_DEV_CONFIG_NAME             0
>>
>> #define KVM_DEV_CONFIG_GROUP_MEM        1
>> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
>> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
>> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
>>
>> #define KVM_DEV_CONFIG_GROUP_CPU        2
>> #define KVM_DEV_CONFIG_CPU_TYPE         0
>> #define KVM_DEV_CONFIG_CPU_FAC          1
>> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
>> #define KVM_DEV_CONFIG_CPU_IBC          3
>> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4
> 
> Why would CPU specific information be set in the VM?

Might be a misleading name here. This is about CPU id and facility list (mostly CPU features).
The list of facilities and the cpu id is unique on VM level. It is queried via a CPU instruction, though. Providing some VM-wide setting via a CPU interface would make the interface look like the user can have different settings per-CPU. Since we cant, its better to make this explicit.
> 
> 
> Alex
> 
>>
>>
>>
>> In addition other groups like
>> #define KVM_DEV_CONFIG_GROUP_CRYPTO
>> are under consideration to configure crypto acceleration.
>>
>> Unless there is a major concern, I will add this to the next
>> s390 PULL requests for KVM.
>>
>> Christian
>>
> 

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Alexander Graf]

On 01.04.2014, at 21:23, Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/04/14 16:59, Alexander Graf wrote:
>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>> We want to configure several things in KVM that go beyond what
>>> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
>>> and we need to do more complex actions) can provide. Instead of
>>> adding several s390 specific ioctls, lets provide a configuration
>>> and control device that encapsulates different commands into
>>> groups of the same area (MEMORY, CPU, ..)
>>> 
>>> We also provide an initial nameless base group, with a simple first
>>> user to set the guest name. We need that name in the kernel for
>>> the emulation of STSI (which provides the guest name to the guest)
>>> but we need to implement the emulation in supervisor mode, as it
>>> also provides the underlying levels of hipervisors.
>>> 
>>> Currently we have the following GROUPS and ATTRs pending, which
>>> configure some memory management related function or allow to set
>>> the guest facilities, cpuids etc:
>>> 
>>> #define KVM_DEV_CONFIG_GROUP            0
>>> #define KVM_DEV_CONFIG_NAME             0
>>> 
>>> #define KVM_DEV_CONFIG_GROUP_MEM        1
>>> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
>>> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
>>> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
>>> 
>>> #define KVM_DEV_CONFIG_GROUP_CPU        2
>>> #define KVM_DEV_CONFIG_CPU_TYPE         0
>>> #define KVM_DEV_CONFIG_CPU_FAC          1
>>> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
>>> #define KVM_DEV_CONFIG_CPU_IBC          3
>>> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4
>> 
>> Why would CPU specific information be set in the VM?
> 
> Might be a misleading name here. This is about CPU id and facility list (mostly CPU features).
> The list of facilities and the cpu id is unique on VM level.

Is this a hardware limitation? I thought s390's virtualization instruction doesn't have a notion of a VM, but I could easily be wrong.


Alex

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Christian Borntraeger]

On 01/04/14 16:47, Christian Borntraeger wrote:
> We want to configure several things in KVM that go beyond what
> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
> and we need to do more complex actions) can provide. Instead of
> adding several s390 specific ioctls, lets provide a configuration
> and control device that encapsulates different commands into
> groups of the same area (MEMORY, CPU, ..)
> 
> We also provide an initial nameless base group, with a simple first
> user to set the guest name. We need that name in the kernel for
> the emulation of STSI (which provides the guest name to the guest)
> but we need to implement the emulation in supervisor mode, as it
> also provides the underlying levels of hipervisors.
> 
> Currently we have the following GROUPS and ATTRs pending, which
> configure some memory management related function or allow to set
> the guest facilities, cpuids etc:
> 
> #define KVM_DEV_CONFIG_GROUP            0
> #define KVM_DEV_CONFIG_NAME             0
> 
> #define KVM_DEV_CONFIG_GROUP_MEM        1
> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
> 
> #define KVM_DEV_CONFIG_GROUP_CPU        2
> #define KVM_DEV_CONFIG_CPU_TYPE         0
> #define KVM_DEV_CONFIG_CPU_FAC          1
> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
> #define KVM_DEV_CONFIG_CPU_IBC          3
> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4
> 
> 
> 
> In addition other groups like 
> #define KVM_DEV_CONFIG_GROUP_CRYPTO 
> are under consideration to configure crypto acceleration.
> 
> Unless there is a major concern, I will add this to the next
> s390 PULL requests for KVM.
> 
> Christian
> 

@Alex,

regarding STSI, what about the following:

The kernel will fill in every part of STSI as of now, but we will provide a CAP that qemu can enable which then tells the kernel to pass control to QEMU after it has filled in the data. QEMU can then do nothing (e.g. for stsi 111) or change
the information (e.g. for 322) and return to kernel. That would
a: cover the name aspect
b: will work with future enhancements for levels 1 and 2 since the kernel will still pass that through to the HW or LPAR
c: allows QEMU to override everything if necessary


@Paolo, Alex,

I have several changes pending that will require new ioctls. I planned to use the config device to avoid creating new ioctls. Some option:
a: define new ioctls for these things (might end up with ~10 new ioctls)
b: allow GET_ATTR/SET_ATTR on the vm fd. We would define those as architecture specific attributes of the VM.
c: use a config device an anchor for GET_ATTR/SET_ATTR
d: any better idea

This question is really a bike shed color discussion (which interface for specific thing between qemu/kvm is considered best), but it will be ABI. Paolo, do you have any preference?
I dont care about which solution we choose, but I obviously need a decision to rework pending patches.

Christian

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Alexander Graf]

On 02.04.14 10:31, Christian Borntraeger wrote:
> On 01/04/14 16:47, Christian Borntraeger wrote:
>> We want to configure several things in KVM that go beyond what
>> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
>> and we need to do more complex actions) can provide. Instead of
>> adding several s390 specific ioctls, lets provide a configuration
>> and control device that encapsulates different commands into
>> groups of the same area (MEMORY, CPU, ..)
>>
>> We also provide an initial nameless base group, with a simple first
>> user to set the guest name. We need that name in the kernel for
>> the emulation of STSI (which provides the guest name to the guest)
>> but we need to implement the emulation in supervisor mode, as it
>> also provides the underlying levels of hipervisors.
>>
>> Currently we have the following GROUPS and ATTRs pending, which
>> configure some memory management related function or allow to set
>> the guest facilities, cpuids etc:
>>
>> #define KVM_DEV_CONFIG_GROUP            0
>> #define KVM_DEV_CONFIG_NAME             0
>>
>> #define KVM_DEV_CONFIG_GROUP_MEM        1
>> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
>> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
>> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
>>
>> #define KVM_DEV_CONFIG_GROUP_CPU        2
>> #define KVM_DEV_CONFIG_CPU_TYPE         0
>> #define KVM_DEV_CONFIG_CPU_FAC          1
>> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
>> #define KVM_DEV_CONFIG_CPU_IBC          3
>> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4
>>
>>
>>
>> In addition other groups like
>> #define KVM_DEV_CONFIG_GROUP_CRYPTO
>> are under consideration to configure crypto acceleration.
>>
>> Unless there is a major concern, I will add this to the next
>> s390 PULL requests for KVM.
>>
>> Christian
>>
> @Alex,
>
> regarding STSI, what about the following:
>
> The kernel will fill in every part of STSI as of now, but we will provide a CAP that qemu can enable which then tells the kernel to pass control to QEMU after it has filled in the data. QEMU can then do nothing (e.g. for stsi 111) or change
> the information (e.g. for 322) and return to kernel. That would
> a: cover the name aspect
> b: will work with future enhancements for levels 1 and 2 since the kernel will still pass that through to the HW or LPAR
> c: allows QEMU to override everything if necessary

I like that one, yes. Does the STSI payload fit into our exit payload union?

> @Paolo, Alex,
>
> I have several changes pending that will require new ioctls. I planned to use the config device to avoid creating new ioctls. Some option:
> a: define new ioctls for these things (might end up with ~10 new ioctls)
> b: allow GET_ATTR/SET_ATTR on the vm fd. We would define those as architecture specific attributes of the VM.
> c: use a config device an anchor for GET_ATTR/SET_ATTR
> d: any better idea
>
> This question is really a bike shed color discussion (which interface for specific thing between qemu/kvm is considered best), but it will be ABI. Paolo, do you have any preference?
> I dont care about which solution we choose, but I obviously need a decision to rework pending patches.

I think it semantically makes a lot of sense to treat the VM fd as an 
"implements device" class :). So allowing GET_ATTR/SET_ATTR on the VM fd 
makes most sense to me.

As for all the individual bits you need to set, I'd like to make sure we 
talk about them on a case-by-case basis. As you've seen with STSI, other 
solutions might be a better fit.


Alex

Re: [Qemu-devel] [PATCH/RFC] s390: Provide a configuration and control device

[Christian Borntraeger]

On 02/04/14 10:39, Alexander Graf wrote:
> 
> On 02.04.14 10:31, Christian Borntraeger wrote:
>> On 01/04/14 16:47, Christian Borntraeger wrote:
>>> We want to configure several things in KVM that go beyond what
>>> ENABLE_CAP (we need payload) or ONE_REG (we need it for the VM
>>> and we need to do more complex actions) can provide. Instead of
>>> adding several s390 specific ioctls, lets provide a configuration
>>> and control device that encapsulates different commands into
>>> groups of the same area (MEMORY, CPU, ..)
>>>
>>> We also provide an initial nameless base group, with a simple first
>>> user to set the guest name. We need that name in the kernel for
>>> the emulation of STSI (which provides the guest name to the guest)
>>> but we need to implement the emulation in supervisor mode, as it
>>> also provides the underlying levels of hipervisors.
>>>
>>> Currently we have the following GROUPS and ATTRs pending, which
>>> configure some memory management related function or allow to set
>>> the guest facilities, cpuids etc:
>>>
>>> #define KVM_DEV_CONFIG_GROUP            0
>>> #define KVM_DEV_CONFIG_NAME             0
>>>
>>> #define KVM_DEV_CONFIG_GROUP_MEM        1
>>> #define KVM_DEV_CONFIG_MEM_ENABLE_CMMA  0
>>> #define KVM_DEV_CONFIG_MEM_CLR_CMMA     1
>>> #define KVM_DEV_CONFIG_MEM_CLR_PAGES    2
>>>
>>> #define KVM_DEV_CONFIG_GROUP_CPU        2
>>> #define KVM_DEV_CONFIG_CPU_TYPE         0
>>> #define KVM_DEV_CONFIG_CPU_FAC          1
>>> #define KVM_DEV_CONFIG_CPU_FAC_MASK     2
>>> #define KVM_DEV_CONFIG_CPU_IBC          3
>>> #define KVM_DEV_CONFIG_CPU_IBC_RANGE    4
>>>
>>>
>>>
>>> In addition other groups like
>>> #define KVM_DEV_CONFIG_GROUP_CRYPTO
>>> are under consideration to configure crypto acceleration.
>>>
>>> Unless there is a major concern, I will add this to the next
>>> s390 PULL requests for KVM.
>>>
>>> Christian
>>>
>> @Alex,
>>
>> regarding STSI, what about the following:
>>
>> The kernel will fill in every part of STSI as of now, but we will provide a CAP that qemu can enable which then tells the kernel to pass control to QEMU after it has filled in the data. QEMU can then do nothing (e.g. for stsi 111) or change
>> the information (e.g. for 322) and return to kernel. That would
>> a: cover the name aspect
>> b: will work with future enhancements for levels 1 and 2 since the kernel will still pass that through to the HW or LPAR
>> c: allows QEMU to override everything if necessary
> 
> I like that one, yes. Does the STSI payload fit into our exit payload union?

No, its up to 4k. So the STSI exit would need Function code, selector1, and selector2, as well as guest logical address.
So something like:

index a8f4ee5..3ea5727 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -297,6 +297,13 @@ struct kvm_run {
                        __u32 ipb;
                        __u8 dequeued;
                } s390_tsch;
+               /* KVM_EXIT_S390_STSI */
+               struct {
+                       __u64 addr;
+                       __u8 fc;
+                       __u8 sel1;
+                       __u16 sel2;
+               } s390_stsi;
                /* KVM_EXIT_EPR */
                struct {
                        __u32 epr;

 
>> @Paolo, Alex,
>>
>> I have several changes pending that will require new ioctls. I planned to use the config device to avoid creating new ioctls. Some option:
>> a: define new ioctls for these things (might end up with ~10 new ioctls)
>> b: allow GET_ATTR/SET_ATTR on the vm fd. We would define those as architecture specific attributes of the VM.
>> c: use a config device an anchor for GET_ATTR/SET_ATTR
>> d: any better idea
>>
>> This question is really a bike shed color discussion (which interface for specific thing between qemu/kvm is considered best), but it will be ABI. Paolo, do you have any preference?
>> I dont care about which solution we choose, but I obviously need a decision to rework pending patches.
> 
> I think it semantically makes a lot of sense to treat the VM fd as an "implements device" class :). So allowing GET_ATTR/SET_ATTR on the VM fd makes most sense to me.

Paolo, are you ok with allowing GET_ATTR/SET_ATTR/HAS_ATTR on the vm fd. All Attributes are then architecture specific. We might also reserve a group (e.g. 0) to be cross architecture.

 
> As for all the individual bits you need to set, I'd like to make sure we talk about them on a case-by-case basis. As you've seen with STSI, other solutions might be a better fit.

Absolutely. There is a reason why this patch has an RFC and not a PULL.

Christian