[v5,11/17] target-s390x: Add KVM VM attribute interface for S390 CPU models

Commit Message

Michael Mueller April 13, 2015, 1:56 p.m. UTC

The patch implements routines to set and retrieve processor configuration
data and to retrieve machine configuration data. The machine related data
is used together with the CPU model facility lists to determine the list of
supported CPU models of this host. The above mentioned routines have QEMU
trace point instrumentation.

Signed-off-by: Michael Mueller <[e-mail]>
---
 target-s390x/cpu-models.h | 36 ++++++++++++++++++++-
 target-s390x/kvm.c        | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 trace-events              |  3 ++
 3 files changed, 117 insertions(+), 1 deletion(-)

Comments

Christian Borntraeger April 27, 2015, 8:15 a.m. UTC | #1

Am 13.04.2015 um 15:56 schrieb Michael Mueller:
[...]
> +static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
> +{
> +    int rc = -ENOSYS;
> +    struct kvm_device_attr dev_attr = {
> +        .group = KVM_S390_VM_CPU_MODEL,
> +        .attr = attr,
> +        .addr = addr,

Would it make sense to do the cast here....
> +    };
> +
> +    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
> +        rc = kvm_vm_ioctl(s, KVM_GET_DEVICE_ATTR, &dev_attr);
> +    }
> +
> +    return rc;
> +}
> +
> +static int cpu_model_set(KVMState *s, uint64_t attr, uint64_t addr)
> +{
> +    int rc = -ENOSYS;
> +    struct kvm_device_attr dev_attr = {
> +        .group = KVM_S390_VM_CPU_MODEL,
> +        .attr = attr,
> +        .addr = addr,

...and here...

> +    };
> +
> +    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
> +        rc = kvm_vm_ioctl(s, KVM_SET_DEVICE_ATTR, &dev_attr);
> +    }
> +
> +    return rc;
> +}
> +
> +static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> +{
> +    int rc = -EFAULT;
> +
> +    if (s) {
> +        rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, (uint64_t) prop);

and pass S390MachineProps as 3rd parameter?


> +int kvm_s390_get_processor_props(S390ProcessorProps *prop)
> +{
> +    int rc;
> +
> +    rc = cpu_model_get(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);

dito
> +    trace_kvm_get_processor_props(rc, prop->cpuid, prop->ibc);
> +    return rc;
> +}
> +
> +int kvm_s390_set_processor_props(S390ProcessorProps *prop)
> +{
> +    int rc;
> +
> +    rc = cpu_model_set(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);

dito

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Mueller April 27, 2015, 9:43 a.m. UTC | #2

On Mon, 27 Apr 2015 10:15:47 +0200
Christian Borntraeger <[e-mail]> wrote:

> Am 13.04.2015 um 15:56 schrieb Michael Mueller:
> [...]
> > +static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
> > +{
> > +    int rc = -ENOSYS;
> > +    struct kvm_device_attr dev_attr = {
> > +        .group = KVM_S390_VM_CPU_MODEL,
> > +        .attr = attr,
> > +        .addr = addr,
> 
> Would it make sense to do the cast here....

cpu_model_get/set() is used to handle both attributes,
KVM_S390_VM_CPU_MACHINE and KVM_S390_VM_CPU_PROCESSOR.
Both require a different type in the signature, (S390ProcessorProps*)
and (S390MachineProps*). Adding both as parameters seems to be odd
and would require additionally logic in the function.
Thus I think doing the cast outside is just the right thing to do.

Michael

> > +    };
> > +
> > +    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
> > +        rc = kvm_vm_ioctl(s, KVM_GET_DEVICE_ATTR, &dev_attr);
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> > +static int cpu_model_set(KVMState *s, uint64_t attr, uint64_t addr)
> > +{
> > +    int rc = -ENOSYS;
> > +    struct kvm_device_attr dev_attr = {
> > +        .group = KVM_S390_VM_CPU_MODEL,
> > +        .attr = attr,
> > +        .addr = addr,
> 
> ...and here...
> 
> > +    };
> > +
> > +    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
> > +        rc = kvm_vm_ioctl(s, KVM_SET_DEVICE_ATTR, &dev_attr);
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> > +static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> > +{
> > +    int rc = -EFAULT;
> > +
> > +    if (s) {
> > +        rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, (uint64_t) prop);
> 
> and pass S390MachineProps as 3rd parameter?
> 
> 
> > +int kvm_s390_get_processor_props(S390ProcessorProps *prop)
> > +{
> > +    int rc;
> > +
> > +    rc = cpu_model_get(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);
> 
> dito
> > +    trace_kvm_get_processor_props(rc, prop->cpuid, prop->ibc);
> > +    return rc;
> > +}
> > +
> > +int kvm_s390_set_processor_props(S390ProcessorProps *prop)
> > +{
> > +    int rc;
> > +
> > +    rc = cpu_model_set(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);
> 
> dito

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christian Borntraeger April 27, 2015, 10:52 a.m. UTC | #3

Am 27.04.2015 um 11:43 schrieb Michael Mueller:
> On Mon, 27 Apr 2015 10:15:47 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
>> Am 13.04.2015 um 15:56 schrieb Michael Mueller:
>> [...]
>>> +static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
>>> +{
>>> +    int rc = -ENOSYS;
>>> +    struct kvm_device_attr dev_attr = {
>>> +        .group = KVM_S390_VM_CPU_MODEL,
>>> +        .attr = attr,
>>> +        .addr = addr,
>>
>> Would it make sense to do the cast here....
> 
> cpu_model_get/set() is used to handle both attributes,
> KVM_S390_VM_CPU_MACHINE and KVM_S390_VM_CPU_PROCESSOR.
> Both require a different type in the signature, (S390ProcessorProps*)
> and (S390MachineProps*). Adding both as parameters seems to be odd
> and would require additionally logic in the function.
> Thus I think doing the cast outside is just the right thing to do.

So what about a void pointer then as parameter?
I prefer a pointer for qemu process memory over uint64_t as part of the 
function interface. This makes it somewhat clearer that this is an
address within QEMU. Both ways will certainly work, though.

Conny, I guess you will pick up the patches. Any preference?

Christian

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Mueller April 27, 2015, 11:07 a.m. UTC | #4

On Mon, 27 Apr 2015 12:52:54 +0200
Christian Borntraeger <[e-mail]> wrote:

> Am 27.04.2015 um 11:43 schrieb Michael Mueller:
> > On Mon, 27 Apr 2015 10:15:47 +0200
> > Christian Borntraeger <[e-mail]> wrote:
> > 
> >> Am 13.04.2015 um 15:56 schrieb Michael Mueller:
> >> [...]
> >>> +static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
> >>> +{
> >>> +    int rc = -ENOSYS;
> >>> +    struct kvm_device_attr dev_attr = {
> >>> +        .group = KVM_S390_VM_CPU_MODEL,
> >>> +        .attr = attr,
> >>> +        .addr = addr,
> >>
> >> Would it make sense to do the cast here....
> > 
> > cpu_model_get/set() is used to handle both attributes,
> > KVM_S390_VM_CPU_MACHINE and KVM_S390_VM_CPU_PROCESSOR.
> > Both require a different type in the signature, (S390ProcessorProps*)
> > and (S390MachineProps*). Adding both as parameters seems to be odd
> > and would require additionally logic in the function.
> > Thus I think doing the cast outside is just the right thing to do.
> 
> So what about a void pointer then as parameter?
> I prefer a pointer for qemu process memory over uint64_t as part of the 
> function interface. This makes it somewhat clearer that this is an
> address within QEMU. Both ways will certainly work, though.

The interface calls are:

int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
int kvm_s390_get_processor_props(S390ProcessorProps *prop)

cpu_model_get/set() are just static helpers.

> 
> Conny, I guess you will pick up the patches. Any preference?
> 
> Christian

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cornelia Huck April 27, 2015, 12:19 p.m. UTC | #5

On Mon, 27 Apr 2015 13:07:58 +0200
Michael Mueller <[e-mail]> wrote:

> On Mon, 27 Apr 2015 12:52:54 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
> > Am 27.04.2015 um 11:43 schrieb Michael Mueller:
> > > On Mon, 27 Apr 2015 10:15:47 +0200
> > > Christian Borntraeger <[e-mail]> wrote:
> > > 
> > >> Am 13.04.2015 um 15:56 schrieb Michael Mueller:
> > >> [...]
> > >>> +static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
> > >>> +{
> > >>> +    int rc = -ENOSYS;
> > >>> +    struct kvm_device_attr dev_attr = {
> > >>> +        .group = KVM_S390_VM_CPU_MODEL,
> > >>> +        .attr = attr,
> > >>> +        .addr = addr,
> > >>
> > >> Would it make sense to do the cast here....
> > > 
> > > cpu_model_get/set() is used to handle both attributes,
> > > KVM_S390_VM_CPU_MACHINE and KVM_S390_VM_CPU_PROCESSOR.
> > > Both require a different type in the signature, (S390ProcessorProps*)
> > > and (S390MachineProps*). Adding both as parameters seems to be odd
> > > and would require additionally logic in the function.
> > > Thus I think doing the cast outside is just the right thing to do.
> > 
> > So what about a void pointer then as parameter?
> > I prefer a pointer for qemu process memory over uint64_t as part of the 
> > function interface. This makes it somewhat clearer that this is an
> > address within QEMU. Both ways will certainly work, though.
> 
> The interface calls are:
> 
> int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> int kvm_s390_get_processor_props(S390ProcessorProps *prop)
> 
> cpu_model_get/set() are just static helpers.

So this makes them internal calls...

> 
> > 
> > Conny, I guess you will pick up the patches. Any preference?

...and I'd prefer using a void pointer for them.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Mueller April 27, 2015, 1:01 p.m. UTC | #6

On Mon, 27 Apr 2015 14:19:13 +0200
Cornelia Huck <[e-mail]> wrote:

> > > >> Would it make sense to do the cast here....  
> > > > 
> > > > cpu_model_get/set() is used to handle both attributes,
> > > > KVM_S390_VM_CPU_MACHINE and KVM_S390_VM_CPU_PROCESSOR.
> > > > Both require a different type in the signature, (S390ProcessorProps*)
> > > > and (S390MachineProps*). Adding both as parameters seems to be odd
> > > > and would require additionally logic in the function.
> > > > Thus I think doing the cast outside is just the right thing to do.  
> > > 
> > > So what about a void pointer then as parameter?
> > > I prefer a pointer for qemu process memory over uint64_t as part of the 
> > > function interface. This makes it somewhat clearer that this is an
> > > address within QEMU. Both ways will certainly work, though.  
> > 
> > The interface calls are:
> > 
> > int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
> > int kvm_s390_get_processor_props(S390ProcessorProps *prop)
> > 
> > cpu_model_get/set() are just static helpers.  
> 
> So this makes them internal calls...
> 
> >   
> > > 
> > > Conny, I guess you will pick up the patches. Any preference?  
> 
> ...and I'd prefer using a void pointer for them.

Ok, I will make void pointers then to emphasize their address characteristics.

Michael

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/target-s390x/cpu-models.h b/target-s390x/cpu-models.h
index 35dde5a..efa255e 100644
--- a/target-s390x/cpu-models.h
+++ b/target-s390x/cpu-models.h
@@ -43,11 +43,45 @@  typedef struct S390CPUAlias {
     char *model;
 } S390CPUAlias;
 
-extern GSList *s390_cpu_aliases;
+typedef struct S390ProcessorProps {
+    uint64_t cpuid;
+    uint16_t ibc;
+    uint8_t  pad[6];
+    uint64_t fac_list[FAC_LIST_ARCH_S390_SIZE_UINT64];
+} S390ProcessorProps;
+
+typedef struct S390MachineProps {
+    uint64_t cpuid;
+    uint32_t ibc;
+    uint8_t  pad[4];
+    uint64_t fac_mask[FAC_LIST_ARCH_S390_SIZE_UINT64];
+    uint64_t fac_list[FAC_LIST_ARCH_S390_SIZE_UINT64];
+} S390MachineProps;
 
 ObjectClass *s390_cpu_class_by_name(const char *name);
 int set_s390_cpu_alias(const char *name, const char *model);
 
+#ifdef CONFIG_KVM
+int kvm_s390_get_processor_props(S390ProcessorProps *prop);
+int kvm_s390_set_processor_props(S390ProcessorProps *prop);
+bool kvm_s390_cpu_classes_initialized(void);
+#else
+static inline int kvm_s390_get_processor_props(S390ProcessorProps *prop)
+{
+    return -ENOSYS;
+}
+static inline int kvm_s390_set_processor_props(S390ProcessorProps *prop)
+{
+    return -ENOSYS;
+}
+static inline bool kvm_s390_cpu_classes_initialized(void)
+{
+    return false;
+}
+#endif
+
+extern GSList *s390_cpu_aliases;
+
 /*
  * bits 0-7   : CMOS generation
  * bits 8-9   : reserved
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index b48c643..42f01a8 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -44,6 +44,7 @@ 
 #include "hw/s390x/s390-pci-inst.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
+#include "cpu-models.h"
 
 /* #define DEBUG_KVM */
 
@@ -122,6 +123,7 @@  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
 
 static int cap_sync_regs;
 static int cap_async_pf;
+static bool cpu_classes_initialized;
 
 static void *legacy_s390_alloc(size_t size, uint64_t *align);
 
@@ -242,6 +244,59 @@  static void kvm_s390_init_crypto(void)
     kvm_s390_init_dea_kw();
 }
 
+static int cpu_model_get(KVMState *s, uint64_t attr, uint64_t addr)
+{
+    int rc = -ENOSYS;
+    struct kvm_device_attr dev_attr = {
+        .group = KVM_S390_VM_CPU_MODEL,
+        .attr = attr,
+        .addr = addr,
+    };
+
+    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
+        rc = kvm_vm_ioctl(s, KVM_GET_DEVICE_ATTR, &dev_attr);
+    }
+
+    return rc;
+}
+
+static int cpu_model_set(KVMState *s, uint64_t attr, uint64_t addr)
+{
+    int rc = -ENOSYS;
+    struct kvm_device_attr dev_attr = {
+        .group = KVM_S390_VM_CPU_MODEL,
+        .attr = attr,
+        .addr = addr,
+    };
+
+    if (kvm_vm_check_attr(s, dev_attr.group, dev_attr.attr)) {
+        rc = kvm_vm_ioctl(s, KVM_SET_DEVICE_ATTR, &dev_attr);
+    }
+
+    return rc;
+}
+
+static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop)
+{
+    int rc = -EFAULT;
+
+    if (s) {
+        rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, (uint64_t) prop);
+    }
+    trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc);
+
+    return rc;
+}
+
+static void kvm_setup_cpu_classes(KVMState *s)
+{
+    S390MachineProps mach;
+
+    if (!kvm_s390_get_machine_props(s, &mach)) {
+        cpu_classes_initialized = false;
+    }
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
@@ -255,6 +310,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     }
 
     kvm_vm_enable_cap(s, KVM_CAP_S390_USER_SIGP, 0);
+    kvm_setup_cpu_classes(s);
 
     return 0;
 }
@@ -1940,3 +1996,26 @@  int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
     return 0;
 }
+
+int kvm_s390_get_processor_props(S390ProcessorProps *prop)
+{
+    int rc;
+
+    rc = cpu_model_get(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);
+    trace_kvm_get_processor_props(rc, prop->cpuid, prop->ibc);
+    return rc;
+}
+
+int kvm_s390_set_processor_props(S390ProcessorProps *prop)
+{
+    int rc;
+
+    rc = cpu_model_set(kvm_state, KVM_S390_VM_CPU_PROCESSOR, (uint64_t) prop);
+    trace_kvm_set_processor_props(rc);
+    return rc;
+}
+
+bool kvm_s390_cpu_classes_initialized(void)
+{
+    return cpu_classes_initialized;
+}
diff --git a/trace-events b/trace-events
index 30eba92..2f1d734 100644
--- a/trace-events
+++ b/trace-events
@@ -1582,6 +1582,9 @@  kvm_enable_cmma(int rc) "CMMA: enabling with result code %d"
 kvm_clear_cmma(int rc) "CMMA: clearing with result code %d"
 kvm_failed_cpu_state_set(int cpu_index, uint8_t state, const char *msg) "Warning: Unable to set cpu %d state %" PRIu8 " to KVM: %s"
 kvm_sigp_finished(uint8_t order, int cpu_index, int dst_index, int cc) "SIGP: Finished order %u on cpu %d -> cpu %d with cc=%d"
+kvm_get_machine_props(int rc, uint64_t cpuid, uint16_t ibc) "CPU-MODEL: fetching machine properties rc=%d cpuid=0x%"PRIx64" ibc=0x%"PRIx16
+kvm_get_processor_props(int rc, uint64_t cpuid, uint32_t ibc_range) "CPU-MODEL: fetching processor properties rc=%d cpuid=0x%"PRIx64" ibc_range=0x%"PRIx32
+kvm_set_processor_props(int rc) "CPU-MODEL: requesting processor properties rc=%d"
 
 # hw/dma/i8257.c
 i8257_unregistered_dma(int nchan, int dma_pos, int dma_len) "unregistered DMA channel used nchan=%d dma_pos=%d dma_len=%d"