[Qemu-devel] [PATCH 0/9] S390x KVM support

[Qemu-devel] [PATCH 0/9] S390x KVM support

[Alexander Graf]

While S390x was one of the first targets that were supported by KVM it always
lacked qemu system emulation support.

In order to change that sad fact, I figured I'd just take on the task myself,
taking kuli (http://www.ibm.com/developerworks/linux/linux390/kuli.html),
Documentation/s390/kvm.txt and lguest as starting points to write a clean,
qemu'ish S390x Virtio machine emulation.

So now you can run Linux VMs on Linux on z/VM on LPAR on zSeries!

This is the resulting code. Please comment on things you like and also on the
ones you don't :-).

The patchset is based on Uli's S390x userspace emulation patches. There's not
really that much shared functionality, but I didn't want to reimplement the
configure wheels. So make sure to have his patches applied first.

Please only run S390x system virtualization using io threads. S390x has very
few intercepts and even less that actually reach userspace. So you'll end up
with a non-responsive VM without a dedicated IO thread.

Use: $ ./configure --target-list=s390x-softmmu --enable-io-thread

Currently only -kernel, -initrd and friends work. Booting from a real dasd
device is not supported.

Also to actually run this code you need a patch for an ugly bug in the kernel
module: http://alex.csgraf.de/psw.patch

I verified that the userspace actually works on a 2.6.27 (SLES11) kernel, so
if it doesn't work on current git, please tell me! I'm too afraid that I might
end up in a 3270 terminal to update the kernel on my z/VM instance :-).

Alexander Graf (9):
  Export function for VA defined ram allocation
  Add KVM support for S390x
  Add support for S390x system emulation
  Add S390x virtio machine bus
  Add S390x virtio machine description
  S390 GDB stub
  Implement early printk in virtio-console
  Set default console to virtio on S390x
  Move mp_state to CPU_COMMON

 Makefile.target          |    2 +
 configure                |    4 +-
 cpu-common.h             |    1 +
 cpu-defs.h               |    1 +
 exec.c                   |   15 ++-
 gdbstub.c                |   52 ++++++
 hw/s390-virtio-bus.c     |  350 +++++++++++++++++++++++++++++++++++
 hw/s390-virtio-bus.h     |   64 +++++++
 hw/s390-virtio.c         |  244 ++++++++++++++++++++++++
 hw/virtio-console.c      |    7 +
 hw/virtio-console.h      |    2 +
 target-i386/cpu.h        |    1 -
 target-s390x/cpu.h       |   86 +++++++++-
 target-s390x/exec.h      |    5 +
 target-s390x/helper.c    |   26 +++-
 target-s390x/kvm.c       |  457 ++++++++++++++++++++++++++++++++++++++++++++++
 target-s390x/machine.c   |   30 +++
 target-s390x/op_helper.c |   53 ++++++
 vl.c                     |   26 +++
 19 files changed, 1418 insertions(+), 8 deletions(-)
 create mode 100644 hw/s390-virtio-bus.c
 create mode 100644 hw/s390-virtio-bus.h
 create mode 100644 hw/s390-virtio.c
 create mode 100644 target-s390x/kvm.c
 create mode 100644 target-s390x/machine.c

[Qemu-devel] [PATCH 1/9] Export function for VA defined ram allocation

[Alexander Graf]

S390 requires vmas for guests to be < 256 GB. So we need to directly export
mmaps "try to use this vma as start address" feature to not accidently get
over that limit.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 cpu-common.h |    1 +
 exec.c       |   15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index 6302372..ecaf9e3 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -30,6 +30,7 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
 
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
 ram_addr_t qemu_ram_alloc(ram_addr_t);
+ram_addr_t _qemu_ram_alloc(ram_addr_t size, void *map_at);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
 void *qemu_get_ram_ptr(ram_addr_t addr);
diff --git a/exec.c b/exec.c
index 076d26b..36c26cd 100644
--- a/exec.c
+++ b/exec.c
@@ -2404,14 +2404,20 @@ void qemu_unregister_coalesced_mmio(target_phys_addr_t addr, ram_addr_t size)
         kvm_uncoalesce_mmio_region(addr, size);
 }
 
-ram_addr_t qemu_ram_alloc(ram_addr_t size)
+ram_addr_t _qemu_ram_alloc(ram_addr_t size, void *map_at)
 {
     RAMBlock *new_block;
 
     size = TARGET_PAGE_ALIGN(size);
     new_block = qemu_malloc(sizeof(*new_block));
 
-    new_block->host = qemu_vmalloc(size);
+    if (map_at) {
+        new_block->host = mmap(map_at, size, PROT_EXEC|PROT_READ|PROT_WRITE,
+                               MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+
+    } else {
+        new_block->host = qemu_vmalloc(size);
+    }
 #ifdef MADV_MERGEABLE
     madvise(new_block->host, size, MADV_MERGEABLE);
 #endif
@@ -2434,6 +2440,11 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
     return new_block->offset;
 }
 
+ram_addr_t qemu_ram_alloc(ram_addr_t size)
+{
+    return _qemu_ram_alloc(size, NULL);
+}
+
 void qemu_ram_free(ram_addr_t addr)
 {
     /* TODO: implement this.  */
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/9] Add KVM support for S390x

[Alexander Graf]

S390x was one of the first platforms that received support for KVM back in the
day. Unfortunately until now there hasn't been a qemu implementation that would
enable users to actually run guests.

So let's include support for KVM S390x in qemu!

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 configure          |    4 +-
 target-s390x/kvm.c |  457 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 460 insertions(+), 1 deletions(-)
 create mode 100644 target-s390x/kvm.c

diff --git a/configure b/configure
index 64be51f..b487094 100755
--- a/configure
+++ b/configure
@@ -1342,6 +1342,8 @@ EOF
             kvm_cflags="$kvm_cflags -I$kerneldir/arch/x86/include"
 	elif test "$cpu" = "ppc" -a -d "$kerneldir/arch/powerpc/include" ; then
 	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/powerpc/include"
+	elif test "$cpu" = "s390x" -a -d "$kerneldir/arch/s390/include" ; then
+	    kvm_cflags="$kvm_cflags -I$kerneldir/arch/s390/include"
         elif test -d "$kerneldir/arch/$cpu/include" ; then
             kvm_cflags="$kvm_cflags -I$kerneldir/arch/$cpu/include"
       fi
@@ -2286,7 +2288,7 @@ case "$target_arch2" in
     fi
 esac
 case "$target_arch2" in
-  i386|x86_64|ppcemb|ppc|ppc64)
+  i386|x86_64|ppcemb|ppc|ppc64|s390x)
     # Make sure the target and host cpus are compatible
     if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
       \( "$target_arch2" = "$cpu" -o \
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
new file mode 100644
index 0000000..29e21ed
--- /dev/null
+++ b/target-s390x/kvm.c
@@ -0,0 +1,457 @@
+/*
+ * QEMU S390x KVM implementation
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+
+#include <linux/kvm.h>
+#include <asm/ptrace.h>
+
+#include "qemu-common.h"
+#include "qemu-timer.h"
+#include "sysemu.h"
+#include "kvm.h"
+#include "cpu.h"
+#include "device_tree.h"
+
+/* #define DEBUG_KVM */
+
+#ifdef DEBUG_KVM
+#define dprintf(fmt, ...) \
+    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#else
+#define dprintf(fmt, ...) \
+    do { } while (0)
+#endif
+
+#define IPA0_DIAG               0x8300
+#define IPA0_SIGP               0xae00
+#define IPA0_PRIV               0xb200
+
+#define PRIV_SCLP_CALL          0x20
+#define DIAG_KVM_HYPERCALL      0x500
+#define DIAG_KVM_BREAKPOINT     0x501
+
+#define SCP_LENGTH              0x00
+#define SCP_FUNCTION_CODE       0x02
+#define SCP_CONTROL_MASK        0x03
+#define SCP_RESPONSE_CODE       0x06
+#define SCP_MEM_CODE            0x08
+#define SCP_INCREMENT           0x0a
+
+#define ICPT_INSTRUCTION        0x04
+#define ICPT_WAITPSW            0x1c
+#define ICPT_SOFT_INTERCEPT     0x24
+#define ICPT_CPU_STOP           0x28
+#define ICPT_IO                 0x40
+
+#define SIGP_RESTART            0x06
+#define SIGP_INITIAL_CPU_RESET  0x0b
+#define SIGP_STORE_STATUS_ADDR  0x0e
+#define SIGP_SET_ARCH           0x12
+
+
+int kvm_arch_init(KVMState *s, int smp_cpus)
+{
+    return 0;
+}
+
+int kvm_arch_init_vcpu(CPUState *env)
+{
+    int ret = 0;
+
+    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL) < 0)
+        perror("cannot init reset vcpu");
+
+    return ret;
+}
+
+int kvm_arch_put_registers(CPUState *env)
+{
+    struct kvm_regs regs;
+    int ret;
+    int i;
+
+    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < 16; i++)
+        regs.gprs[i] = env->regs[i];
+
+    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    env->kvm_run->s390_sieic.addr = env->psw.addr;
+    env->kvm_run->s390_sieic.mask = env->psw.mask;
+
+    return ret;
+}
+
+int kvm_arch_get_registers(CPUState *env)
+{
+    uint32_t ret;
+    struct kvm_regs regs;
+    int i;
+
+    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
+    if (ret < 0)
+        return ret;
+
+    for (i = 0; i < 16; i++)
+        env->regs[i] = regs.gprs[i];
+
+    env->psw.addr = env->kvm_run->s390_sieic.addr;
+    env->psw.mask = env->kvm_run->s390_sieic.mask;
+
+    return 0;
+}
+
+int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
+
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)diag_501, 4, 1))
+        return -EINVAL;
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    uint8_t t[4];
+    static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
+
+    if (cpu_memory_rw_debug(env, bp->pc, t, 4, 0))
+        return -EINVAL;
+    if (memcmp(t, diag_501, 4))
+        return -EINVAL;
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 1, 1))
+        return -EINVAL;
+
+    return 0;
+}
+
+int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
+{
+    return 0;
+}
+
+int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
+{
+    return 0;
+}
+
+static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, uint64_t parm64, int vm)
+{
+    struct kvm_s390_interrupt kvmint;
+    int r;
+
+    env->halted = 0;
+    env->exception_index = 0;
+
+    kvmint.type = type;
+    kvmint.parm = parm;
+    kvmint.parm64 = parm64;
+
+    if (vm)
+        r = kvm_vm_ioctl(env->kvm_state, KVM_S390_INTERRUPT, &kvmint);
+    else 
+        r = kvm_vcpu_ioctl(env, KVM_S390_INTERRUPT, &kvmint);
+
+    if (r < 0) {
+        fprintf(stderr, "KVM failed to inject interrupt\n");
+        exit(1);
+    }
+}
+
+void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token)
+{
+    _kvm_s390_interrupt(env, KVM_S390_INT_VIRTIO, config_change, token, 1);
+}
+
+static void kvm_s390_interrupt(CPUState *env, int type, uint32_t code)
+{
+    _kvm_s390_interrupt(env, type, code, 0, 0);
+}
+
+static void enter_pgmcheck(CPUState *env, uint16_t code)
+{
+    kvm_s390_interrupt(env, KVM_S390_PROGRAM_INT, code);
+}
+
+static void setcc(CPUState *env, uint64_t cc)
+{
+    env->kvm_run->s390_sieic.mask &= ~(3ul << 44);
+    env->kvm_run->s390_sieic.mask |= (cc & 3) << 44;
+
+    env->psw.mask &= ~(3ul << 44);
+    env->psw.mask |= (cc & 3) << 44;
+}
+
+static int sclp_service_call(CPUState *env, struct kvm_run *run, uint16_t ipbh0)
+{
+    uint32_t sccb;
+    uint64_t code;
+    int r = 0;
+
+    cpu_synchronize_state(env);
+    sccb = env->regs[ipbh0 & 0xf];
+    code = env->regs[(ipbh0 & 0xf0) >> 4];
+
+    dprintf("sclp(0x%x, 0x%lx)\n", sccb, code);
+
+    if (sccb & ~0x7ffffff8ul) {
+        fprintf(stderr, "KVM: invalid sccb address 0x%x\n", sccb);
+        r = -1;
+        goto out;
+    }
+
+    switch(code) {
+        case 0x00020001:
+        case 0x00120001:
+            stw_phys(sccb + SCP_MEM_CODE, ram_size >> 20);
+            stb_phys(sccb + SCP_INCREMENT, 1);
+            stw_phys(sccb + SCP_RESPONSE_CODE, 0x10);
+            setcc(env, 0);
+
+            _kvm_s390_interrupt(env, KVM_S390_INT_SERVICE, sccb & ~3, 0, 1);
+            break;
+        default:
+            dprintf("KVM: invalid sclp call 0x%x / 0x%lx\n", sccb, code);
+            r = -1;
+            break;
+    }
+
+out:
+    if (r < 0)
+        setcc(env, 3);
+    return 0;
+}
+
+static int handle_priv(CPUState *env, struct kvm_run *run, uint8_t ipa1)
+{
+    int r = 0;
+    uint16_t ipbh0 = (run->s390_sieic.ipb & 0xffff0000) >> 16;
+
+    dprintf("KVM: PRIV: %d\n", ipa1);
+    switch (ipa1) {
+        case PRIV_SCLP_CALL:
+            r = sclp_service_call(env, run, ipbh0);
+            break;
+        default:
+            dprintf("KVM: unknown PRIV: 0x%x\n", ipa1);
+            r = -1;
+            break;
+    }
+
+    return r;
+}
+
+static int handle_hypercall(CPUState *env, struct kvm_run *run)
+{
+    int r;
+
+    cpu_synchronize_state(env);
+    r = s390_virtio_hypercall(env);
+    kvm_arch_put_registers(env);
+
+    return r;
+}
+
+static int handle_diag(CPUState *env, struct kvm_run *run, int ipb_code)
+{
+    int r = 0;
+
+    switch (ipb_code) {
+        case DIAG_KVM_HYPERCALL:
+            r = handle_hypercall(env, run);
+            break;
+        case DIAG_KVM_BREAKPOINT:
+            sleep(10);
+            break;
+        default:
+            dprintf("KVM: unknown DIAG: 0x%x\n", ipb_code);
+            r = -1;
+            break;
+    }
+
+    return r;
+}
+
+static int s390_cpu_restart(CPUState *env)
+{
+    kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
+    env->halted = 0;
+    env->exception_index = 0;
+    qemu_cpu_kick(env);
+    fprintf(stderr, "DONE: SIGP cpu restart: %p\n", env);
+    return 0;
+}
+
+static int s390_store_status(CPUState *env, uint32_t parameter)
+{
+    /* XXX */
+    fprintf(stderr, "XXX SIGP store status\n");
+    return -1;
+}
+
+static int s390_cpu_initial_reset(CPUState *env)
+{
+    /* XXX */
+    fprintf(stderr, "XXX SIGP init\n");
+    return -1;
+}
+
+static int handle_sigp(CPUState *env, struct kvm_run *run, uint8_t ipa1)
+{
+    uint8_t order_code;
+    uint32_t parameter;
+    uint16_t cpu_addr;
+    uint8_t t;
+    int r = -1;
+    CPUState *target_env;
+
+    cpu_synchronize_state(env);
+
+    /* get order code */
+    order_code = run->s390_sieic.ipb >> 28;
+    if (order_code > 0)
+        order_code = env->regs[order_code];
+    order_code += (run->s390_sieic.ipb & 0x0fff0000) >> 16;
+
+    /* get parameters */
+    t = (ipa1 & 0xf0) >> 4;
+    if (!(t % 2))
+        t++;
+
+    parameter = env->regs[t] & 0x7ffffe00;
+    cpu_addr = env->regs[ipa1 & 0x0f];
+
+    target_env = s390_cpu_addr2state(cpu_addr);
+    if (!target_env)
+        goto out;
+
+    switch (order_code) {
+        case SIGP_RESTART:
+            r = s390_cpu_restart(target_env);
+            break;
+        case SIGP_STORE_STATUS_ADDR:
+            r = s390_store_status(target_env, parameter);
+            break;
+        case SIGP_SET_ARCH:
+            /* make the caller panic */
+            return -1;
+        case SIGP_INITIAL_CPU_RESET:
+            r = s390_cpu_initial_reset(target_env);
+            break;
+        default:
+            fprintf(stderr, "KVM: unknown SIGP: 0x%x\n", ipa1);
+            break;
+    }
+
+out:
+    setcc(env, r ? 3 : 0);
+    return 0;
+}
+
+static int handle_instruction(CPUState *env, struct kvm_run *run)
+{
+    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
+    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
+    int ipb_code = (run->s390_sieic.ipb & 0x0fff0000) >> 16;
+    int r = 0;
+
+    dprintf("handle_instruction 0x%x 0x%x\n", run->s390_sieic.ipa, run->s390_sieic.ipb);
+    switch (ipa0) {
+        case IPA0_PRIV:
+            r = handle_priv(env, run, ipa1);
+            break;
+        case IPA0_DIAG:
+            r = handle_diag(env, run, ipb_code);
+            break;
+        case IPA0_SIGP:
+            r = handle_sigp(env, run, ipa1);
+            break;
+    }
+
+    if (r < 0) {
+        enter_pgmcheck(env, 0x0001);
+    }
+    return r;
+}
+
+static int handle_intercept(CPUState *env)
+{
+    struct kvm_run *run = env->kvm_run;
+    int icpt_code = run->s390_sieic.icptcode;
+    int r = 0;
+
+    dprintf("intercept: 0x%x (at 0x%lx)\n", icpt_code, env->kvm_run->s390_sieic.addr);
+    switch (icpt_code) {
+        case ICPT_INSTRUCTION:
+            r = handle_instruction(env, run);
+            break;
+        case ICPT_WAITPSW:
+            /* XXX What to do on system shutdown? */
+            env->halted = 1;
+            env->exception_index = EXCP_HLT;
+            break;
+        case ICPT_SOFT_INTERCEPT:
+            fprintf(stderr, "KVM unimplemented icpt SOFT\n");
+            exit(1);
+            break;
+        case ICPT_CPU_STOP:
+            fprintf(stderr, "KVM unimplemented icpt STOP\n");
+            exit(1);
+            break;
+        case ICPT_IO:
+            fprintf(stderr, "KVM unimplemented icpt IO\n");
+            exit(1);
+            break;
+        default:
+            fprintf(stderr, "Unknown intercept code: %d\n", icpt_code);
+            exit(1);
+            break;
+    }
+
+    return r;
+}
+
+int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run)
+{
+    int ret = 0;
+
+    switch (run->exit_reason) {
+        case KVM_EXIT_S390_SIEIC:
+            ret = handle_intercept(env);
+            break;
+        case KVM_EXIT_S390_RESET:
+            fprintf(stderr, "RESET not implemented\n");
+            exit(1);
+            break;
+        default:
+            fprintf(stderr, "Unknown KVM exit: %d\n", run->exit_reason);
+            break;
+    }
+
+    return ret;
+}
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/9] Add support for S390x system emulation

