[Qemu-devel] [PATCH 0/2] s390-virtio: Some cleanups.

[Qemu-devel] [PATCH 0/2] s390-virtio: Some cleanups.

[Cornelia Huck]

Hi,

here are two cleanup/refactoring patches for the s390-virtio code.
They will make it easier for virtio-ccw, so I'd like to get them
in before the bulk of the virtio-ccw related patches.

The first one creates an interface for registering callbacks
for diagnose 500, inspired by the spapr hcall interface.

The second one factors initialization code that also can be used
by other machines out into exported functions.

Cornelia Huck (2):
  s390: Add a hypercall registration interface.
  s390-virtio: Factor out some initialization code.

 hw/s390-virtio.c             | 222 +++++++++++++++++++++++++------------------
 hw/s390-virtio.h             |  29 ++++++
 hw/s390x/Makefile.objs       |   1 +
 hw/s390x/s390-virtio-hcall.c |  33 +++++++
 target-s390x/cpu.h           |   2 +-
 target-s390x/kvm.c           |   2 +-
 target-s390x/misc_helper.c   |   2 +-
 7 files changed, 194 insertions(+), 97 deletions(-)
 create mode 100644 hw/s390-virtio.h
 create mode 100644 hw/s390x/s390-virtio-hcall.c

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/2] s390: Add a hypercall registration interface.

[Cornelia Huck]

Allow virtio machines to register for different diag500 function
codes and convert s390-virtio to use it.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390-virtio.c             | 94 ++++++++++++++++++++++++--------------------
 hw/s390-virtio.h             | 23 +++++++++++
 hw/s390x/Makefile.objs       |  1 +
 hw/s390x/s390-virtio-hcall.c | 33 ++++++++++++++++
 target-s390x/cpu.h           |  2 +-
 target-s390x/kvm.c           |  2 +-
 target-s390x/misc_helper.c   |  2 +-
 7 files changed, 112 insertions(+), 45 deletions(-)
 create mode 100644 hw/s390-virtio.h
 create mode 100644 hw/s390x/s390-virtio-hcall.c

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 0e93cc3..91ad43b 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -33,6 +33,7 @@
 
 #include "hw/s390-virtio-bus.h"
 #include "hw/s390x/sclp.h"
+#include "hw/s390-virtio.h"
 
 //#define DEBUG_S390
 
@@ -44,10 +45,6 @@
     do { } while (0)
 #endif
 
-#define KVM_S390_VIRTIO_NOTIFY          0
-#define KVM_S390_VIRTIO_RESET           1
-#define KVM_S390_VIRTIO_SET_STATUS      2
-
 #define KERN_IMAGE_START                0x010000UL
 #define KERN_PARM_AREA                  0x010480UL
 #define INITRD_START                    0x800000UL
@@ -73,56 +70,67 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
     return ipi_states[cpu_addr];
 }
 
