Re: [Qemu-devel] [PATCH 04/11] Add KVM support for S390x

[Aurelien Jarno]

On Mon, Nov 30, 2009 at 11:25:03PM +0100, Alexander Graf wrote:
> 
> On 30.11.2009, at 19:18, Aurelien Jarno wrote:
> 
> > On Thu, Nov 26, 2009 at 02:23:13PM +0100, Alexander Graf wrote:
> >> 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!
> > 
> > Please find the comments below.
> > 
> >> Signed-off-by: Alexander Graf <address@hidden>
> >> ---
> >> configure          |    4 +-
> >> target-s390x/kvm.c |  463 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 466 insertions(+), 1 deletions(-)
> >> create mode 100644 target-s390x/kvm.c
> >> 
> >> diff --git a/configure b/configure
> >> index b616e1a..cf466ec 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -1348,6 +1348,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
> >> @@ -2360,7 +2362,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..d477664
> >> --- /dev/null
> >> +++ b/target-s390x/kvm.c
> >> @@ -0,0 +1,463 @@
> >> +/*
> >> + * QEMU S390x KVM implementation
> >> + *
> >> + * Copyright (c) 2009 Alexander Graf <address@hidden>
> >> + *
> >> + * 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");
> > 
> > coding style.
> > 
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +void kvm_arch_reset_vcpu(CPUState *env)
> >> +{
> >> +}
> > 
> > Is there really nothing to do? If some code has to be added later, it
> > may be worth adding a comment.
> 
> As you have probably realized by now, all reset code is missing. In fact, we 
> don't even ever call the qemu reset functions to actually do a reset.

Can you make it clean with an abort?

> >> +
> >> +static void _kvm_s390_interrupt(CPUState *env, int type, uint32_t parm, 
> >> uint64_t parm64, int vm)
> >> +{
> > 
> > Why such a name starting with an underscore?
> 
> Because that's the internal function that gets used by the exported, properly 
> named ones. Are there any conventions on how to declare private functions?

I don't think there is any convention, but I know malc always complains
about not introducing names starting with an underscore.

> >> +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:
> > 
> > What are those constants?
> 
> If I knew I'd have defined some more verbose constants. I just took them 1:1 
> from kuli.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net