[Alexander Graf]

Right now only S390x Linux userspace emulation is supported. Let's enable
the basics for system emulation so we can run virtual machines with KVM!

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-s390x/cpu.h       |   86 ++++++++++++++++++++++++++++++++++++++++++++-
 target-s390x/exec.h      |    5 +++
 target-s390x/helper.c    |   26 +++++++++++++-
 target-s390x/machine.c   |   30 ++++++++++++++++
 target-s390x/op_helper.c |   53 ++++++++++++++++++++++++++++
 5 files changed, 196 insertions(+), 4 deletions(-)
 create mode 100644 target-s390x/machine.c

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 93b09cd..c5ba7c3 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -30,8 +30,7 @@
 
 #include "softfloat.h"
 
-#define NB_MMU_MODES 2 // guess
-#define MMU_USER_IDX 0 // guess
+#define NB_MMU_MODES 2
 
 typedef union FPReg {
     struct {
@@ -77,12 +76,28 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
 }
 #endif
 
+#define MMU_MODE0_SUFFIX _kernel
+#define MMU_MODE1_SUFFIX _user
+#define MMU_USER_IDX 1
+static inline int cpu_mmu_index (CPUState *env)
+{
+    /* XXX: Currently we don't implement virtual memory */
+    return 0;
+}
+
+
 CPUS390XState *cpu_s390x_init(const char *cpu_model);
 void s390x_translate_init(void);
 int cpu_s390x_exec(CPUS390XState *s);
 void cpu_s390x_close(CPUS390XState *s);
 void do_interrupt (CPUState *env);
 
+#ifndef CONFIG_USER_ONLY
+extern int s390_virtio_hypercall(CPUState *env);
+extern void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token);
+extern CPUState *s390_cpu_addr2state(uint16_t cpu_addr);
+#endif
+
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
    is returned if the signal was handled by the virtual CPU.  */
@@ -129,4 +144,71 @@ static inline void cpu_get_tb_cpu_state(CPUState* env, target_ulong *pc,
     *cs_base = 0;
     *flags = env->psw.mask; // guess
 }
+
+/* Program Status Word.  */
+#define S390_PSWM_REGNUM 0
+#define S390_PSWA_REGNUM 1
+/* General Purpose Registers.  */
+#define S390_R0_REGNUM 2
+#define S390_R1_REGNUM 3
+#define S390_R2_REGNUM 4
+#define S390_R3_REGNUM 5
+#define S390_R4_REGNUM 6
+#define S390_R5_REGNUM 7
+#define S390_R6_REGNUM 8
+#define S390_R7_REGNUM 9
+#define S390_R8_REGNUM 10
+#define S390_R9_REGNUM 11
+#define S390_R10_REGNUM 12
+#define S390_R11_REGNUM 13
+#define S390_R12_REGNUM 14
+#define S390_R13_REGNUM 15
+#define S390_R14_REGNUM 16
+#define S390_R15_REGNUM 17
+/* Access Registers.  */
+#define S390_A0_REGNUM 18
+#define S390_A1_REGNUM 19
+#define S390_A2_REGNUM 20
+#define S390_A3_REGNUM 21
+#define S390_A4_REGNUM 22
+#define S390_A5_REGNUM 23
+#define S390_A6_REGNUM 24
+#define S390_A7_REGNUM 25
+#define S390_A8_REGNUM 26
+#define S390_A9_REGNUM 27
+#define S390_A10_REGNUM 28
+#define S390_A11_REGNUM 29
+#define S390_A12_REGNUM 30
+#define S390_A13_REGNUM 31
+#define S390_A14_REGNUM 32
+#define S390_A15_REGNUM 33
+/* Floating Point Control Word.  */
+#define S390_FPC_REGNUM 34
+/* Floating Point Registers.  */
+#define S390_F0_REGNUM 35
+#define S390_F1_REGNUM 36
+#define S390_F2_REGNUM 37
+#define S390_F3_REGNUM 38
+#define S390_F4_REGNUM 39
+#define S390_F5_REGNUM 40
+#define S390_F6_REGNUM 41
+#define S390_F7_REGNUM 42
+#define S390_F8_REGNUM 43
+#define S390_F9_REGNUM 44
+#define S390_F10_REGNUM 45
+#define S390_F11_REGNUM 46
+#define S390_F12_REGNUM 47
+#define S390_F13_REGNUM 48
+#define S390_F14_REGNUM 49
+#define S390_F15_REGNUM 50
+/* Total.  */
+#define S390_NUM_REGS 51
+
+/* Pseudo registers -- PC and condition code.  */
+#define S390_PC_REGNUM S390_NUM_REGS
+#define S390_CC_REGNUM (S390_NUM_REGS+1)
+#define S390_NUM_PSEUDO_REGS 2
+#define S390_NUM_TOTAL_REGS (S390_NUM_REGS+2)
+
+
 #endif
diff --git a/target-s390x/exec.h b/target-s390x/exec.h
index 5198359..13dc7dd 100644
--- a/target-s390x/exec.h
+++ b/target-s390x/exec.h
@@ -22,9 +22,14 @@
 
 register struct CPUS390XState *env asm(AREG0);
 
+#include "config.h"
 #include "cpu.h"
 #include "exec-all.h"
 
+#if !defined(CONFIG_USER_ONLY)
+#include "softmmu_exec.h"
+#endif /* !defined(CONFIG_USER_ONLY) */
+
 static inline int cpu_has_work(CPUState *env)
 {
     return env->interrupt_request & CPU_INTERRUPT_HARD; // guess
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 5407c62..d98d603 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -27,6 +27,9 @@
 #include "gdbstub.h"
 #include "qemu-common.h"
 
+#include <linux/kvm.h>
+#include "kvm.h"
+
 CPUS390XState *cpu_s390x_init(const char *cpu_model)
 {
     CPUS390XState *env;
@@ -61,13 +64,13 @@ int cpu_s390x_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
     return 1;
 }
 
+#endif /* CONFIG_USER_ONLY */
+
 target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 {
     return addr;
 }
 
-#endif /* CONFIG_USER_ONLY */
-
 void cpu_reset(CPUS390XState *env)
 {
     if (qemu_loglevel_mask(CPU_LOG_RESET)) {
@@ -79,3 +82,22 @@ void cpu_reset(CPUS390XState *env)
     /* FIXME: reset vector? */
     tlb_flush(env, 1);
 }
+
+#ifndef CONFIG_USER_ONLY
+
+int cpu_s390x_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
+                                int mmu_idx, int is_softmmu)
+{
+    target_ulong phys;
+    int prot;
+
+    /* XXX: implement mmu */
+
+    phys = address;
+    prot = PAGE_READ | PAGE_WRITE;
+
+    return tlb_set_page(env, address & TARGET_PAGE_MASK,
+                        phys & TARGET_PAGE_MASK, prot,
+                        mmu_idx, is_softmmu);
+}
+#endif /* CONFIG_USER_ONLY */
diff --git a/target-s390x/machine.c b/target-s390x/machine.c
new file mode 100644
index 0000000..3e79be6
--- /dev/null
+++ b/target-s390x/machine.c
@@ -0,0 +1,30 @@
+/*
+ * QEMU S390x machine definitions
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/hw.h"
+#include "hw/boards.h"
+
+void cpu_save(QEMUFile *f, void *opaque)
+{
+}
+
+int cpu_load(QEMUFile *f, void *opaque, int version_id)
+{
+    return 0;
+}
diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
index 5de4d08..7b88225 100644
--- a/target-s390x/op_helper.c
+++ b/target-s390x/op_helper.c
@@ -1717,3 +1717,56 @@ void HELPER(sqdbr)(uint32_t f1, uint32_t f2)
 {
     env->fregs[f1].d = float64_sqrt(env->fregs[f2].d, &env->fpu_status);
 }
+
+/*****************************************************************************/
+/* Softmmu support */
+#if !defined (CONFIG_USER_ONLY)
+
+#define MMUSUFFIX _mmu
+
+#define SHIFT 0
+#include "softmmu_template.h"
+
+#define SHIFT 1
+#include "softmmu_template.h"
+
+#define SHIFT 2
+#include "softmmu_template.h"
+
+#define SHIFT 3
+#include "softmmu_template.h"
+
+/* try to fill the TLB and return an exception if error. If retaddr is
+   NULL, it means that the function was called in C code (i.e. not
+   from generated code or from helper.c) */
+/* XXX: fix it to restore all registers */
+void tlb_fill (target_ulong addr, int is_write, int mmu_idx, void *retaddr)
+{
+    TranslationBlock *tb;
+    CPUState *saved_env;
+    unsigned long pc;
+    int ret;
+
+    /* XXX: hack to restore env in all cases, even if not called from
+       generated code */
+    saved_env = env;
+    env = cpu_single_env;
+    ret = cpu_s390x_handle_mmu_fault(env, addr, is_write, mmu_idx, 1);
+    if (unlikely(ret != 0)) {
+        if (likely(retaddr)) {
+            /* now we have a real cpu fault */
+            pc = (unsigned long)retaddr;
+            tb = tb_find_pc(pc);
+            if (likely(tb)) {
+                /* the PC is inside the translated code. It means that we have
+                   a virtual CPU fault */
+                cpu_restore_state(tb, env, pc, NULL);
+            }
+        }
+        /* XXX */
+        /* helper_raise_exception_err(env->exception_index, env->error_code); */
+    }
+    env = saved_env;
+}
+
+#endif
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/9] Add S390x virtio machine bus

[Alexander Graf]

On S390x we don't want to go through the hassle of emulating real existing
hardware, because we don't need to for running Linux.

So let's instead implement a machine that is 100% based on VirtIO which we
fortunately implement already.

This patch implements the bus that is the groundwork for such an S390x
virtio machine.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Makefile.target      |    2 +
 hw/s390-virtio-bus.c |  350 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390-virtio-bus.h |   64 +++++++++
 3 files changed, 416 insertions(+), 0 deletions(-)
 create mode 100644 hw/s390-virtio-bus.c
 create mode 100644 hw/s390-virtio-bus.h

diff --git a/Makefile.target b/Makefile.target
index 8d146c5..ef72867 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -294,6 +294,8 @@ obj-sh4-y += ide/core.o ide/mmio.o
 obj-m68k-y = an5206.o mcf5206.o mcf_uart.o mcf_intc.o mcf5208.o mcf_fec.o
 obj-m68k-y += m68k-semi.o dummy_m68k.o
 
+obj-s390x-y = s390-virtio-bus.o
+
 main.o vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
 vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