-int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t hypercall)
+static int s390_virtio_hcall_notify(uint64_t reg2, uint64_t reg3, uint64_t reg4,
+                                    uint64_t reg5, uint64_t reg6, uint64_t reg7)
 {
+    uint64_t mem = reg2;
     int r = 0, i;
 
-    dprintf("KVM hypercall: %ld\n", hypercall);
-    switch (hypercall) {
-    case KVM_S390_VIRTIO_NOTIFY:
-        if (mem > ram_size) {
-            VirtIOS390Device *dev = s390_virtio_bus_find_vring(s390_bus,
-                                                               mem, &i);
-            if (dev) {
-                virtio_queue_notify(dev->vdev, i);
-            } else {
-                r = -EINVAL;
-            }
-        } else {
-            /* Early printk */
-        }
-        break;
-    case KVM_S390_VIRTIO_RESET:
-    {
-        VirtIOS390Device *dev;
-
-        dev = s390_virtio_bus_find_mem(s390_bus, mem);
-        virtio_reset(dev->vdev);
-        stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS, 0);
-        s390_virtio_device_sync(dev);
-        s390_virtio_reset_idx(dev);
-        break;
-    }
-    case KVM_S390_VIRTIO_SET_STATUS:
-    {
-        VirtIOS390Device *dev;
-
-        dev = s390_virtio_bus_find_mem(s390_bus, mem);
+    if (mem > ram_size) {
+        VirtIOS390Device *dev = s390_virtio_bus_find_vring(s390_bus, mem, &i);
         if (dev) {
-            s390_virtio_device_update_status(dev);
+            virtio_queue_notify(dev->vdev, i);
         } else {
             r = -EINVAL;
         }
-        break;
+    } else {
+        /* Early printk */
     }
-    default:
+    return r;
+}
+
+static int s390_virtio_hcall_reset(uint64_t reg2, uint64_t reg3, uint64_t reg4,
+                                   uint64_t reg5, uint64_t reg6, uint64_t reg7)
+{
+    uint64_t mem = reg2;
+    VirtIOS390Device *dev;
+
+    dev = s390_virtio_bus_find_mem(s390_bus, mem);
+    virtio_reset(dev->vdev);
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS, 0);
+    s390_virtio_device_sync(dev);
+    s390_virtio_reset_idx(dev);
+
+    return 0;
+}
+
+static int s390_virtio_hcall_set_status(uint64_t reg2, uint64_t reg3,
+                                        uint64_t reg4, uint64_t reg5,
+                                        uint64_t reg6, uint64_t reg7)
+{
+    uint64_t mem = reg2;
+    int r = 0;
+    VirtIOS390Device *dev;
+
+    dev = s390_virtio_bus_find_mem(s390_bus, mem);
+    if (dev) {
+        s390_virtio_device_update_status(dev);
+    } else {
         r = -EINVAL;
-        break;
     }
-
     return r;
 }
 
+static void s390_virtio_register_hcalls(void)
+{
+    s390_register_virtio_hypercall(KVM_S390_VIRTIO_NOTIFY,
+                                   s390_virtio_hcall_notify);
+    s390_register_virtio_hypercall(KVM_S390_VIRTIO_RESET,
+                                   s390_virtio_hcall_reset);
+    s390_register_virtio_hypercall(KVM_S390_VIRTIO_SET_STATUS,
+                                   s390_virtio_hcall_set_status);
+}
+
 /*
  * The number of running CPUs. On s390 a shutdown is the state of all CPUs
  * being either stopped or disabled (for interrupts) waiting. We have to
@@ -186,6 +194,9 @@ static void s390_init(QEMUMachineInitArgs *args)
     s390_bus = s390_virtio_bus_init(&my_ram_size);
     s390_sclp_init();
 
+    /* register hypercalls */
+    s390_virtio_register_hcalls();
+
     /* allocate RAM */
     memory_region_init_ram(ram, "s390.ram", my_ram_size);
     vmstate_register_ram_global(ram);
@@ -339,4 +350,3 @@ static void s390_machine_init(void)
 }
 
 machine_init(s390_machine_init);
