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

[malc <av1474@comtv.ru>]

On Wed, 2 Dec 2009, Alexander Graf wrote:

> 
> On 02.12.2009, at 09:12, Aurelien Jarno wrote:
> 
> > 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 <agraf@suse.de>
> >>>> ---
> >>>> 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 <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");
> >>> 
> >>> 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.

Yeah he does.

> 
> Hm - I just wanted to clearly show that this is an internal API, nobody 
> should really have to call directly. But I'm open for other naming 
> suggestions.

Thing is, in 7.1.3#1 standard says (after explicitly reserving __ _[A-Z]
for any use):
         -- All  identifiers  that  begin  with  an  underscore are
            always reserved for use as identifiers with file  scope
            in both the ordinary and tag name spaces.

And i could never really understand (or recall/comprehend when asked
and being given an answer) what this entails. (Anyone?)

So i would go with something imaginative like internal_do_not_use_kvm*,
but that's just me. You can go wild here, leading underscore doesn't look
attractive though.

-- 
mailto:av1474@comtv.ru