new file mode 100644
index 0000000..b925c30
--- /dev/null
+++ b/hw/s390-virtio-bus.c
@@ -0,0 +1,350 @@
+/*
+ * QEMU S390 virtio target
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw.h"
+#include "block.h"
+#include "sysemu.h"
+#include "net.h"
+#include "boards.h"
+#include "monitor.h"
+#include "loader.h"
+#include "elf.h"
+#include "hw/virtio.h"
+#include "hw/virtio-console.h"
+#include "hw/sysbus.h"
+#include "kvm.h"
+
+#include "hw/s390-virtio-bus.h"
+
+//#define DEBUG_S390
+
+#ifdef DEBUG_S390
+#define dprintf(fmt, ...) \
+    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#else
+#define dprintf(fmt, ...) \
+    do { } while (0)
+#endif
+
+struct BusInfo s390_virtio_bus_info = {
+    .name       = "s390-virtio",
+    .size       = sizeof(VirtIOS390Bus),
+};
+
+typedef struct {
+    DeviceInfo qdev;
+    int (*init)(VirtIOS390Device *dev);
+} VirtIOS390DeviceInfo;
+
+
+static const VirtIOBindings virtio_s390_bindings;
+
+static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev);
+static void s390_virtio_device_sync(VirtIOS390Device *dev);
+
+VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
+{
+    VirtIOS390Bus *bus;
+
+    bus = (VirtIOS390Bus *)qbus_create(&s390_virtio_bus_info, NULL, "s390-virtio");
+
+    bus->dev_page = *ram_size;
+    bus->dev_offs = bus->dev_page;
+    bus->next_ring = bus->dev_page + TARGET_PAGE_SIZE;
+
+    /* Allocate RAM for VirtIO device pages (descriptors, queues, rings) */
+    *ram_size += S390_DEVICE_PAGES * TARGET_PAGE_SIZE;
+
+    return bus;
+}
+
+static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
+{
+    VirtIOS390Bus *bus;
+    int dev_len;
+
+    bus = (VirtIOS390Bus *)dev->qdev.parent_bus;
+    dev->vdev = vdev;
+    dev->dev_offs = bus->dev_offs;
+    dev->feat_len = sizeof(uint32_t); /* always keep 32 bits features */
+
+    dev_len = VIRTIO_DEV_OFFS_CONFIG;
+    dev_len += s390_virtio_device_num_vq(dev) * VIRTIO_VQCONFIG_LEN;
+    dev_len += dev->feat_len * 2;
+    dev_len += vdev->config_len;
+    
+    bus->dev_offs += dev_len;
+
+    virtio_bind_device(vdev, &virtio_s390_bindings, dev);
+    s390_virtio_device_sync(dev);
+
+    return 0;
+}
+
+static int s390_virtio_net_init(VirtIOS390Device *dev)
+{
+    VirtIODevice *vdev;
+
+    vdev = virtio_net_init((DeviceState *)dev);
+    if (!vdev)
+        return -1;
+
+    return s390_virtio_device_init(dev, vdev);
+}
+
+static int s390_virtio_blk_init(VirtIOS390Device *dev)
+{
+    VirtIODevice *vdev;
+
+    vdev = virtio_blk_init((DeviceState *)dev, dev->dinfo);
+    if (!vdev)
+        return -1;
+
+    return s390_virtio_device_init(dev, vdev);
+}
+
+static int s390_virtio_console_init(VirtIOS390Device *dev)
+{
+    VirtIOS390Bus *bus;
+    VirtIODevice *vdev;
+    int r;
+
+    bus = (VirtIOS390Bus *)dev->qdev.parent_bus;
+
+    vdev = virtio_console_init((DeviceState *)dev);
+    if (!vdev)
+        return -1;
+
+    r = s390_virtio_device_init(dev, vdev);
+    if (!r)
+        bus->console = dev;
+
+    return r;
+}
+
+static uint64_t s390_virtio_device_vq_token(VirtIOS390Device *dev, int vq)
+{
+    ram_addr_t token_off;
+
+    token_off = (dev->dev_offs + VIRTIO_DEV_OFFS_CONFIG) +
+                (vq * VIRTIO_VQCONFIG_LEN) +
+                VIRTIO_VQCONFIG_OFFS_TOKEN;
+
+    return ldq_phys(token_off);
+}
+
+static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev)
+{
+    VirtIODevice *vdev = dev->vdev;
+    int num_vq;
+
+    for (num_vq = 0; num_vq < VIRTIO_PCI_QUEUE_MAX; num_vq++) {
+        if (!virtio_queue_get_num(vdev, num_vq))
+            break;
+    }
+
+    return num_vq;
+}
+
+static ram_addr_t s390_virtio_next_ring(VirtIOS390Bus *bus)
+{
+    ram_addr_t r = bus->next_ring;
+
+    bus->next_ring += VIRTIO_RING_LEN;
+    return r;
+}
+
+static void s390_virtio_device_sync(VirtIOS390Device *dev)
+{
+    VirtIOS390Bus *bus = (VirtIOS390Bus *)dev->qdev.parent_bus;
+    ram_addr_t cur_offs;
+    uint8_t num_vq;
+    int i;
+
+    virtio_reset(dev->vdev);
+
+    /* Sync dev space */
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_TYPE, dev->vdev->device_id);
+
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_NUM_VQ, s390_virtio_device_num_vq(dev));
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_FEATURE_LEN, dev->feat_len);
+
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_CONFIG_LEN, dev->vdev->config_len);
+
+    num_vq = s390_virtio_device_num_vq(dev);
+    stb_phys(dev->dev_offs + VIRTIO_DEV_OFFS_NUM_VQ, num_vq);
+
+    /* Sync virtqueues */
+    for (i = 0; i < num_vq; i++) {
+        ram_addr_t vq = (dev->dev_offs + VIRTIO_DEV_OFFS_CONFIG) +
+                        (i * VIRTIO_VQCONFIG_LEN);
+        ram_addr_t vring;
+
+        vring = s390_virtio_next_ring(bus);
+        virtio_queue_set_addr(dev->vdev, i, vring);
+        virtio_queue_set_vector(dev->vdev, i, i);
+        stq_phys(vq + VIRTIO_VQCONFIG_OFFS_ADDRESS, vring);
+        stw_phys(vq + VIRTIO_VQCONFIG_OFFS_NUM, virtio_queue_get_num(dev->vdev, i));
+    }
+
+    cur_offs = dev->dev_offs;
+    cur_offs += VIRTIO_DEV_OFFS_CONFIG;
+    cur_offs += num_vq * VIRTIO_VQCONFIG_LEN;
+
+    /* Sync feature bitmap */
+    if (dev->vdev->get_features)
+        stl_phys(cur_offs, dev->vdev->get_features(dev->vdev));
+
+    dev->feat_offs = cur_offs + dev->feat_len;
+    cur_offs += dev->feat_len * 2;
+
+    /* Sync config space */
+    if (dev->vdev->get_config)
+        dev->vdev->get_config(dev->vdev, dev->vdev->config);
+
+    cpu_physical_memory_rw(cur_offs, dev->vdev->config, dev->vdev->config_len, 1);
+    cur_offs += dev->vdev->config_len;
+}
+
+void s390_virtio_device_update_status(VirtIOS390Device *dev)
+{
+    VirtIODevice *vdev = dev->vdev;
+    uint32_t features;
+
+    vdev->status = ldub_phys(dev->dev_offs + VIRTIO_DEV_OFFS_STATUS);
+
+    /* Update guest supported feature bitmap */
+
+    features = ldl_phys(dev->feat_offs);
+    if (vdev->set_features)
+        vdev->set_features(vdev, features);
+    vdev->features = features;
+}
+
+VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
+{
+    return bus->console;
+}
+
+/* Find a device by vring address */
+VirtIOS390Device *s390_virtio_bus_find_vring(VirtIOS390Bus *bus,
+                                             ram_addr_t mem,
+                                             int *vq_num)
+{
+    VirtIOS390Device *_dev;
+    DeviceState *dev;
+    int i;
+
+    QLIST_FOREACH(dev, &bus->bus.children, sibling) {
+        _dev = (VirtIOS390Device *)dev;
+        for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+            if (!virtio_queue_get_addr(_dev->vdev, i))
+                break;
+            if (virtio_queue_get_addr(_dev->vdev, i) == mem) {
+                if (vq_num) *vq_num = i;
+                return _dev;
+            }
+        }
+    }
+
+    return NULL;
+}
+
+/* Find a device by device descriptor location */
+VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem)
+{
+    VirtIOS390Device *_dev;
+    DeviceState *dev;
+
+    QLIST_FOREACH(dev, &bus->bus.children, sibling) {
+        _dev = (VirtIOS390Device *)dev;
+        if (_dev->dev_offs == mem) {
+            return _dev;
+        }
+    }
+
+    return NULL;
+}
+
+static void virtio_s390_notify(void *opaque, uint16_t vector)
+{
+    VirtIOS390Device *dev = (VirtIOS390Device*)opaque;
+    uint64_t token = s390_virtio_device_vq_token(dev, vector);
+
+    /* XXX kvm dependency! */
+    kvm_s390_virtio_irq(s390_cpu_addr2state(0), 1, token);
+}
+
+static const VirtIOBindings virtio_s390_bindings = {
+    .notify = virtio_s390_notify,
+};
+
+static VirtIOS390DeviceInfo s390_virtio_net = {
+    .init = s390_virtio_net_init,
+    .qdev.name = "virtio-net-s390",
+    .qdev.size = sizeof(VirtIOS390Device),
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static VirtIOS390DeviceInfo s390_virtio_blk = {
+    .init = s390_virtio_blk_init,
+    .qdev.name = "virtio-blk-s390",
+    .qdev.size = sizeof(VirtIOS390Device),
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_DRIVE("drive", VirtIOS390Device, dinfo),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static VirtIOS390DeviceInfo s390_virtio_console = {
+    .init = s390_virtio_console_init,
+    .qdev.name = "virtio-console-s390",
+    .qdev.size = sizeof(VirtIOS390Device),
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static int s390_virtio_busdev_init(DeviceState *dev, DeviceInfo *info)
+{
+    VirtIOS390DeviceInfo *_info = (VirtIOS390DeviceInfo *)info;
+    VirtIOS390Device *_dev = (VirtIOS390Device *)dev;
+
+    return _info->init(_dev);
+}
+
+static void s390_virtio_bus_register_withprop(VirtIOS390DeviceInfo *info)
+{
+    info->qdev.init = s390_virtio_busdev_init;
+    info->qdev.bus_info = &s390_virtio_bus_info;
+
+    assert(info->qdev.size >= sizeof(VirtIOS390Device));
+    qdev_register(&info->qdev);
+}
+
+static void s390_virtio_register(void)
+{
+    s390_virtio_bus_register_withprop(&s390_virtio_console);
+    s390_virtio_bus_register_withprop(&s390_virtio_blk);
+    s390_virtio_bus_register_withprop(&s390_virtio_net);
+}
+device_init(s390_virtio_register);
+
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
new file mode 100644
index 0000000..383074b
--- /dev/null
+++ b/hw/s390-virtio-bus.h
@@ -0,0 +1,64 @@
+/*
+ * QEMU S390x VirtIO BUS definitions
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define VIRTIO_DEV_OFFS_TYPE		0	/* 8 bits */
+#define VIRTIO_DEV_OFFS_NUM_VQ		1	/* 8 bits */
+#define VIRTIO_DEV_OFFS_FEATURE_LEN	2	/* 8 bits */
+#define VIRTIO_DEV_OFFS_CONFIG_LEN	3	/* 8 bits */
+#define VIRTIO_DEV_OFFS_STATUS		4	/* 8 bits */
+#define VIRTIO_DEV_OFFS_CONFIG		5	/* dynamic */
+
+#define VIRTIO_VQCONFIG_OFFS_TOKEN	0	/* 64 bits */
+#define VIRTIO_VQCONFIG_OFFS_ADDRESS	8	/* 64 bits */
+#define VIRTIO_VQCONFIG_OFFS_NUM	16	/* 16 bits */
+#define VIRTIO_VQCONFIG_LEN		24
+
+#define VIRTIO_RING_LEN			(TARGET_PAGE_SIZE * 3)
+#define S390_DEVICE_PAGES		256
+
+typedef struct VirtIOS390Device {
+    DeviceState qdev;
+    ram_addr_t dev_offs;
+    ram_addr_t feat_offs;
+    uint8_t feat_len;
+    VirtIODevice *vdev;
+    DriveInfo *dinfo;
+} VirtIOS390Device;
+
+typedef struct VirtIOS390Bus {
+    BusState bus;
+
+    VirtIOS390Device *console;
+    ram_addr_t dev_page;
+    ram_addr_t dev_offs;
+    ram_addr_t next_ring;
+} VirtIOS390Bus;
+
+
+extern void s390_virtio_device_update_status(VirtIOS390Device *dev);
+
+extern VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus);
+extern VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size);
+
+extern VirtIOS390Device *s390_virtio_bus_find_vring(VirtIOS390Bus *bus,
+                                                    ram_addr_t mem,
+                                                    int *vq_num);
+extern VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus,
+                                                  ram_addr_t mem);
+
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/9] Add S390x virtio machine description

[Alexander Graf]

In order to use the new S390x virtio bus we just introduced, we also
need a machine description that sets up the machine according to our
PV specification.

Let's add that machine description and be happy!

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Makefile.target  |    2 +-
 hw/s390-virtio.c |  244 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 245 insertions(+), 1 deletions(-)
 create mode 100644 hw/s390-virtio.c

diff --git a/Makefile.target b/Makefile.target
index ef72867..524be43 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -294,7 +294,7 @@ obj-sh4-y += ide/core.o ide/mmio.o
 obj-m68k-y = an5206.o mcf5206.o mcf_uart.o mcf_intc.o mcf5208.o mcf_fec.o
 obj-m68k-y += m68k-semi.o dummy_m68k.o
 
-obj-s390x-y = s390-virtio-bus.o
+obj-s390x-y = s390-virtio-bus.o s390-virtio.o 
 
 main.o vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