-
diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
new file mode 100644
index 0000000..cd88179
--- /dev/null
+++ b/hw/s390-virtio.h
@@ -0,0 +1,23 @@
+/*
+ * Virtio interfaces for s390
+ *
+ * Copyright 2012 IBM Corp.
+ * Author(s): Cornelia Huck <cornelia.huck@de.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 HW_S390_VIRTIO_H
+#define HW_S390_VIRTIO_H 1
+
+#define KVM_S390_VIRTIO_NOTIFY          0
+#define KVM_S390_VIRTIO_RESET           1
+#define KVM_S390_VIRTIO_SET_STATUS      2
+
+typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t reg3, uint64_t reg4,
+                              uint64_t reg5, uint64_t reg6, uint64_t reg7);
+void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
+
+#endif
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 096dfcd..ae87a12 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -1,6 +1,7 @@
 obj-y = s390-virtio-bus.o s390-virtio.o
 
 obj-y := $(addprefix ../,$(obj-y))
+obj-y += s390-virtio-hcall.o
 obj-y += sclp.o
 obj-y += event-facility.o
 obj-y += sclpquiesce.o sclpconsole.o
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
new file mode 100644
index 0000000..cc5f47f
--- /dev/null
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -0,0 +1,33 @@
+/*
+ * Support for virtio hypercalls on s390
+ *
+ * Copyright 2012 IBM Corp.
+ * Author(s): Cornelia Huck <cornelia.huck@de.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 "cpu.h"
+#include "hw/s390-virtio.h"
+
+#define MAX_DIAG_SUBCODES 255
+
+static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
+
+void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
+{
+    assert(code < MAX_DIAG_SUBCODES);
+    assert(!s390_diag500_table[code]);
+
+    s390_diag500_table[code] = fn;
+}
+
+int s390_virtio_hypercall(CPUS390XState *env)
+{
+    s390_virtio_fn fn = s390_diag500_table[env->regs[1]];
+
+    return fn ? fn(env->regs[2], env->regs[3], env->regs[4], env->regs[5],
+                   env->regs[6], env->regs[7]) : -EINVAL;
+}
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index bc3fab2..6700fe9 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -302,7 +302,7 @@ int cpu_s390x_handle_mmu_fault (CPUS390XState *env, target_ulong address, int rw
 void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
 
-int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t hypercall);
+int s390_virtio_hypercall(CPUS390XState *env);
 
 #ifdef CONFIG_KVM
 void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code);
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 6ec5e6d..ae6ae07 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -386,7 +386,7 @@ static int handle_priv(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
 static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
 {
     cpu_synchronize_state(env);
-    env->regs[2] = s390_virtio_hypercall(env, env->regs[2], env->regs[1]);
+    env->regs[2] = s390_virtio_hypercall(env);
 
     return 0;
 }
diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
index 3015bfe..1937e0c 100644
--- a/target-s390x/misc_helper.c
+++ b/target-s390x/misc_helper.c
@@ -107,7 +107,7 @@ uint64_t HELPER(diag)(CPUS390XState *env, uint32_t num, uint64_t mem,
     switch (num) {
     case 0x500:
         /* KVM hypercall */
-        r = s390_virtio_hypercall(env, mem, code);
+        r = s390_virtio_hypercall(env);
         break;
     case 0x44:
         /* yield */
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code.

[Cornelia Huck]

Some of the machine initialization for s390-virtio will be reused
by virtio-ccw.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390-virtio.c | 134 ++++++++++++++++++++++++++++++++-----------------------
 hw/s390-virtio.h |   6 +++
 2 files changed, 85 insertions(+), 55 deletions(-)

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 91ad43b..3856ab7 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -159,62 +159,11 @@ unsigned s390_del_running_cpu(CPUS390XState *env)
     return s390_running_cpus;
 }
 
-/* PC hardware initialisation */
-static void s390_init(QEMUMachineInitArgs *args)
+CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
 {
-    ram_addr_t my_ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
     CPUS390XState *env = NULL;
-    MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
-    ram_addr_t kernel_size = 0;
-    ram_addr_t initrd_offset;
-    ram_addr_t initrd_size = 0;
-    int shift = 0;
-    uint8_t *storage_keys;
-    void *virtio_region;
-    hwaddr virtio_region_len;
-    hwaddr virtio_region_start;
     int i;
 
-    /* s390x ram size detection needs a 16bit multiplier + an increment. So
-       guests > 64GB can be specified in 2MB steps etc. */
-    while ((my_ram_size >> (20 + shift)) > 65535) {
-        shift++;
-    }
-    my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
-
-    /* lets propagate the changed ram size into the global variable. */
-    ram_size = my_ram_size;
-
-    /* get a BUS */
-    s390_bus = s390_virtio_bus_init(&my_ram_size);
-    s390_sclp_init();
-
-    /* register hypercalls */
-    s390_virtio_register_hcalls();
-
-    /* allocate RAM */
-    memory_region_init_ram(ram, "s390.ram", my_ram_size);
-    vmstate_register_ram_global(ram);
-    memory_region_add_subregion(sysmem, 0, ram);
-
-    /* clear virtio region */
-    virtio_region_len = my_ram_size - ram_size;
-    virtio_region_start = ram_size;
-    virtio_region = cpu_physical_memory_map(virtio_region_start,
-                                            &virtio_region_len, true);
-    memset(virtio_region, 0, virtio_region_len);
-    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
-                              virtio_region_len);
-
-    /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
-
-    /* init CPUs */
     if (cpu_model == NULL) {
         cpu_model = "host";
     }