new file mode 100644
index 0000000..cd62e54
--- /dev/null
+++ b/hw/s390-virtio.c
@@ -0,0 +1,244 @@
+/*
+ * QEMU S390 virtio target
+ *
+ * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw.h"
+#include "block.h"
+#include "sysemu.h"
+#include "net.h"
+#include "boards.h"
+#include "monitor.h"
+#include "loader.h"
+#include "elf.h"
+#include "hw/virtio.h"
+#include "hw/virtio-console.h"
+#include "hw/sysbus.h"
+#include "kvm.h"
+
+#include "hw/s390-virtio-bus.h"
+
+//#define DEBUG_S390
+
+#ifdef DEBUG_S390
+#define dprintf(fmt, ...) \
+    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#else
+#define dprintf(fmt, ...) \
+    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
+#define INITRD_PARM_START               0x010408UL
+#define INITRD_PARM_SIZE                0x010410UL
+#define PARMFILE_START                  0x001000UL
+
+#define MAX_BLK_DEVS                    10
+
+static VirtIOS390Bus *s390_bus;
+static CPUState **ipi_states;
+
+void irq_info(Monitor *mon);
+void pic_info(Monitor *mon);
+
+void irq_info(Monitor *mon)
+{
+}
+
+void pic_info(Monitor *mon)
+{
+}
+
+CPUState *s390_cpu_addr2state(uint16_t cpu_addr)
+{
+    if (cpu_addr >= smp_cpus)
+        return NULL;
+
+    return ipi_states[cpu_addr];
+}
+
+int s390_virtio_hypercall(CPUState *env)
+{
+    int r = 0, i;
+    target_ulong mem = env->regs[2];
+
+    dprintf("KVM hypercall: %ld\n", env->regs[1]);
+    switch (env->regs[1]) {
+    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 */
+            uint8_t *p = (uint8_t *)qemu_get_ram_ptr(mem);
+            VirtIOS390Device *dev = s390_virtio_bus_console(s390_bus);
+            virtio_console_print_early(dev->vdev, p);
+        }
+        break;
+    case KVM_S390_VIRTIO_RESET:
+    {
+        /* Virtio_reset resets the internal addresses, so we'd have to sync
+           them up again. We don't want to reallicate a vring though, so let's
+           just not reset. */
+        /* virtio_reset(dev->vdev); */
+        break;
+    }
+    case KVM_S390_VIRTIO_SET_STATUS:
+    {
+        VirtIOS390Device *dev;
+
+        dev = s390_virtio_bus_find_mem(s390_bus, mem);
+        if (dev)
+            s390_virtio_device_update_status(dev);
+        else
+            r = -EINVAL;
+        break;
+    }
+    default:
+        r = -EINVAL;
+        break;
+    }
+
+    env->regs[2] = r;
+    return 0;
+}
+
+/* PC hardware initialisation */
+static void s390_init(ram_addr_t _ram_size,
+                      const char *boot_device,
+                      const char *kernel_filename,
+                      const char *kernel_cmdline,
+                      const char *initrd_filename,
+                      const char *cpu_model)
+{
+    CPUState *env = NULL;
+    ram_addr_t ram_addr, ram_size = _ram_size;
+    ram_addr_t kernel_size = 0;
+    ram_addr_t initrd_offset;
+    ram_addr_t initrd_size = 0;
+    int i;
+
+    /* get a BUS */
+    s390_bus = s390_virtio_bus_init(&ram_size);
+
+    /* allocate RAM */
+    /* S390 requires the topmost virtual address of the RAM to be < 256GB */
+    ram_addr = _qemu_ram_alloc(ram_size, (void*)0x1000000);
+    cpu_register_physical_memory(0, ram_size, ram_addr);
+
+    /* init CPUs */
+    if (cpu_model == NULL) {
+        cpu_model = "host";
+    }
+
+    ipi_states = qemu_malloc(sizeof(CPUState *) * smp_cpus);
+
+    for (i = 0; i < smp_cpus; i++) {
+        env = cpu_init(cpu_model);
+        ipi_states[smp_cpus - (i + 1)] = env;
+        env->halted = 1;
+        env->exception_index = EXCP_HLT;
+    }
+
+    env->halted = 0;
+    env->exception_index = 0;
+
+    if (kernel_filename) {
+        kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
+
+        if (lduw_phys(KERN_IMAGE_START) != 0x0dd0) {
+            fprintf(stderr, "Specified image is not an s390 boot image\n");
+            exit(1);
+        }
+
+        cpu_synchronize_state(env);
+        env->psw.addr = KERN_IMAGE_START;
+        env->psw.mask = 0x0000000180000000UL;
+    }
+
+    if (initrd_filename) {
+        initrd_offset = INITRD_START;
+        while (kernel_size + 0x100000 > initrd_offset)
+            initrd_offset += 0x100000;
+        initrd_size = load_image(initrd_filename, qemu_get_ram_ptr(initrd_offset));
+
+        stq_phys(INITRD_PARM_START, initrd_offset);
+        stq_phys(INITRD_PARM_SIZE, initrd_size);
+    }
+
+    if (kernel_cmdline) {
+        cpu_physical_memory_rw(KERN_PARM_AREA, (uint8_t *)kernel_cmdline,
+                               strlen(kernel_cmdline), 1);
+    }
+
+    /* Create VirtIO console */
+    qdev_init_nofail(qdev_create((BusState *)s390_bus, "virtio-console-s390"));
+
+    /* Create VirtIO network adapters */
+    for(i = 0; i < nb_nics; i++) {
+        NICInfo *nd = &nd_table[i];
+        DeviceState *dev;
+
+        if (!nd->model)
+            nd->model = (char*)"virtio";
+
+        dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
+        dev->nd = nd;
+        qdev_init_nofail(dev);
+    }
+
+    /* Create VirtIO disk drives */
+    for(i = 0; i < MAX_BLK_DEVS; i++) {
+        DriveInfo *dinfo;
+        DeviceState *dev;
+
+        dinfo = drive_get(IF_IDE, 0, i);
+        if (!dinfo)
+            continue;
+
+        dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
+        qdev_prop_set_drive(dev, "drive", dinfo);
+        qdev_init_nofail(dev);
+    }
+}
+
+static QEMUMachine s390_machine = {
+    .name = "s390-virtio",
+    .alias = "s390",
+    .desc = "VirtIO based S390 machine",
+    .init = s390_init,
+    .max_cpus = 255,
+    .is_default = 1,
+};
+
+static void s390_machine_init(void)
+{
+    qemu_register_machine(&s390_machine);
+}
+
+machine_init(s390_machine_init);
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/9] S390 GDB stub

[Alexander Graf]

In order to debug funny kernel breakages it's always good to have a working
gdb stub around.

While Uli's patches don't include one one, I needed one that's at least good
enough for 'bt' and some variable examinations during early bootup.

So here it is - the absolute basics to get the qemu gdb stub running with s390x
targets.

Sgined-off-by: Alexander Graf <agraf@suse.de>
---
 gdbstub.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 315f606..49f6fe3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1348,6 +1348,55 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
 
     return 8;
 }
+#elif defined (TARGET_S390X)
+
+#define NUM_CORE_REGS S390_NUM_TOTAL_REGS
+
+static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
+{
+    switch (n) {
+        case S390_PSWM_REGNUM: GET_REGL(env->psw.mask); break;
+        case S390_PSWA_REGNUM: GET_REGL(env->psw.addr); break;
+        case S390_R0_REGNUM ... S390_R15_REGNUM:
+            GET_REGL(env->regs[n-S390_R0_REGNUM]); break;
+        case S390_A0_REGNUM ... S390_A15_REGNUM:
+            GET_REG32(env->aregs[n-S390_A0_REGNUM]); break;
+        case S390_FPC_REGNUM: GET_REG32(env->fpc); break;
+        case S390_F0_REGNUM ... S390_F15_REGNUM:
+            /* XXX */
+            break;
+        case S390_PC_REGNUM: GET_REGL(env->psw.addr); break;
+        case S390_CC_REGNUM: GET_REG32(env->cc); break;
+    }
+
+    return 0;
+}
+
+static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
+{
+    target_ulong tmpl;
+    uint32_t tmp32;
+    int r = 8;
+    tmpl = ldtul_p(mem_buf);
+    tmp32 = ldl_p(mem_buf);
+
+    switch (n) {
+        case S390_PSWM_REGNUM: env->psw.mask = tmpl; break;
+        case S390_PSWA_REGNUM: env->psw.addr = tmpl; break;
+        case S390_R0_REGNUM ... S390_R15_REGNUM:
+            env->regs[n-S390_R0_REGNUM] = tmpl; break;
+        case S390_A0_REGNUM ... S390_A15_REGNUM:
+            env->aregs[n-S390_A0_REGNUM] = tmp32; r=4; break;
+        case S390_FPC_REGNUM: env->fpc = tmp32; r=4; break;
+        case S390_F0_REGNUM ... S390_F15_REGNUM:
+            /* XXX */
+            break;
+        case S390_PC_REGNUM: env->psw.addr = tmpl; break;
+        case S390_CC_REGNUM: env->cc = tmp32; r=4; break;
+    }
+
+    return r;
+}
 #else
 
 #define NUM_CORE_REGS 0
@@ -1616,6 +1665,9 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
     s->c_cpu->pc = pc;
 #elif defined (TARGET_ALPHA)
     s->c_cpu->pc = pc;
+#elif defined (TARGET_S390X)
+    cpu_synchronize_state(s->c_cpu);
+    s->c_cpu->psw.addr = pc;
 #endif
 }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 7/9] Implement early printk in virtio-console

[Alexander Graf]

On our S390x Virtio machine we don't have anywhere to display early printks
on, because we don't know about VGA or serial ports.

So instead we just forward everything to the virtio console that we created
anyways.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/virtio-console.c |    7 +++++++
 hw/virtio-console.h |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 57f8f89..cd6cf20 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -105,6 +105,13 @@ static void vcon_event(void *opaque, int event)
     /* we will ignore any event for the time being */
 }
 
+void virtio_console_print_early(VirtIODevice *vdev, uint8_t *buf)
+{
+    VirtIOConsole *s = to_virtio_console(vdev);
+
+    qemu_chr_write(s->chr, buf, strlen((char*)buf));
+}
+
 static void virtio_console_save(QEMUFile *f, void *opaque)
 {
     VirtIOConsole *s = opaque;
diff --git a/hw/virtio-console.h b/hw/virtio-console.h
index 84d0717..f3ccc3c 100644
--- a/hw/virtio-console.h
+++ b/hw/virtio-console.h
@@ -16,4 +16,6 @@
 /* The ID for virtio console */
 #define VIRTIO_ID_CONSOLE 3
 
+void virtio_console_print_early(VirtIODevice *vdev, uint8_t *buf);
+
 #endif
-- 
1.6.0.2

[Qemu-devel] [PATCH 8/9] Set default console to virtio on S390x

[Alexander Graf]

All "normal" system emulation targets in qemu I'm aware of display output
on either VGA or serial output.

Our S390x virtio machine doesn't have such kind of legacy hardware. So
instead we need to default to a virtio console.

I'm not particularly proud of this patch. It would be a lot better to
have something in the machine description that tells us about the default
terminal.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 vl.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 96838f8..f3654ff 100644
--- a/vl.c
+++ b/vl.c
@@ -4818,6 +4818,20 @@ int main(int argc, char **argv, char **envp)
     cyls = heads = secs = 0;
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
+#ifdef TARGET_S390X
+    for(i = 0; i < MAX_SERIAL_PORTS; i++)
+        serial_devices[i] = NULL;
+    serial_device_index = 0;
+
+    for(i = 0; i < MAX_PARALLEL_PORTS; i++)
+        parallel_devices[i] = NULL;
+    parallel_device_index = 0;
+
+    virtio_consoles[0] = "mon:stdio";
+    for(i = 1; i < MAX_VIRTIO_CONSOLES; i++)
+        virtio_consoles[i] = NULL;
+    virtio_console_index = 0;
+#else
     serial_devices[0] = "vc:80Cx24C";
     for(i = 1; i < MAX_SERIAL_PORTS; i++)
         serial_devices[i] = NULL;
@@ -4831,6 +4845,7 @@ int main(int argc, char **argv, char **envp)
     for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
         virtio_consoles[i] = NULL;
     virtio_console_index = 0;
+#endif
 
     monitor_devices[0] = "vc:80Cx24C";
     for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
@@ -5709,6 +5724,17 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
         }
+        for (i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
+            const char *devname = virtio_consoles[i];
+            if (devname && !strcmp(devname,"mon:stdio")) {
+                monitor_devices[0] = NULL;
+                break;
+            } else if (devname && !strcmp(devname,"stdio")) {
+                monitor_devices[0] = NULL;
+                virtio_consoles[i] = "mon:stdio";
+                break;
+            }
+        }
     }
 
     if (nb_numa_nodes > 0) {
-- 
1.6.0.2

[Qemu-devel] [PATCH 9/9] Move mp_state to CPU_COMMON

[Alexander Graf]

MP State is implemented in the generic code, so let's move the variable
it accesses to generic code as well.

Still unbreaks PPC and now even S390x w/ KVM.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 cpu-defs.h        |    1 +
 target-i386/cpu.h |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 95068b5..ad804e6 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -197,6 +197,7 @@ typedef struct CPUWatchpoint {
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
     struct kvm_run *kvm_run;                                            \
+    uint32_t mp_state;                                                  \
     int kvm_fd;
 
 #endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5929d28..8d0c2a9 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -692,7 +692,6 @@ typedef struct CPUX86State {
 
     /* For KVM */
     uint64_t interrupt_bitmap[256 / 64];
-    uint32_t mp_state;
 
     /* in order to simplify APIC support, we leave this pointer to the
        user */
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 0/9] S390x KVM support

[Gerd Hoffmann]

   Hi,

> So now you can run Linux VMs on Linux on z/VM on LPAR on zSeries!

Cool.  Don't have a zSeries though ...

> The patchset is based on Uli's S390x userspace emulation patches. There's not
> really that much shared functionality, but I didn't want to reimplement the
> configure wheels. So make sure to have his patches applied first.

Hmm.  How hard would it be to wind up the cpu emulation bits so I can 
run s390x virtual machines on my x86 box?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 19.10.2009, at 21:24, Gerd Hoffmann wrote:

>  Hi,
>
>> So now you can run Linux VMs on Linux on z/VM on LPAR on zSeries!
>
> Cool.  Don't have a zSeries though ...

I always thought that's commodity hardware .. oh well :-)

>> The patchset is based on Uli's S390x userspace emulation patches.  
>> There's not
>> really that much shared functionality, but I didn't want to  
>> reimplement the
>> configure wheels. So make sure to have his patches applied first.
>
> Hmm.  How hard would it be to wind up the cpu emulation bits so I  
> can run s390x virtual machines on my x86 box?

It's feasible, but currently not implemented. I also don't know enough  
of the s390x internals to do it as is :-). On S390 the hardware and OS  
handle most of the virtualization parts themselves.

Uli, any comments on this?

Either way - let's focus on getting it working with KVM first!

Alex

Re: [Qemu-devel] [PATCH 4/9] Add S390x virtio machine bus

[Gerd Hoffmann]

> +VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
> +{
> +    VirtIOS390Bus *bus;
> +
> +    bus = (VirtIOS390Bus *)qbus_create(&s390_virtio_bus_info, NULL, "s390-virtio");

While a cast works the political correct way to do this is using DO_UPCAST.

Also I'd suggest to add a sysbus -> s390-virtio bus bridge device, so 
your bus gets properly hooked up in the device tree.  Have a look at the 
(quite simple) "isabus-bridge" device in isa-bus.c, which is used with 
'qemu -M isapc'.  You'll see the s390-virtio bus and all virtio devices 
attached to it in 'info qtree' then.

> +    bus = (VirtIOS390Bus *)dev->qdev.parent_bus;

DO_UPCAST().

> +VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
> +{
> +    return bus->console;
> +}

What this is needed for?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/9] Add S390x virtio machine bus

[Alexander Graf]

On 19.10.2009, at 21:34, Gerd Hoffmann wrote:

>> +VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
>> +{
>> +    VirtIOS390Bus *bus;
>> +
>> +    bus = (VirtIOS390Bus *)qbus_create(&s390_virtio_bus_info,  
>> NULL, "s390-virtio");
>
> While a cast works the political correct way to do this is using  
> DO_UPCAST.

Oh - that one's new to me :-)

> Also I'd suggest to add a sysbus -> s390-virtio bus bridge device,  
> so your bus gets properly hooked up in the device tree.  Have a look  
> at the (quite simple) "isabus-bridge" device in isa-bus.c, which is  
> used with 'qemu -M isapc'.  You'll see the s390-virtio bus and all  
> virtio devices attached to it in 'info qtree' then.

Uh, ok :o. Is that really important?

>> +    bus = (VirtIOS390Bus *)dev->qdev.parent_bus;
>
> DO_UPCAST().
>
>> +VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
>> +{
>> +    return bus->console;
>> +}
>
> What this is needed for?

We need to know the console to do early printk. I figured it'd be  
better to have a function returning it, so we could possibly change  
semantics later.

Alex

Re: [Qemu-devel] [PATCH 8/9] Set default console to virtio on S390x

[Gerd Hoffmann]

On 10/19/09 16:37, Alexander Graf wrote:
> All "normal" system emulation targets in qemu I'm aware of display output
> on either VGA or serial output.
>
> Our S390x virtio machine doesn't have such kind of legacy hardware. So
> instead we need to default to a virtio console.
>
> I'm not particularly proud of this patch. It would be a lot better to
> have something in the machine description that tells us about the default
> terminal.

One more reason to move the default devices qemu creates to a (machine 
specific) config file ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/9] Add S390x virtio machine bus

[Gerd Hoffmann]

   Hi,

>> Also I'd suggest to add a sysbus -> s390-virtio bus bridge device, so
>> your bus gets properly hooked up in the device tree. Have a look at
>> the (quite simple) "isabus-bridge" device in isa-bus.c, which is used
>> with 'qemu -M isapc'. You'll see the s390-virtio bus and all virtio
>> devices attached to it in 'info qtree' then.
>
> Uh, ok :o. Is that really important?

Quite a few things depend on it.  I think creating disks via '-device 
virtio-blk-s390' will not work without that ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/19/2009 11:37 PM, Alexander Graf wrote:
> While S390x was one of the first targets that were supported by KVM it always
> lacked qemu system emulation support.
>
> In order to change that sad fact, I figured I'd just take on the task myself,
> taking kuli (http://www.ibm.com/developerworks/linux/linux390/kuli.html),
> Documentation/s390/kvm.txt and lguest as starting points to write a clean,
> qemu'ish S390x Virtio machine emulation.
>
> So now you can run Linux VMs on Linux on z/VM on LPAR on zSeries!
>
> This is the resulting code. Please comment on things you like and also on the
> ones you don't :-).
>    

Didn't find anything to dislike.  It all looks good!

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH 1/9] Export function for VA defined ram allocation

[Carsten Otte]

Alexander Graf wrote:
> S390 requires vmas for guests to be < 256 GB. So we need to directly export
> mmaps "try to use this vma as start address" feature to not accidently get
> over that limit.
Hmmh, now that x86 has solved all the problems we should probably move 
away from using the same page table for userspace and guest too. That 
would make this "workaround" superfluous and memslots would work just 
the same on s390 as they do on x86. I've put it on my todo list.
For now, this looks good to me.

[Qemu-devel] Re: [PATCH 2/9] Add KVM support for S390x

[Carsten Otte]

Alexander Graf wrote:
> ++static int s390_cpu_initial_reset(CPUState *env)
+{
+    /* XXX */
+    fprintf(stderr, "XXX SIGP init\n");
+    return -1;
+}
That one will really  break SMP. Most of the initial reset is handled 
in-kernel for your convenience, therefore please think about using Kuli 
as inspiration on how to do this one ;-).
Other then that, the s390 specifics in this patch look really nice.

[Qemu-devel] Re: [PATCH 2/9] Add KVM support for S390x

[Alexander Graf]

On 20.10.2009, at 10:36, Carsten Otte wrote:

> Alexander Graf wrote:
>> ++static int s390_cpu_initial_reset(CPUState *env)
> +{
> +    /* XXX */
> +    fprintf(stderr, "XXX SIGP init\n");
> +    return -1;
> +}
> That one will really  break SMP. Most of the initial reset is  
> handled in-kernel for your convenience, therefore please think about  
> using Kuli as inspiration on how to do this one ;-).

Yes, SMP support is missing still. I ran into some problem  
implementing it.
Qemu by default doesn't allow -smp > 1 anyways, so I figured it's not  
too much of a problem for starters.

> Other then that, the s390 specifics in this patch look really nice.


Thanks :-)


Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Alexander Graf wrote:
> This is the resulting code. Please comment on things you like and also on the
> ones you don't :-).
I've reviewed and tested it, great work Alex :-)

> Also to actually run this code you need a patch for an ugly bug in the kernel
> module: http://alex.csgraf.de/psw.patch
Well, it was not exactly a bug. Kuli does'nt need the psw to be updated 
in kvm_run at all times. Your hotfix updates the psw in a union, even if 
the exit reason indicates that s390_sieic is not valid. The patch at the 
end of this mail moves the PSW out of the union. I think it makes most 
sense if Avi picks this patch and you adopt your series to it. This way 
the user interface of the kvm module works for both kuli and qemu.







This patch moves s390 processor status word into the base kvm_run
struct and keeps it up-to date on all userspace exits.

Signed-off-by: Carsten Otte <cotte@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c |   14 ++++++++------
 include/linux/kvm.h      |    8 ++++++--
 2 files changed, 14 insertions(+), 8 deletions(-)

Index: kvm/include/linux/kvm.h
===================================================================
--- kvm.orig/include/linux/kvm.h	2009-10-20 15:00:40.000000000 +0200
+++ kvm/include/linux/kvm.h	2009-10-20 16:51:48.000000000 +0200
@@ -7,6 +7,7 @@
  * Note: you must update KVM_API_VERSION if you change this interface.
  */
 
+#include <linux/autoconf.h>
 #include <linux/types.h>
 #include <linux/compiler.h>
 #include <linux/ioctl.h>
@@ -116,6 +117,11 @@
 	__u64 cr8;
 	__u64 apic_base;
 
+#ifdef CONFIG_S390
+	/* the processor status word for s390 */
+	__u64 psw_mask; /* psw upper half */
+	__u64 psw_addr; /* psw lower half */
+#endif
 	union {
 		/* KVM_EXIT_UNKNOWN */
 		struct {
@@ -167,8 +173,6 @@
 		/* KVM_EXIT_S390_SIEIC */
 		struct {
 			__u8 icptcode;
-			__u64 mask; /* psw upper half */
-			__u64 addr; /* psw lower half */
 			__u16 ipa;
 			__u32 ipb;
 		} s390_sieic;
Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
--- kvm.orig/arch/s390/kvm/kvm-s390.c	2009-10-20 15:01:02.000000000 +0200
+++ kvm/arch/s390/kvm/kvm-s390.c	2009-10-20 18:13:45.000000000 +0200
@@ -421,7 +421,8 @@
 	if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
 		rc = -EBUSY;
 	else
-		vcpu->arch.sie_block->gpsw = psw;
+		vcpu->run->psw_mask = psw.mask;
+		vcpu->run->psw_addr = psw.addr;
 	vcpu_put(vcpu);
 	return rc;
 }
@@ -509,9 +510,6 @@
 
 	switch (kvm_run->exit_reason) {
 	case KVM_EXIT_S390_SIEIC:
-		vcpu->arch.sie_block->gpsw.mask = kvm_run->s390_sieic.mask;
-		vcpu->arch.sie_block->gpsw.addr = kvm_run->s390_sieic.addr;
-		break;
 	case KVM_EXIT_UNKNOWN:
 	case KVM_EXIT_INTR:
 	case KVM_EXIT_S390_RESET:
@@ -520,6 +518,9 @@
 		BUG();
 	}
 
+	vcpu->arch.sie_block->gpsw.mask = kvm_run->psw_mask;
+	vcpu->arch.sie_block->gpsw.addr = kvm_run->psw_addr;
+
 	might_fault();
 
 	do {
@@ -539,8 +540,6 @@
 		/* intercept cannot be handled in-kernel, prepare kvm-run */
 		kvm_run->exit_reason         = KVM_EXIT_S390_SIEIC;
 		kvm_run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
-		kvm_run->s390_sieic.mask     = vcpu->arch.sie_block->gpsw.mask;
-		kvm_run->s390_sieic.addr     = vcpu->arch.sie_block->gpsw.addr;
 		kvm_run->s390_sieic.ipa      = vcpu->arch.sie_block->ipa;
 		kvm_run->s390_sieic.ipb      = vcpu->arch.sie_block->ipb;
 		rc = 0;
@@ -552,6 +551,9 @@
 		rc = 0;
 	}
 
+	kvm_run->psw_mask     = vcpu->arch.sie_block->gpsw.mask;
+	kvm_run->psw_addr     = vcpu->arch.sie_block->gpsw.addr;
+
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 20.10.2009, at 18:40, Carsten Otte wrote:

> Alexander Graf wrote:
>> This is the resulting code. Please comment on things you like and  
>> also on the
>> ones you don't :-).
> I've reviewed and tested it, great work Alex :-)

Thanks :-).

>> Also to actually run this code you need a patch for an ugly bug in  
>> the kernel
>> module: http://alex.csgraf.de/psw.patch
> Well, it was not exactly a bug. Kuli does'nt need the psw to be  
> updated in kvm_run at all times. Your hotfix updates the psw in a  
> union, even if the exit reason indicates that s390_sieic is not  
> valid. The patch at the end of this mail moves the PSW out of the  
> union. I think it makes most sense if Avi picks this patch and you  
> adopt your series to it. This way the user interface of the kvm  
> module works for both kuli and qemu.

Alright, expect v2 today!

On a sidenode, please post the patch to kvm-devel too, so Marcelo has  
the chance to pick it up.


> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> Signed-off-by: Carsten Otte <cotte@de.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c |   14 ++++++++------
> include/linux/kvm.h      |    8 ++++++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> Index: kvm/include/linux/kvm.h
> ===================================================================
> --- kvm.orig/include/linux/kvm.h	2009-10-20 15:00:40.000000000 +0200
> +++ kvm/include/linux/kvm.h	2009-10-20 16:51:48.000000000 +0200
> @@ -7,6 +7,7 @@
> * Note: you must update KVM_API_VERSION if you change this interface.
> */
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>
> @@ -116,6 +117,11 @@
> 	__u64 cr8;
> 	__u64 apic_base;
> +#ifdef CONFIG_S390
> +	/* the processor status word for s390 */
> +	__u64 psw_mask; /* psw upper half */
> +	__u64 psw_addr; /* psw lower half */
> +#endif
> 	union {
> 		/* KVM_EXIT_UNKNOWN */
> 		struct {
> @@ -167,8 +173,6 @@
> 		/* KVM_EXIT_S390_SIEIC */
> 		struct {
> 			__u8 icptcode;
> -			__u64 mask; /* psw upper half */
> -			__u64 addr; /* psw lower half */
> 			__u16 ipa;
> 			__u32 ipb;
> 		} s390_sieic;
> Index: kvm/arch/s390/kvm/kvm-s390.c
> ===================================================================
> --- kvm.orig/arch/s390/kvm/kvm-s390.c	2009-10-20 15:01:02.000000000  
> +0200
> +++ kvm/arch/s390/kvm/kvm-s390.c	2009-10-20 18:13:45.000000000 +0200
> @@ -421,7 +421,8 @@
> 	if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
> 		rc = -EBUSY;
> 	else
> -		vcpu->arch.sie_block->gpsw = psw;
> +		vcpu->run->psw_mask = psw.mask;
> +		vcpu->run->psw_addr = psw.addr;
> 	vcpu_put(vcpu);
> 	return rc;
> }
> @@ -509,9 +510,6 @@
> 	switch (kvm_run->exit_reason) {
> 	case KVM_EXIT_S390_SIEIC:
> -		vcpu->arch.sie_block->gpsw.mask = kvm_run->s390_sieic.mask;
> -		vcpu->arch.sie_block->gpsw.addr = kvm_run->s390_sieic.addr;
> -		break;
> 	case KVM_EXIT_UNKNOWN:
> 	case KVM_EXIT_INTR:
> 	case KVM_EXIT_S390_RESET:
> @@ -520,6 +518,9 @@
> 		BUG();
> 	}

Why leave the switch? Do you want the BUG() that badly?

Most of the time that's just wasted cycles, right?

> +	vcpu->arch.sie_block->gpsw.mask = kvm_run->psw_mask;
> +	vcpu->arch.sie_block->gpsw.addr = kvm_run->psw_addr;
> +
> 	might_fault();
> 	do {
> @@ -539,8 +540,6 @@
> 		/* intercept cannot be handled in-kernel, prepare kvm-run */
> 		kvm_run->exit_reason         = KVM_EXIT_S390_SIEIC;
> 		kvm_run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
> -		kvm_run->s390_sieic.mask     = vcpu->arch.sie_block->gpsw.mask;
> -		kvm_run->s390_sieic.addr     = vcpu->arch.sie_block->gpsw.addr;
> 		kvm_run->s390_sieic.ipa      = vcpu->arch.sie_block->ipa;
> 		kvm_run->s390_sieic.ipb      = vcpu->arch.sie_block->ipb;
> 		rc = 0;
> @@ -552,6 +551,9 @@
> 		rc = 0;
> 	}
> +	kvm_run->psw_mask     = vcpu->arch.sie_block->gpsw.mask;
> +	kvm_run->psw_addr     = vcpu->arch.sie_block->gpsw.addr;
> +

I think you should not indent the equal signs here. They're not  
aligned to the ones above anymore anyways.

Apart from that, great patch!
I'll put all the pieces together today, so expect to be running Qemu  
backed virtualization on your mainframe :-).

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 20.10.2009, at 18:40, Carsten Otte wrote:

> Alexander Graf wrote:
>> This is the resulting code. Please comment on things you like and  
>> also on the
>> ones you don't :-).
> I've reviewed and tested it, great work Alex :-)
>
>> Also to actually run this code you need a patch for an ugly bug in  
>> the kernel
>> module: http://alex.csgraf.de/psw.patch
> Well, it was not exactly a bug. Kuli does'nt need the psw to be  
> updated in kvm_run at all times. Your hotfix updates the psw in a  
> union, even if the exit reason indicates that s390_sieic is not  
> valid. The patch at the end of this mail moves the PSW out of the  
> union. I think it makes most sense if Avi picks this patch and you  
> adopt your series to it. This way the user interface of the kvm  
> module works for both kuli and qemu.
>
>
>
>
>
>
>
> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> Signed-off-by: Carsten Otte <cotte@de.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c |   14 ++++++++------
> include/linux/kvm.h      |    8 ++++++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> Index: kvm/include/linux/kvm.h
> ===================================================================
> --- kvm.orig/include/linux/kvm.h	2009-10-20 15:00:40.000000000 +0200
> +++ kvm/include/linux/kvm.h	2009-10-20 16:51:48.000000000 +0200
> @@ -7,6 +7,7 @@
> * Note: you must update KVM_API_VERSION if you change this interface.
> */
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>
> @@ -116,6 +117,11 @@
> 	__u64 cr8;
> 	__u64 apic_base;
> +#ifdef CONFIG_S390
> +	/* the processor status word for s390 */
> +	__u64 psw_mask; /* psw upper half */
> +	__u64 psw_addr; /* psw lower half */
> +#endif