@@ -235,6 +184,17 @@ static void s390_init(QEMUMachineInitArgs *args)
         tmp_env->exception_index = EXCP_HLT;
         tmp_env->storage_keys = storage_keys;
     }
+    return env;
+}
+
+void s390_set_up_kernel(CPUS390XState *env,
+                        const char *kernel_filename,
+                        const char *kernel_cmdline,
+                        const char *initrd_filename)
+{
+    ram_addr_t kernel_size = 0;
+    ram_addr_t initrd_offset;
+    ram_addr_t initrd_size = 0;
 
     /* One CPU has to run */
     s390_add_running_cpu(env);
@@ -306,9 +266,13 @@ static void s390_init(QEMUMachineInitArgs *args)
         memcpy(rom_ptr(KERN_PARM_AREA), kernel_cmdline,
                strlen(kernel_cmdline) + 1);
     }
+}
 
-    /* Create VirtIO network adapters */
-    for(i = 0; i < nb_nics; i++) {
+void s390_create_virtio_net(BusState *bus, const char *name)
+{
+    int i;
+
+    for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
         DeviceState *dev;
 
@@ -321,12 +285,72 @@ static void s390_init(QEMUMachineInitArgs *args)
             exit(1);
         }
 
-        dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
+        dev = qdev_create(bus, name);
         qdev_set_nic_properties(dev, nd);
         qdev_init_nofail(dev);
     }
 }
 
+/* PC hardware initialisation */
+static void s390_init(QEMUMachineInitArgs *args)
+{
+    ram_addr_t my_ram_size = args->ram_size;
+    const char *cpu_model = args->cpu_model;
+    const char *kernel_filename = args->kernel_filename;
+    const char *kernel_cmdline = args->kernel_cmdline;
+    const char *initrd_filename = args->initrd_filename;
+    CPUS390XState *env = NULL;
+    MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *ram = g_new(MemoryRegion, 1);
+    int shift = 0;
+    uint8_t *storage_keys;
+    void *virtio_region;
+    hwaddr virtio_region_len;
+    hwaddr virtio_region_start;
+
+    /* s390x ram size detection needs a 16bit multiplier + an increment. So
+       guests > 64GB can be specified in 2MB steps etc. */
+    while ((my_ram_size >> (20 + shift)) > 65535) {
+        shift++;
+    }
+    my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
+
+    /* lets propagate the changed ram size into the global variable. */
+    ram_size = my_ram_size;
+
+    /* get a BUS */
+    s390_bus = s390_virtio_bus_init(&my_ram_size);
+    s390_sclp_init();
+
+    /* register hypercalls */
+    s390_virtio_register_hcalls();
+
+    /* allocate RAM */
+    memory_region_init_ram(ram, "s390.ram", my_ram_size);
+    vmstate_register_ram_global(ram);
+    memory_region_add_subregion(sysmem, 0, ram);
+
+    /* clear virtio region */
+    virtio_region_len = my_ram_size - ram_size;
+    virtio_region_start = ram_size;
+    virtio_region = cpu_physical_memory_map(virtio_region_start,
+                                            &virtio_region_len, true);
+    memset(virtio_region, 0, virtio_region_len);
+    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
+                              virtio_region_len);
+
+    /* allocate storage keys */
+    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+
+    /* init CPUs */
+    env = s390_init_cpus(cpu_model, storage_keys);
+
+    s390_set_up_kernel(env, kernel_filename, kernel_cmdline, initrd_filename);
+
+    /* Create VirtIO network adapters */
+    s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
+}
+
 static QEMUMachine s390_machine = {
     .name = "s390-virtio",
     .alias = "s390",
diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
index cd88179..acd4846 100644
--- a/hw/s390-virtio.h
+++ b/hw/s390-virtio.h
@@ -20,4 +20,10 @@ typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t reg3, uint64_t reg4,
                               uint64_t reg5, uint64_t reg6, uint64_t reg7);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
+CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
+void s390_set_up_kernel(CPUS390XState *env,
+                        const char *kernel_filename,
+                        const char *kernel_cmdline,
+                        const char *initrd_filename);
+void s390_create_virtio_net(BusState *bus, const char *name);
 #endif
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/2] s390: Add a hypercall registration interface.

[Alexander Graf]

On 16.01.2013, at 12:57, Cornelia Huck wrote:

> Allow virtio machines to register for different diag500 function
> codes and convert s390-virtio to use it.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Nice cleanup :). One minor nitpick below

> ---
> hw/s390-virtio.c             | 94 ++++++++++++++++++++++++--------------------
> hw/s390-virtio.h             | 23 +++++++++++
> hw/s390x/Makefile.objs       |  1 +
> hw/s390x/s390-virtio-hcall.c | 33 ++++++++++++++++
> target-s390x/cpu.h           |  2 +-
> target-s390x/kvm.c           |  2 +-
> target-s390x/misc_helper.c   |  2 +-
> 7 files changed, 112 insertions(+), 45 deletions(-)
> create mode 100644 hw/s390-virtio.h
> create mode 100644 hw/s390x/s390-virtio-hcall.c
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 0e93cc3..91ad43b 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -33,6 +33,7 @@
> 
> #include "hw/s390-virtio-bus.h"
> #include "hw/s390x/sclp.h"
> +#include "hw/s390-virtio.h"
> 
> //#define DEBUG_S390
> 
> @@ -44,10 +45,6 @@
>     do { } while (0)
> #endif
> 
> -#define KVM_S390_VIRTIO_NOTIFY          0
> -#define KVM_S390_VIRTIO_RESET           1
> -#define KVM_S390_VIRTIO_SET_STATUS      2
> -
> #define KERN_IMAGE_START                0x010000UL
> #define KERN_PARM_AREA                  0x010480UL
> #define INITRD_START                    0x800000UL
> @@ -73,56 +70,67 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>     return ipi_states[cpu_addr];
> }
> 
> -int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t hypercall)
> +static int s390_virtio_hcall_notify(uint64_t reg2, uint64_t reg3, uint64_t reg4,
> +                                    uint64_t reg5, uint64_t reg6, uint64_t reg7)
> {
> +    uint64_t mem = reg2;
>     int r = 0, i;
> 
> -    dprintf("KVM hypercall: %ld\n", hypercall);
> -    switch (hypercall) {
> -    case KVM_S390_VIRTIO_NOTIFY:
> -        if (mem > ram_size) {
> -            VirtIOS390Device *dev = s390_virtio_bus_find_vring(s390_bus,
> -                                                               mem, &i);
> -            if (dev) {
> -                virtio_queue_notify(dev->vdev, i);
> -            } else {
> -                r = -EINVAL;
> -            }
> -        } else {
> -            /* Early printk */
> -        }
> -        break;
> -    case KVM_S390_VIRTIO_RESET:
> -    {
> -        VirtIOS390Device *dev;
> -
> -        dev = s390_virtio_bus_find_mem(s390_bus, mem);
> -        virtio_reset(dev->vdev);
> -        stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS, 0);
> -        s390_virtio_device_sync(dev);
> -        s390_virtio_reset_idx(dev);
> -        break;
> -    }
> -    case KVM_S390_VIRTIO_SET_STATUS:
> -    {
> -        VirtIOS390Device *dev;
> -
> -        dev = s390_virtio_bus_find_mem(s390_bus, mem);
> +    if (mem > ram_size) {
> +        VirtIOS390Device *dev = s390_virtio_bus_find_vring(s390_bus, mem, &i);
>         if (dev) {
> -            s390_virtio_device_update_status(dev);
> +            virtio_queue_notify(dev->vdev, i);
>         } else {
>             r = -EINVAL;
>         }
> -        break;
> +    } else {
> +        /* Early printk */
>     }
> -    default:
> +    return r;
> +}
> +
> +static int s390_virtio_hcall_reset(uint64_t reg2, uint64_t reg3, uint64_t reg4,
> +                                   uint64_t reg5, uint64_t reg6, uint64_t reg7)
> +{
> +    uint64_t mem = reg2;
> +    VirtIOS390Device *dev;
> +
> +    dev = s390_virtio_bus_find_mem(s390_bus, mem);
> +    virtio_reset(dev->vdev);
> +    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS, 0);
> +    s390_virtio_device_sync(dev);
> +    s390_virtio_reset_idx(dev);
> +
> +    return 0;
> +}
> +
> +static int s390_virtio_hcall_set_status(uint64_t reg2, uint64_t reg3,
> +                                        uint64_t reg4, uint64_t reg5,
> +                                        uint64_t reg6, uint64_t reg7)
> +{
> +    uint64_t mem = reg2;
> +    int r = 0;
> +    VirtIOS390Device *dev;
> +
> +    dev = s390_virtio_bus_find_mem(s390_bus, mem);
> +    if (dev) {
> +        s390_virtio_device_update_status(dev);
> +    } else {
>         r = -EINVAL;
> -        break;
>     }
> -
>     return r;
> }
> 
> +static void s390_virtio_register_hcalls(void)
> +{
> +    s390_register_virtio_hypercall(KVM_S390_VIRTIO_NOTIFY,
> +                                   s390_virtio_hcall_notify);
> +    s390_register_virtio_hypercall(KVM_S390_VIRTIO_RESET,
> +                                   s390_virtio_hcall_reset);
> +    s390_register_virtio_hypercall(KVM_S390_VIRTIO_SET_STATUS,
> +                                   s390_virtio_hcall_set_status);
> +}
> +
> /*
>  * The number of running CPUs. On s390 a shutdown is the state of all CPUs
>  * being either stopped or disabled (for interrupts) waiting. We have to
> @@ -186,6 +194,9 @@ static void s390_init(QEMUMachineInitArgs *args)
>     s390_bus = s390_virtio_bus_init(&my_ram_size);
>     s390_sclp_init();
> 
> +    /* register hypercalls */
> +    s390_virtio_register_hcalls();
> +
>     /* allocate RAM */
>     memory_region_init_ram(ram, "s390.ram", my_ram_size);
>     vmstate_register_ram_global(ram);
> @@ -339,4 +350,3 @@ static void s390_machine_init(void)
> }
> 
> machine_init(s390_machine_init);
> -
> diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
> new file mode 100644
> index 0000000..cd88179
> --- /dev/null
> +++ b/hw/s390-virtio.h
> @@ -0,0 +1,23 @@
> +/*
> + * Virtio interfaces for s390
> + *
> + * Copyright 2012 IBM Corp.
> + * Author(s): Cornelia Huck <cornelia.huck@de.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 HW_S390_VIRTIO_H
> +#define HW_S390_VIRTIO_H 1
> +
> +#define KVM_S390_VIRTIO_NOTIFY          0
> +#define KVM_S390_VIRTIO_RESET           1
> +#define KVM_S390_VIRTIO_SET_STATUS      2
> +
> +typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t reg3, uint64_t reg4,
> +                              uint64_t reg5, uint64_t reg6, uint64_t reg7);

const uint64_t *args

> +void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
> +
> +#endif
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 096dfcd..ae87a12 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -1,6 +1,7 @@
> obj-y = s390-virtio-bus.o s390-virtio.o
> 
> obj-y := $(addprefix ../,$(obj-y))
> +obj-y += s390-virtio-hcall.o
> obj-y += sclp.o
> obj-y += event-facility.o
> obj-y += sclpquiesce.o sclpconsole.o
> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
> new file mode 100644
> index 0000000..cc5f47f
> --- /dev/null
> +++ b/hw/s390x/s390-virtio-hcall.c
> @@ -0,0 +1,33 @@
> +/*
> + * Support for virtio hypercalls on s390
> + *
> + * Copyright 2012 IBM Corp.
> + * Author(s): Cornelia Huck <cornelia.huck@de.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 "cpu.h"
> +#include "hw/s390-virtio.h"
> +
> +#define MAX_DIAG_SUBCODES 255
> +
> +static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
> +
> +void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
> +{
> +    assert(code < MAX_DIAG_SUBCODES);
> +    assert(!s390_diag500_table[code]);
> +
> +    s390_diag500_table[code] = fn;
> +}
> +
> +int s390_virtio_hypercall(CPUS390XState *env)
> +{
> +    s390_virtio_fn fn = s390_diag500_table[env->regs[1]];
> +
> +    return fn ? fn(env->regs[2], env->regs[3], env->regs[4], env->regs[5],
> +                   env->regs[6], env->regs[7]) : -EINVAL;

if (!fn) {
    return -EINVAL;
}

return fn(&env->regs[2]);

That way the hypercall handling function can determine itself which registers it really needs to access.


> +}
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index bc3fab2..6700fe9 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -302,7 +302,7 @@ int cpu_s390x_handle_mmu_fault (CPUS390XState *env, target_ulong address, int rw
> void s390x_tod_timer(void *opaque);
> void s390x_cpu_timer(void *opaque);
> 
> -int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t hypercall);
> +int s390_virtio_hypercall(CPUS390XState *env);
> 
> #ifdef CONFIG_KVM
> void kvm_s390_interrupt(S390CPU *cpu, int type, uint32_t code);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 6ec5e6d..ae6ae07 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -386,7 +386,7 @@ static int handle_priv(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
> static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
> {
>     cpu_synchronize_state(env);
> -    env->regs[2] = s390_virtio_hypercall(env, env->regs[2], env->regs[1]);
> +    env->regs[2] = s390_virtio_hypercall(env);

Just thinking out loud here. With synchronized registers, we have full access to the GPRs already without copying them to env. So if instead we would call

    s390_virtio_hypercall(env->regs);

we could in case we support synchronized registers call

    s390_virtio_hypercall(kvm_run->s.regs.gprs);

which would completely remove the need for cpu_synchronize_state() for normal hypercalls.

This is outside of the scope of this patch, but might be a useful thing to do :). As a nice side effect, the global s390_virtio_hypercall function wouldn't have to know anything about CPUState either.


Alex

Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code.

[Alexander Graf]

On 16.01.2013, at 12:57, Cornelia Huck wrote:

> Some of the machine initialization for s390-virtio will be reused
> by virtio-ccw.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I would prefer to see this patch as part of the patch set that introduces the new s390 ccw machine, so I can check whether what it does is sound in context.


Alex

Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code.

[Andreas Färber]

Am 16.01.2013 12:57, schrieb Cornelia Huck:
> diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
> index cd88179..acd4846 100644
> --- a/hw/s390-virtio.h
> +++ b/hw/s390-virtio.h
> @@ -20,4 +20,10 @@ typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t reg3, uint64_t reg4,
>                                uint64_t reg5, uint64_t reg6, uint64_t reg7);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>  
> +CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
> +void s390_set_up_kernel(CPUS390XState *env,
> +                        const char *kernel_filename,
> +                        const char *kernel_cmdline,
> +                        const char *initrd_filename);

I don't like this interface: It reads "cpus" but appears to return a
single CPUS390XState. Can't you at least use S390CPU* instead?

Alternatively it would be possible (although at some point to be
changed) to use global first_cpu and to iterate over the CPUs rather
than returning one from one function to the other.

However since the only usage I spot in the patch without looking up the
file myself is s390_add_running_cpu(), can the call be moved out of the
kernel setup function to avoid this dependency?

Andreas

> +void s390_create_virtio_net(BusState *bus, const char *name);
>  #endif

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] s390: Add a hypercall registration interface.

[Cornelia Huck]

On Wed, 16 Jan 2013 13:17:34 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 16.01.2013, at 12:57, Cornelia Huck wrote:
> 
> > Allow virtio machines to register for different diag500 function
> > codes and convert s390-virtio to use it.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Nice cleanup :). One minor nitpick below

> > +int s390_virtio_hypercall(CPUS390XState *env)
> > +{
> > +    s390_virtio_fn fn = s390_diag500_table[env->regs[1]];
> > +
> > +    return fn ? fn(env->regs[2], env->regs[3], env->regs[4], env->regs[5],
> > +                   env->regs[6], env->regs[7]) : -EINVAL;
> 
> if (!fn) {
>     return -EINVAL;
> }
> 
> return fn(&env->regs[2]);
> 
> That way the hypercall handling function can determine itself which registers it really needs to access.

Yes, this looks a bit nicer. v2 is on the way.

> 
> 
> > +}

> > static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
> > {
> >     cpu_synchronize_state(env);
> > -    env->regs[2] = s390_virtio_hypercall(env, env->regs[2], env->regs[1]);
> > +    env->regs[2] = s390_virtio_hypercall(env);
> 
> Just thinking out loud here. With synchronized registers, we have full access to the GPRs already without copying them to env. So if instead we would call
> 
>     s390_virtio_hypercall(env->regs);
> 
> we could in case we support synchronized registers call
> 
>     s390_virtio_hypercall(kvm_run->s.regs.gprs);
> 
> which would completely remove the need for cpu_synchronize_state() for normal hypercalls.
> 
> This is outside of the scope of this patch, but might be a useful thing to do :). As a nice side effect, the global s390_virtio_hypercall function wouldn't have to know anything about CPUState either.

Sounds like a good future improvement.

Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code.

[Cornelia Huck]

On Wed, 16 Jan 2013 13:41:10 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 16.01.2013 12:57, schrieb Cornelia Huck:
> > diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
> > index cd88179..acd4846 100644
> > --- a/hw/s390-virtio.h
> > +++ b/hw/s390-virtio.h
> > @@ -20,4 +20,10 @@ typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t reg3, uint64_t reg4,
> >                                uint64_t reg5, uint64_t reg6, uint64_t reg7);
> >  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
> >  
> > +CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
> > +void s390_set_up_kernel(CPUS390XState *env,
> > +                        const char *kernel_filename,
> > +                        const char *kernel_cmdline,
> > +                        const char *initrd_filename);
> 
> I don't like this interface: It reads "cpus" but appears to return a
> single CPUS390XState. Can't you at least use S390CPU* instead?
> 
> Alternatively it would be possible (although at some point to be
> changed) to use global first_cpu and to iterate over the CPUs rather
> than returning one from one function to the other.

An alternative might be to use s390_cpu_addr2state(0) to effectively
get to the same cpu.

> 
> However since the only usage I spot in the patch without looking up the
> file myself is s390_add_running_cpu(), can the call be moved out of the
> kernel setup function to avoid this dependency?

s390_set_up_kernel() uses it to specify the initial psw, and for
virtio-ccw, it will be needed to issue an ioctl. But both use cases
could be covered by grabbing cpu 0.

> 
> Andreas
> 
> > +void s390_create_virtio_net(BusState *bus, const char *name);
> >  #endif
>