While at it, I think we should either go all or nothing here.

Either we take the psw in on x86 too or we #ifdef out the x86 specific  
stuff on s390.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 20.10.2009, at 18:40, Carsten Otte wrote:

> Alexander Graf wrote:
>> This is the resulting code. Please comment on things you like and  
>> also on the
>> ones you don't :-).
> I've reviewed and tested it, great work Alex :-)
>
>> Also to actually run this code you need a patch for an ugly bug in  
>> the kernel
>> module: http://alex.csgraf.de/psw.patch
> Well, it was not exactly a bug. Kuli does'nt need the psw to be  
> updated in kvm_run at all times. Your hotfix updates the psw in a  
> union, even if the exit reason indicates that s390_sieic is not  
> valid. The patch at the end of this mail moves the PSW out of the  
> union. I think it makes most sense if Avi picks this patch and you  
> adopt your series to it. This way the user interface of the kvm  
> module works for both kuli and qemu.
>
>
>
>
>
>
>
> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> Signed-off-by: Carsten Otte <cotte@de.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c |   14 ++++++++------
> include/linux/kvm.h      |    8 ++++++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> Index: kvm/include/linux/kvm.h
> ===================================================================
> --- kvm.orig/include/linux/kvm.h	2009-10-20 15:00:40.000000000 +0200
> +++ kvm/include/linux/kvm.h	2009-10-20 16:51:48.000000000 +0200
> @@ -7,6 +7,7 @@
> * Note: you must update KVM_API_VERSION if you change this interface.
> */
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>
> @@ -116,6 +117,11 @@
> 	__u64 cr8;
> 	__u64 apic_base;
> +#ifdef CONFIG_S390
> +	/* the processor status word for s390 */
> +	__u64 psw_mask; /* psw upper half */
> +	__u64 psw_addr; /* psw lower half */
> +#endif
> 	union {
> 		/* KVM_EXIT_UNKNOWN */
> 		struct {
> @@ -167,8 +173,6 @@
> 		/* KVM_EXIT_S390_SIEIC */
> 		struct {
> 			__u8 icptcode;
> -			__u64 mask; /* psw upper half */
> -			__u64 addr; /* psw lower half */
> 			__u16 ipa;
> 			__u32 ipb;
> 		} s390_sieic;
> Index: kvm/arch/s390/kvm/kvm-s390.c
> ===================================================================
> --- kvm.orig/arch/s390/kvm/kvm-s390.c	2009-10-20 15:01:02.000000000  
> +0200
> +++ kvm/arch/s390/kvm/kvm-s390.c	2009-10-20 18:13:45.000000000 +0200
> @@ -421,7 +421,8 @@
> 	if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
> 		rc = -EBUSY;
> 	else
> -		vcpu->arch.sie_block->gpsw = psw;
> +		vcpu->run->psw_mask = psw.mask;
> +		vcpu->run->psw_addr = psw.addr;

Missing braces.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Alexander Graf wrote:
> Either we take the psw in on x86 too or we #ifdef out the x86 specific 
> stuff on s390.
Good idea, I'll #ifndef CONFIG_S390 the x86 specifics while we're at it.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 21.10.2009, at 10:18, Carsten Otte wrote:

> Alexander Graf wrote:
>> Either we take the psw in on x86 too or we #ifdef out the x86  
>> specific stuff on s390.
> Good idea, I'll #ifndef CONFIG_S390 the x86 specifics while we're at  
> it.

Maybe better #if defined(TARGET_X86) || defined(TARGET_IA64). We don't  
really need them for ppc either :-).

Alex

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/20/2009 06:40 PM, Carsten Otte wrote:
>
> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>

Not needed.

> @@ -116,6 +117,11 @@
>     __u64 cr8;
>     __u64 apic_base;
>
> +#ifdef CONFIG_S390
> +    /* the processor status word for s390 */
> +    __u64 psw_mask; /* psw upper half */
> +    __u64 psw_addr; /* psw lower half */
> +#endif

Doesn't this break backward compatibility by changing the structure?

Best to put it after the union (and as a copy, so userspace that expects 
the previous location still works).  If you're reading it from the 
kernel, also need a way to tell the kernel which copy to read from.

Also advertise with a KVM_CAP.

Additionally, CONFIG_ in public headers are frowned upon as 
non-portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and 
depend on that.

> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20 15:01:02.000000000 
> +0200
> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000 +0200
> @@ -421,7 +421,8 @@
>     if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
>         rc = -EBUSY;
>     else
> -        vcpu->arch.sie_block->gpsw = psw;
> +        vcpu->run->psw_mask = psw.mask;
> +        vcpu->run->psw_addr = psw.addr;

It's traditional to add braces around multi-line else blocks.

I'd also appreciate an explanation of what this is all about.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/20/2009 06:40 PM, Carsten Otte wrote:
>
> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>

Not needed.

> @@ -116,6 +117,11 @@
>     __u64 cr8;
>     __u64 apic_base;
>
> +#ifdef CONFIG_S390
> +    /* the processor status word for s390 */
> +    __u64 psw_mask; /* psw upper half */
> +    __u64 psw_addr; /* psw lower half */
> +#endif

Doesn't this break backward compatibility by changing the structure?

Best to put it after the union (and as a copy, so userspace that expects 
the previous location still works).  If you're reading it from the 
kernel, also need a way to tell the kernel which copy to read from.

Also advertise with a KVM_CAP.

Additionally, CONFIG_ in public headers are frowned upon as 
non-portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and 
depend on that.

> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20 15:01:02.000000000 
> +0200
> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000 +0200
> @@ -421,7 +421,8 @@
>     if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
>         rc = -EBUSY;
>     else
> -        vcpu->arch.sie_block->gpsw = psw;
> +        vcpu->run->psw_mask = psw.mask;
> +        vcpu->run->psw_addr = psw.addr;

It's traditional to add braces around multi-line else blocks.

I'd also appreciate an explanation of what this is all about.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:08, Avi Kivity wrote:

> On 10/20/2009 06:40 PM, Carsten Otte wrote:
>>
>> This patch moves s390 processor status word into the base kvm_run
>> struct and keeps it up-to date on all userspace exits.
>>
>> +#include <linux/autoconf.h>
>> #include <linux/types.h>
>> #include <linux/compiler.h>
>> #include <linux/ioctl.h>
>
> Not needed.
>
>> @@ -116,6 +117,11 @@
>>    __u64 cr8;
>>    __u64 apic_base;
>>
>> +#ifdef CONFIG_S390
>> +    /* the processor status word for s390 */
>> +    __u64 psw_mask; /* psw upper half */
>> +    __u64 psw_addr; /* psw lower half */
>> +#endif
>
> Doesn't this break backward compatibility by changing the structure?
>
> Best to put it after the union (and as a copy, so userspace that  
> expects the previous location still works).  If you're reading it  
> from the kernel, also need a way to tell the kernel which copy to  
> read from.
>
> Also advertise with a KVM_CAP.

I don't think we need to go through the hassle here. There is  
effectively no user of that code for now and the ABI is considered  
unstable.

> Additionally, CONFIG_ in public headers are frowned upon as non- 
> portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and  
> depend on that.
>
>> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20  
>> 15:01:02.000000000 +0200
>> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000  
>> +0200
>> @@ -421,7 +421,8 @@
>>    if (atomic_read(&vcpu->arch.sie_block->cpuflags) &  
>> CPUSTAT_RUNNING)
>>        rc = -EBUSY;
>>    else
>> -        vcpu->arch.sie_block->gpsw = psw;
>> +        vcpu->run->psw_mask = psw.mask;
>> +        vcpu->run->psw_addr = psw.addr;
>
> It's traditional to add braces around multi-line else blocks.
>
> I'd also appreciate an explanation of what this is all about.

Explanation in the code or explanation in an email reply?

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:08, Avi Kivity wrote:

> On 10/20/2009 06:40 PM, Carsten Otte wrote:
>>
>> This patch moves s390 processor status word into the base kvm_run
>> struct and keeps it up-to date on all userspace exits.
>>
>> +#include <linux/autoconf.h>
>> #include <linux/types.h>
>> #include <linux/compiler.h>
>> #include <linux/ioctl.h>
>
> Not needed.
>
>> @@ -116,6 +117,11 @@
>>    __u64 cr8;
>>    __u64 apic_base;
>>
>> +#ifdef CONFIG_S390
>> +    /* the processor status word for s390 */
>> +    __u64 psw_mask; /* psw upper half */
>> +    __u64 psw_addr; /* psw lower half */
>> +#endif
>
> Doesn't this break backward compatibility by changing the structure?
>
> Best to put it after the union (and as a copy, so userspace that  
> expects the previous location still works).  If you're reading it  
> from the kernel, also need a way to tell the kernel which copy to  
> read from.
>
> Also advertise with a KVM_CAP.

I don't think we need to go through the hassle here. There is  
effectively no user of that code for now and the ABI is considered  
unstable.

> Additionally, CONFIG_ in public headers are frowned upon as non- 
> portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and  
> depend on that.
>
>> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20  
>> 15:01:02.000000000 +0200
>> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000  
>> +0200
>> @@ -421,7 +421,8 @@
>>    if (atomic_read(&vcpu->arch.sie_block->cpuflags) &  
>> CPUSTAT_RUNNING)
>>        rc = -EBUSY;
>>    else
>> -        vcpu->arch.sie_block->gpsw = psw;
>> +        vcpu->run->psw_mask = psw.mask;
>> +        vcpu->run->psw_addr = psw.addr;
>
> It's traditional to add braces around multi-line else blocks.
>
> I'd also appreciate an explanation of what this is all about.

Explanation in the code or explanation in an email reply?

Alex

Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
>> @@ -116,6 +117,11 @@
>>     __u64 cr8;
>>     __u64 apic_base;
>>
>> +#ifdef CONFIG_S390
>> +    /* the processor status word for s390 */
>> +    __u64 psw_mask; /* psw upper half */
>> +    __u64 psw_addr; /* psw lower half */
>> +#endif
> 
> Doesn't this break backward compatibility by changing the structure?
Yes, but with a zero user base I think it's okay. I'd update our 
userspace. Once we pull CONFIG_EXPERIMENTAL we keep the API stable.

> Additionally, CONFIG_ in public headers are frowned upon as 
> non-portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and 
> depend on that.
Yea, that's better.

>> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20 15:01:02.000000000 
>> +0200
>> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000 +0200
>> @@ -421,7 +421,8 @@
>>     if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
>>         rc = -EBUSY;
>>     else
>> -        vcpu->arch.sie_block->gpsw = psw;
>> +        vcpu->run->psw_mask = psw.mask;
>> +        vcpu->run->psw_addr = psw.addr;
> 
> It's traditional to add braces around multi-line else blocks.
This is a plain bug, will fix.


> I'd also appreciate an explanation of what this is all about.
The processor status word does contain various bits about the CPU's 
state, such as interrupt mask bits, current address space, and the 
current instruction address. The status is kept in the in-kernel sie 
control block data structure and has so far only been mirrored into 
kvm_run during exit_reason == s390_sieic exits because user space needs 
to work on it. It was never part of get_regs/set_regs and friends as 
performance optimization: it's needed on almost every exit, having it in 
kvm_run saves doing syscalls.
The gdb stub requires an up-to-date copy at every exit, and therefore 
the patch moves it out of the union and updates it at all userland exits.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
>> @@ -116,6 +117,11 @@
>>     __u64 cr8;
>>     __u64 apic_base;
>>
>> +#ifdef CONFIG_S390
>> +    /* the processor status word for s390 */
>> +    __u64 psw_mask; /* psw upper half */
>> +    __u64 psw_addr; /* psw lower half */
>> +#endif
> 
> Doesn't this break backward compatibility by changing the structure?
Yes, but with a zero user base I think it's okay. I'd update our 
userspace. Once we pull CONFIG_EXPERIMENTAL we keep the API stable.

> Additionally, CONFIG_ in public headers are frowned upon as 
> non-portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and 
> depend on that.
Yea, that's better.

>> --- kvm.orig/arch/s390/kvm/kvm-s390.c    2009-10-20 15:01:02.000000000 
>> +0200
>> +++ kvm/arch/s390/kvm/kvm-s390.c    2009-10-20 18:13:45.000000000 +0200
>> @@ -421,7 +421,8 @@
>>     if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
>>         rc = -EBUSY;
>>     else
>> -        vcpu->arch.sie_block->gpsw = psw;
>> +        vcpu->run->psw_mask = psw.mask;
>> +        vcpu->run->psw_addr = psw.addr;
> 
> It's traditional to add braces around multi-line else blocks.
This is a plain bug, will fix.


> I'd also appreciate an explanation of what this is all about.
The processor status word does contain various bits about the CPU's 
state, such as interrupt mask bits, current address space, and the 
current instruction address. The status is kept in the in-kernel sie 
control block data structure and has so far only been mirrored into 
kvm_run during exit_reason == s390_sieic exits because user space needs 
to work on it. It was never part of get_regs/set_regs and friends as 
performance optimization: it's needed on almost every exit, having it in 
kvm_run saves doing syscalls.
The gdb stub requires an up-to-date copy at every exit, and therefore 
the patch moves it out of the union and updates it at all userland exits.

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:11 AM, Alexander Graf wrote:
>> Doesn't this break backward compatibility by changing the structure?
>>
>> Best to put it after the union (and as a copy, so userspace that 
>> expects the previous location still works).  If you're reading it 
>> from the kernel, also need a way to tell the kernel which copy to 
>> read from.
>>
>> Also advertise with a KVM_CAP.
>
>
> I don't think we need to go through the hassle here. There is 
> effectively no user of that code for now and the ABI is considered 
> unstable.

At the very least we need a KVM_CAP so qemu knows to fail on older kernels.

>>
>> I'd also appreciate an explanation of what this is all about.
>
> Explanation in the code or explanation in an email reply?


email.  I assume s390 hackers would understand why the psw needs to be 
exposed to qemu on every exit.  This is mostly for my personal interest.


-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:11 AM, Alexander Graf wrote:
>> Doesn't this break backward compatibility by changing the structure?
>>
>> Best to put it after the union (and as a copy, so userspace that 
>> expects the previous location still works).  If you're reading it 
>> from the kernel, also need a way to tell the kernel which copy to 
>> read from.
>>
>> Also advertise with a KVM_CAP.
>
>
> I don't think we need to go through the hassle here. There is 
> effectively no user of that code for now and the ABI is considered 
> unstable.

At the very least we need a KVM_CAP so qemu knows to fail on older kernels.

>>
>> I'd also appreciate an explanation of what this is all about.
>
> Explanation in the code or explanation in an email reply?


email.  I assume s390 hackers would understand why the psw needs to be 
exposed to qemu on every exit.  This is mostly for my personal interest.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:53, Avi Kivity wrote:

> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>> Doesn't this break backward compatibility by changing the structure?
>>>
>>> Best to put it after the union (and as a copy, so userspace that  
>>> expects the previous location still works).  If you're reading it  
>>> from the kernel, also need a way to tell the kernel which copy to  
>>> read from.
>>>
>>> Also advertise with a KVM_CAP.
>>
>>
>> I don't think we need to go through the hassle here. There is  
>> effectively no user of that code for now and the ABI is considered  
>> unstable.
>
> At the very least we need a KVM_CAP so qemu knows to fail on older  
> kernels.

Hm. Oh well :-). It can't hurt to have yet another CAP, right?

>>> I'd also appreciate an explanation of what this is all about.
>>
>> Explanation in the code or explanation in an email reply?
>
>
> email.  I assume s390 hackers would understand why the psw needs to  
> be exposed to qemu on every exit.  This is mostly for my personal  
> interest.

PSW = (eflags << 32) | pc;

:-)

Before that patch it was only synced with the "vmcb" on special  
userspace handled intercepts, now it's synced on every exit to  
userspace.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:53, Avi Kivity wrote:

> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>> Doesn't this break backward compatibility by changing the structure?
>>>
>>> Best to put it after the union (and as a copy, so userspace that  
>>> expects the previous location still works).  If you're reading it  
>>> from the kernel, also need a way to tell the kernel which copy to  
>>> read from.
>>>
>>> Also advertise with a KVM_CAP.
>>
>>
>> I don't think we need to go through the hassle here. There is  
>> effectively no user of that code for now and the ABI is considered  
>> unstable.
>
> At the very least we need a KVM_CAP so qemu knows to fail on older  
> kernels.

Hm. Oh well :-). It can't hurt to have yet another CAP, right?

>>> I'd also appreciate an explanation of what this is all about.
>>
>> Explanation in the code or explanation in an email reply?
>
>
> email.  I assume s390 hackers would understand why the psw needs to  
> be exposed to qemu on every exit.  This is mostly for my personal  
> interest.

PSW = (eflags << 32) | pc;

:-)

Before that patch it was only synced with the "vmcb" on special  
userspace handled intercepts, now it's synced on every exit to  
userspace.

Alex

Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:55, Alexander Graf wrote:

> PSW = (eflags << 32) | pc;

Eh - 64 of course. The PSW is 128 bits.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 11:55, Alexander Graf wrote:

> PSW = (eflags << 32) | pc;

Eh - 64 of course. The PSW is 128 bits.

Alex

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:18 AM, Carsten Otte wrote:
>
>> I'd also appreciate an explanation of what this is all about.
> The processor status word does contain various bits about the CPU's 
> state, such as interrupt mask bits, current address space, and the 
> current instruction address. The status is kept in the in-kernel sie 
> control block data structure and has so far only been mirrored into 
> kvm_run during exit_reason == s390_sieic exits because user space 
> needs to work on it. It was never part of get_regs/set_regs and 
> friends as performance optimization: it's needed on almost every exit, 
> having it in kvm_run saves doing syscalls.
> The gdb stub requires an up-to-date copy at every exit, and therefore 
> the patch moves it out of the union and updates it at all userland exits.

gdb is hardly performance critical.  Is that the only reason for the change?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:18 AM, Carsten Otte wrote:
>
>> I'd also appreciate an explanation of what this is all about.
> The processor status word does contain various bits about the CPU's 
> state, such as interrupt mask bits, current address space, and the 
> current instruction address. The status is kept in the in-kernel sie 
> control block data structure and has so far only been mirrored into 
> kvm_run during exit_reason == s390_sieic exits because user space 
> needs to work on it. It was never part of get_regs/set_regs and 
> friends as performance optimization: it's needed on almost every exit, 
> having it in kvm_run saves doing syscalls.
> The gdb stub requires an up-to-date copy at every exit, and therefore 
> the patch moves it out of the union and updates it at all userland exits.

gdb is hardly performance critical.  Is that the only reason for the change?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:55 AM, Alexander Graf wrote:
>
> On 22.10.2009, at 11:53, Avi Kivity wrote:
>
>> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>>> Doesn't this break backward compatibility by changing the structure?
>>>>
>>>> Best to put it after the union (and as a copy, so userspace that 
>>>> expects the previous location still works).  If you're reading it 
>>>> from the kernel, also need a way to tell the kernel which copy to 
>>>> read from.
>>>>
>>>> Also advertise with a KVM_CAP.
>>>
>>>
>>> I don't think we need to go through the hassle here. There is 
>>> effectively no user of that code for now and the ABI is considered 
>>> unstable.
>>
>> At the very least we need a KVM_CAP so qemu knows to fail on older 
>> kernels.
>
> Hm. Oh well :-). It can't hurt to have yet another CAP, right?
>
>>>> I'd also appreciate an explanation of what this is all about.
>>>
>>> Explanation in the code or explanation in an email reply?
>>
>>
>> email.  I assume s390 hackers would understand why the psw needs to 
>> be exposed to qemu on every exit.  This is mostly for my personal 
>> interest.
>
> PSW = (eflags << 32) | pc;
>
> :-)
>
> Before that patch it was only synced with the "vmcb" on special 
> userspace handled intercepts, now it's synced on every exit to userspace.

Right, but why?  x86 qemu doesn't care about either pc or eflags (with 
in-kernel irqchip, which s390 essentially is).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 11:55 AM, Alexander Graf wrote:
>
> On 22.10.2009, at 11:53, Avi Kivity wrote:
>
>> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>>> Doesn't this break backward compatibility by changing the structure?
>>>>
>>>> Best to put it after the union (and as a copy, so userspace that 
>>>> expects the previous location still works).  If you're reading it 
>>>> from the kernel, also need a way to tell the kernel which copy to 
>>>> read from.
>>>>
>>>> Also advertise with a KVM_CAP.
>>>
>>>
>>> I don't think we need to go through the hassle here. There is 
>>> effectively no user of that code for now and the ABI is considered 
>>> unstable.
>>
>> At the very least we need a KVM_CAP so qemu knows to fail on older 
>> kernels.
>
> Hm. Oh well :-). It can't hurt to have yet another CAP, right?
>
>>>> I'd also appreciate an explanation of what this is all about.
>>>
>>> Explanation in the code or explanation in an email reply?
>>
>>
>> email.  I assume s390 hackers would understand why the psw needs to 
>> be exposed to qemu on every exit.  This is mostly for my personal 
>> interest.
>
> PSW = (eflags << 32) | pc;
>
> :-)
>
> Before that patch it was only synced with the "vmcb" on special 
> userspace handled intercepts, now it's synced on every exit to userspace.

Right, but why?  x86 qemu doesn't care about either pc or eflags (with 
in-kernel irqchip, which s390 essentially is).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 12:03, Avi Kivity wrote:

> On 10/22/2009 11:55 AM, Alexander Graf wrote:
>>
>> On 22.10.2009, at 11:53, Avi Kivity wrote:
>>
>>> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>>>> Doesn't this break backward compatibility by changing the  
>>>>> structure?
>>>>>
>>>>> Best to put it after the union (and as a copy, so userspace that  
>>>>> expects the previous location still works).  If you're reading  
>>>>> it from the kernel, also need a way to tell the kernel which  
>>>>> copy to read from.
>>>>>
>>>>> Also advertise with a KVM_CAP.
>>>>
>>>>
>>>> I don't think we need to go through the hassle here. There is  
>>>> effectively no user of that code for now and the ABI is  
>>>> considered unstable.
>>>
>>> At the very least we need a KVM_CAP so qemu knows to fail on older  
>>> kernels.
>>
>> Hm. Oh well :-). It can't hurt to have yet another CAP, right?
>>
>>>>> I'd also appreciate an explanation of what this is all about.
>>>>
>>>> Explanation in the code or explanation in an email reply?
>>>
>>>
>>> email.  I assume s390 hackers would understand why the psw needs  
>>> to be exposed to qemu on every exit.  This is mostly for my  
>>> personal interest.
>>
>> PSW = (eflags << 32) | pc;
>>
>> :-)
>>
>> Before that patch it was only synced with the "vmcb" on special  
>> userspace handled intercepts, now it's synced on every exit to  
>> userspace.
>
> Right, but why?  x86 qemu doesn't care about either pc or eflags  
> (with in-kernel irqchip, which s390 essentially is).

The old code would only sync the psw for us on "intercepts that the  
kernel knows require userspace interaction". We'd "x /i $pc" somewhere  
along the way and would get an old psw, but GET_REGS would give us  
recent register states, not including the psw. That's a total nightmare.

To solve the psw nightmare there were basically 2 roads possible to  
take here:

1) Sync the PSW in the GET_REGS ioctl
2) Sync the PSW in kvm_run on all exits to userspace

Otherwise we can't debug guests.

The same goes for setting the PSW. Setting the PSW was only  
implemented when kvm_run->exit_reason == "the kernel thinks userspace  
needs to do something". So If you wanted to change the pc along the  
way, you couldn't.

Furthermore, there was a nasty bug in this whole optimization that  
lead me to think it's better to just get rid of the whole "kernel  
tries to be clever" implementation and just sync the psw at all times.  
Most exits to userspace are of the "userspace needs to do something"  
type anyways, so we're not really losing anything here. Implementing  
2) was just really easy.

So I guess Carsten thought the same way and improved my original patch.


I hope that clarifies things.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 22.10.2009, at 12:03, Avi Kivity wrote:

> On 10/22/2009 11:55 AM, Alexander Graf wrote:
>>
>> On 22.10.2009, at 11:53, Avi Kivity wrote:
>>
>>> On 10/22/2009 11:11 AM, Alexander Graf wrote:
>>>>> Doesn't this break backward compatibility by changing the  
>>>>> structure?
>>>>>
>>>>> Best to put it after the union (and as a copy, so userspace that  
>>>>> expects the previous location still works).  If you're reading  
>>>>> it from the kernel, also need a way to tell the kernel which  
>>>>> copy to read from.
>>>>>
>>>>> Also advertise with a KVM_CAP.
>>>>
>>>>
>>>> I don't think we need to go through the hassle here. There is  
>>>> effectively no user of that code for now and the ABI is  
>>>> considered unstable.
>>>
>>> At the very least we need a KVM_CAP so qemu knows to fail on older  
>>> kernels.
>>
>> Hm. Oh well :-). It can't hurt to have yet another CAP, right?
>>
>>>>> I'd also appreciate an explanation of what this is all about.
>>>>
>>>> Explanation in the code or explanation in an email reply?
>>>
>>>
>>> email.  I assume s390 hackers would understand why the psw needs  
>>> to be exposed to qemu on every exit.  This is mostly for my  
>>> personal interest.
>>
>> PSW = (eflags << 32) | pc;
>>
>> :-)
>>
>> Before that patch it was only synced with the "vmcb" on special  
>> userspace handled intercepts, now it's synced on every exit to  
>> userspace.
>
> Right, but why?  x86 qemu doesn't care about either pc or eflags  
> (with in-kernel irqchip, which s390 essentially is).

The old code would only sync the psw for us on "intercepts that the  
kernel knows require userspace interaction". We'd "x /i $pc" somewhere  
along the way and would get an old psw, but GET_REGS would give us  
recent register states, not including the psw. That's a total nightmare.

To solve the psw nightmare there were basically 2 roads possible to  
take here:

1) Sync the PSW in the GET_REGS ioctl
2) Sync the PSW in kvm_run on all exits to userspace

Otherwise we can't debug guests.

The same goes for setting the PSW. Setting the PSW was only  
implemented when kvm_run->exit_reason == "the kernel thinks userspace  
needs to do something". So If you wanted to change the pc along the  
way, you couldn't.

Furthermore, there was a nasty bug in this whole optimization that  
lead me to think it's better to just get rid of the whole "kernel  
tries to be clever" implementation and just sync the psw at all times.  
Most exits to userspace are of the "userspace needs to do something"  
type anyways, so we're not really losing anything here. Implementing  
2) was just really easy.

So I guess Carsten thought the same way and improved my original patch.


I hope that clarifies things.

Alex

Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> gdb is hardly performance critical.  Is that the only reason for the 
> change?
Right, gdb is not performance critical. gdb is the reason for moving it 
out of the union, performance is the reason for having it in kvm_run at all.
There's only more reason to it: with a little tweaking in kuli, we'll be 
able to get rid of KVM_S390_SET_INITIAL_PSW ioctl with this change. That 
removes one oddity that makes us different from other platforms.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> gdb is hardly performance critical.  Is that the only reason for the 
> change?
Right, gdb is not performance critical. gdb is the reason for moving it 
out of the union, performance is the reason for having it in kvm_run at all.
There's only more reason to it: with a little tweaking in kuli, we'll be 
able to get rid of KVM_S390_SET_INITIAL_PSW ioctl with this change. That 
removes one oddity that makes us different from other platforms.

Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> Right, but why?  x86 qemu doesn't care about either pc or eflags (with 
> in-kernel irqchip, which s390 essentially is).
For different reasons. Most prominent for setting the condition code,
which is a sideband result of most instructions that indicates whether or
not the instruction actually worked - similar to the exception model in
high level programming languages.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> Right, but why?  x86 qemu doesn't care about either pc or eflags (with 
> in-kernel irqchip, which s390 essentially is).
For different reasons. Most prominent for setting the condition code,
which is a sideband result of most instructions that indicates whether or
not the instruction actually worked - similar to the exception model in
high level programming languages.

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:22 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> Right, but why?  x86 qemu doesn't care about either pc or eflags 
>> (with in-kernel irqchip, which s390 essentially is).
> For different reasons. Most prominent for setting the condition code,
> which is a sideband result of most instructions that indicates whether or
> not the instruction actually worked - similar to the exception model in
> high level programming languages.

Ok.  Thanks for the explanation.

On x86 we avoid emulating instructions in userspace.  Instead the kernel 
requests userspace to do something (triggered by the instruction), and 
the kernel does anything which might be implied by the instruction (like 
copying the result into a register, or updating pc).

An example is port I/O.  instead of userspace reading %edx to query the 
port number and setting %eax to indicate the result, userspace reads a 
port number struct field and writes an I/O result struct field.  Only 
the kernel accesses registers.

I don't know whether that model makes sense or not for s390, but please 
consider it.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:22 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> Right, but why?  x86 qemu doesn't care about either pc or eflags 
>> (with in-kernel irqchip, which s390 essentially is).
> For different reasons. Most prominent for setting the condition code,
> which is a sideband result of most instructions that indicates whether or
> not the instruction actually worked - similar to the exception model in
> high level programming languages.

Ok.  Thanks for the explanation.

On x86 we avoid emulating instructions in userspace.  Instead the kernel 
requests userspace to do something (triggered by the instruction), and 
the kernel does anything which might be implied by the instruction (like 
copying the result into a register, or updating pc).

An example is port I/O.  instead of userspace reading %edx to query the 
port number and setting %eax to indicate the result, userspace reads a 
port number struct field and writes an I/O result struct field.  Only 
the kernel accesses registers.

I don't know whether that model makes sense or not for s390, but please 
consider it.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:20 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> gdb is hardly performance critical.  Is that the only reason for the 
>> change?
> Right, gdb is not performance critical. gdb is the reason for moving 
> it out of the union, performance is the reason for having it in 
> kvm_run at all.
> There's only more reason to it: with a little tweaking in kuli, we'll 
> be able to get rid of KVM_S390_SET_INITIAL_PSW ioctl with this change. 
> That removes one oddity that makes us different from other platforms.

In fact qemu/x86 does a KVM_SET_SREGS (which updates pc) on reset.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:20 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> gdb is hardly performance critical.  Is that the only reason for the 
>> change?
> Right, gdb is not performance critical. gdb is the reason for moving 
> it out of the union, performance is the reason for having it in 
> kvm_run at all.
> There's only more reason to it: with a little tweaking in kuli, we'll 
> be able to get rid of KVM_S390_SET_INITIAL_PSW ioctl with this change. 
> That removes one oddity that makes us different from other platforms.

In fact qemu/x86 does a KVM_SET_SREGS (which updates pc) on reset.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> On x86 we avoid emulating instructions in userspace.  Instead the kernel 
> requests userspace to do something (triggered by the instruction), and 
> the kernel does anything which might be implied by the instruction (like 
> copying the result into a register, or updating pc).
> 
> An example is port I/O.  instead of userspace reading %edx to query the 
> port number and setting %eax to indicate the result, userspace reads a 
> port number struct field and writes an I/O result struct field.  Only 
> the kernel accesses registers.
> 
> I don't know whether that model makes sense or not for s390, but please 
> consider it.
We do the same for many instructions (arch/s390/kvm/instruction.c). User 
exits
are only performed for isntructions that cannot be handled in kernel. 
Also, we do exit with requests to userspace, see the s390_reset exit 
reason. I think in this regard, our implementation is very similar to 
x86. Btw: this is something I did copycat from your implementation on 
integration into kvm. The original zlive code did handle all 
instructions in userland.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> On x86 we avoid emulating instructions in userspace.  Instead the kernel 
> requests userspace to do something (triggered by the instruction), and 
> the kernel does anything which might be implied by the instruction (like 
> copying the result into a register, or updating pc).
> 
> An example is port I/O.  instead of userspace reading %edx to query the 
> port number and setting %eax to indicate the result, userspace reads a 
> port number struct field and writes an I/O result struct field.  Only 
> the kernel accesses registers.
> 
> I don't know whether that model makes sense or not for s390, but please 
> consider it.
We do the same for many instructions (arch/s390/kvm/instruction.c). User 
exits
are only performed for isntructions that cannot be handled in kernel. 
Also, we do exit with requests to userspace, see the s390_reset exit 
reason. I think in this regard, our implementation is very similar to 
x86. Btw: this is something I did copycat from your implementation on 
integration into kvm. The original zlive code did handle all 
instructions in userland.

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:43 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> On x86 we avoid emulating instructions in userspace.  Instead the 
>> kernel requests userspace to do something (triggered by the 
>> instruction), and the kernel does anything which might be implied by 
>> the instruction (like copying the result into a register, or updating 
>> pc).
>>
>> An example is port I/O.  instead of userspace reading %edx to query 
>> the port number and setting %eax to indicate the result, userspace 
>> reads a port number struct field and writes an I/O result struct 
>> field.  Only the kernel accesses registers.
>>
>> I don't know whether that model makes sense or not for s390, but 
>> please consider it.
> We do the same for many instructions (arch/s390/kvm/instruction.c). 
> User exits
> are only performed for isntructions that cannot be handled in kernel. 
> Also, we do exit with requests to userspace, see the s390_reset exit 
> reason. I think in this regard, our implementation is very similar to 
> x86. Btw: this is something I did copycat from your implementation on 
> integration into kvm. The original zlive code did handle all 
> instructions in userland.

So why not do it for this instruction as well?  Instead of updating the 
psw, return a success/error code and let the kernel update psw.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 10/22/2009 12:43 PM, Carsten Otte wrote:
> Avi Kivity wrote:
>> On x86 we avoid emulating instructions in userspace.  Instead the 
>> kernel requests userspace to do something (triggered by the 
>> instruction), and the kernel does anything which might be implied by 
>> the instruction (like copying the result into a register, or updating 
>> pc).
>>
>> An example is port I/O.  instead of userspace reading %edx to query 
>> the port number and setting %eax to indicate the result, userspace 
>> reads a port number struct field and writes an I/O result struct 
>> field.  Only the kernel accesses registers.
>>
>> I don't know whether that model makes sense or not for s390, but 
>> please consider it.
> We do the same for many instructions (arch/s390/kvm/instruction.c). 
> User exits
> are only performed for isntructions that cannot be handled in kernel. 
> Also, we do exit with requests to userspace, see the s390_reset exit 
> reason. I think in this regard, our implementation is very similar to 
> x86. Btw: this is something I did copycat from your implementation on 
> integration into kvm. The original zlive code did handle all 
> instructions in userland.

So why not do it for this instruction as well?  Instead of updating the 
psw, return a success/error code and let the kernel update psw.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> So why not do it for this instruction as well?  Instead of updating the 
> psw, return a success/error code and let the kernel update psw.
It's not a single instruction, but a set of reasons we need the psw in 
userspace:
- for logging the instruction address on exits
- to check if a wait-type psw is active, and if so to switch between:
	- enabled wait psws, where interrupts could wake the cpu (aka
	  /halt, which needs to be handled in-kernel)
	- disabled wait psws, where CPUs are suspended due to cpu
           hotremove, system dump on panic
- to handle cpus in stopped state, typically on regular
   reboots/shutdowns

- for setting the condition code during implementation of:
   - all s/390 I/O instructions
   - IPI for restarting remote CPUs (that are not in KVM_RUN)
   - IPI for storing remote CPU status into the remote cpu's lowcore page
   - IPI for resetting remote CPUs
   - for all IPIs where the remote CPU is not known to the kernel, such
     as the detection loop for all 65535 cpu slots on startup of the
     guest kernel
   - for interpretion of service calls (usually to the machine's service
     processor)
- virtio I/O

The sweet thing about having the psw in userspace is that for almost all 
above reasons, there's nothing more than the psw that we need to have in 
order to perform that operation. Giving the 128-bit psw to userspace 
does simply save us having special cases for above list, with an 
exit_reason and a struct for each of them.

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Carsten Otte]

Avi Kivity wrote:
> So why not do it for this instruction as well?  Instead of updating the 
> psw, return a success/error code and let the kernel update psw.
It's not a single instruction, but a set of reasons we need the psw in 
userspace:
- for logging the instruction address on exits
- to check if a wait-type psw is active, and if so to switch between:
	- enabled wait psws, where interrupts could wake the cpu (aka
	  /halt, which needs to be handled in-kernel)
	- disabled wait psws, where CPUs are suspended due to cpu
           hotremove, system dump on panic
- to handle cpus in stopped state, typically on regular
   reboots/shutdowns

- for setting the condition code during implementation of:
   - all s/390 I/O instructions
   - IPI for restarting remote CPUs (that are not in KVM_RUN)
   - IPI for storing remote CPU status into the remote cpu's lowcore page
   - IPI for resetting remote CPUs
   - for all IPIs where the remote CPU is not known to the kernel, such
     as the detection loop for all 65535 cpu slots on startup of the
     guest kernel
   - for interpretion of service calls (usually to the machine's service
     processor)
- virtio I/O

The sweet thing about having the psw in userspace is that for almost all 
above reasons, there's nothing more than the psw that we need to have in 
order to perform that operation. Giving the 128-bit psw to userspace 
does simply save us having special cases for above list, with an 
exit_reason and a struct for each of them.

Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

Carsten Otte wrote:
> Avi Kivity wrote:
>> So why not do it for this instruction as well?  Instead of updating
>> the psw, return a success/error code and let the kernel update psw.
> It's not a single instruction, but a set of reasons we need the psw in
> userspace:
> - for logging the instruction address on exits
> - to check if a wait-type psw is active, and if so to switch between:
>     - enabled wait psws, where interrupts could wake the cpu (aka
>       /halt, which needs to be handled in-kernel)
>     - disabled wait psws, where CPUs are suspended due to cpu
>           hotremove, system dump on panic
> - to handle cpus in stopped state, typically on regular
>   reboots/shutdowns
>
> - for setting the condition code during implementation of:
>   - all s/390 I/O instructions
>   - IPI for restarting remote CPUs (that are not in KVM_RUN)
>   - IPI for storing remote CPU status into the remote cpu's lowcore page
>   - IPI for resetting remote CPUs
>   - for all IPIs where the remote CPU is not known to the kernel, such
>     as the detection loop for all 65535 cpu slots on startup of the
>     guest kernel
>   - for interpretion of service calls (usually to the machine's service
>     processor)
> - virtio I/O
>
> The sweet thing about having the psw in userspace is that for almost
> all above reasons, there's nothing more than the psw that we need to
> have in order to perform that operation. Giving the 128-bit psw to
> userspace does simply save us having special cases for above list,
> with an exit_reason and a struct for each of them.

Any progress on the patch? This is really important to make KVM work
properly on S390. I'd even go as far as suggesting it for linux-stable.

Alex

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

Carsten Otte wrote:
> Avi Kivity wrote:
>> So why not do it for this instruction as well?  Instead of updating
>> the psw, return a success/error code and let the kernel update psw.
> It's not a single instruction, but a set of reasons we need the psw in
> userspace:
> - for logging the instruction address on exits
> - to check if a wait-type psw is active, and if so to switch between:
>     - enabled wait psws, where interrupts could wake the cpu (aka
>       /halt, which needs to be handled in-kernel)
>     - disabled wait psws, where CPUs are suspended due to cpu
>           hotremove, system dump on panic
> - to handle cpus in stopped state, typically on regular
>   reboots/shutdowns
>
> - for setting the condition code during implementation of:
>   - all s/390 I/O instructions
>   - IPI for restarting remote CPUs (that are not in KVM_RUN)
>   - IPI for storing remote CPU status into the remote cpu's lowcore page
>   - IPI for resetting remote CPUs
>   - for all IPIs where the remote CPU is not known to the kernel, such
>     as the detection loop for all 65535 cpu slots on startup of the
>     guest kernel
>   - for interpretion of service calls (usually to the machine's service
>     processor)
> - virtio I/O
>
> The sweet thing about having the psw in userspace is that for almost
> all above reasons, there's nothing more than the psw that we need to
> have in order to perform that operation. Giving the 128-bit psw to
> userspace does simply save us having special cases for above list,
> with an exit_reason and a struct for each of them.

Any progress on the patch? This is really important to make KVM work
properly on S390. I'd even go as far as suggesting it for linux-stable.

Alex

Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 11/02/2009 10:23 PM, Alexander Graf wrote:
> Any progress on the patch? This is really important to make KVM work
> properly on S390. I'd even go as far as suggesting it for linux-stable.
>
>    

I forgot all about it, sorry.  Marcelo, can you commit it?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Avi Kivity]

On 11/02/2009 10:23 PM, Alexander Graf wrote:
> Any progress on the patch? This is really important to make KVM work
> properly on S390. I'd even go as far as suggesting it for linux-stable.
>
>    

I forgot all about it, sorry.  Marcelo, can you commit it?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 8/9] Set default console to virtio on S390x

[Alexander Graf]

All "normal" system emulation targets in qemu I'm aware of display output
on either VGA or serial output.

Our S390x virtio machine doesn't have such kind of legacy hardware. So
instead we need to default to a virtio console.

I'm not particularly proud of this patch. It would be a lot better to
have something in the machine description that tells us about the default
terminal.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 vl.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index eb2744e..e7f1504 100644
--- a/vl.c
+++ b/vl.c
@@ -4818,6 +4818,20 @@ int main(int argc, char **argv, char **envp)
     cyls = heads = secs = 0;
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
+#ifdef TARGET_S390X
+    for(i = 0; i < MAX_SERIAL_PORTS; i++)
+        serial_devices[i] = NULL;
+    serial_device_index = 0;
+
+    for(i = 0; i < MAX_PARALLEL_PORTS; i++)
+        parallel_devices[i] = NULL;
+    parallel_device_index = 0;
+
+    virtio_consoles[0] = "mon:stdio";
+    for(i = 1; i < MAX_VIRTIO_CONSOLES; i++)
+        virtio_consoles[i] = NULL;
+    virtio_console_index = 0;
+#else
     serial_devices[0] = "vc:80Cx24C";
     for(i = 1; i < MAX_SERIAL_PORTS; i++)
         serial_devices[i] = NULL;
@@ -4831,6 +4845,7 @@ int main(int argc, char **argv, char **envp)
     for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
         virtio_consoles[i] = NULL;
     virtio_console_index = 0;
+#endif
 
     monitor_devices[0] = "vc:80Cx24C";
     for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
@@ -5709,6 +5724,17 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
         }
+        for (i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
+            const char *devname = virtio_consoles[i];
+            if (devname && !strcmp(devname,"mon:stdio")) {
+                monitor_devices[0] = NULL;
+                break;
+            } else if (devname && !strcmp(devname,"stdio")) {
+                monitor_devices[0] = NULL;
+                virtio_consoles[i] = "mon:stdio";
+                break;
+            }
+        }
     }
 
     if (nb_numa_nodes > 0) {
-- 
1.6.0.2

[Qemu-devel] [PATCH 8/9] Set default console to virtio on S390x

[Alexander Graf]

All "normal" system emulation targets in qemu I'm aware of display output
on either VGA or serial output.

Our S390x virtio machine doesn't have such kind of legacy hardware. So
instead we need to default to a virtio console.

I'm not particularly proud of this patch. It would be a lot better to
have something in the machine description that tells us about the default
terminal.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 vl.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index eb2744e..e7f1504 100644
--- a/vl.c
+++ b/vl.c
@@ -4818,6 +4818,20 @@ int main(int argc, char **argv, char **envp)
     cyls = heads = secs = 0;
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
+#ifdef TARGET_S390X
+    for(i = 0; i < MAX_SERIAL_PORTS; i++)
+        serial_devices[i] = NULL;
+    serial_device_index = 0;
+
+    for(i = 0; i < MAX_PARALLEL_PORTS; i++)
+        parallel_devices[i] = NULL;
+    parallel_device_index = 0;
+
+    virtio_consoles[0] = "mon:stdio";
+    for(i = 1; i < MAX_VIRTIO_CONSOLES; i++)
+        virtio_consoles[i] = NULL;
+    virtio_console_index = 0;
+#else
     serial_devices[0] = "vc:80Cx24C";
     for(i = 1; i < MAX_SERIAL_PORTS; i++)
         serial_devices[i] = NULL;
@@ -4831,6 +4845,7 @@ int main(int argc, char **argv, char **envp)
     for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
         virtio_consoles[i] = NULL;
     virtio_console_index = 0;
+#endif
 
     monitor_devices[0] = "vc:80Cx24C";
     for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
@@ -5709,6 +5724,17 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
         }
+        for (i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
+            const char *devname = virtio_consoles[i];
+            if (devname && !strcmp(devname,"mon:stdio")) {
+                monitor_devices[0] = NULL;
+                break;
+            } else if (devname && !strcmp(devname,"stdio")) {
+                monitor_devices[0] = NULL;
+                virtio_consoles[i] = "mon:stdio";
+                break;
+            }
+        }
     }
 
     if (nb_numa_nodes > 0) {
-- 
1.6.0.2