Re: [PATCH] Make S390x work in qemu-kvm

[Alexander Graf <agraf@suse.de>]

Avi Kivity wrote:
> On 12/08/2009 05:35 PM, Alexander Graf wrote:
>> We now have S390x KVM support in qemu upstream.
>>
>> Unfortunately it doesn't work in qemu-kvm, because that has its own main
>> loop and slightly different calling conventions for the KVM helpers.
>>
>> So let's hack in some small compat ifdefs that make qemu-kvm work on
>> S390x!
>>
>>
>>
>>
>> diff --git a/hw/msix.c b/hw/msix.c
>> index 6d598ee..b2c2857 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -11,6 +11,8 @@
>>    * the COPYING file in the top-level directory.
>>    */
>>
>> +#ifndef __s390x__
>> +
>>    
>
> This should be CONFIG_PCI or CONFIG_PCI_MSI.  Also, better to hack it
> at the Makefile level.

That's what I did at first in a hacky way. To be honest, the new
makefile structure scares me a bit, so I'm not sure I'll easily figure
out how to do that properly :).

>
>> diff --git a/hw/msix.h b/hw/msix.h
>> index a9f7993..643b3a1 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -4,6 +4,37 @@
>>   #include "qemu-common.h"
>>   #include "pci.h"
>>
>> +#ifdef __s390x__
>>    
>
> Ditto (minus Makefile).
>
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index cc21ee6..ea74955 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -179,7 +179,9 @@ static void s390_init(ram_addr_t ram_size,
>>               exit(1);
>>           }
>>
>> +#ifdef KVM_UPSTREAM
>>           cpu_synchronize_state(env);
>> +#endif
>>           env->psw.addr = KERN_IMAGE_START;
>>           env->psw.mask = 0x0000000180000000UL;
>>       }
>> @@ -236,6 +238,10 @@ static void s390_init(ram_addr_t ram_size,
>>           qdev_prop_set_drive(dev, "drive", dinfo);
>>           qdev_init_nofail(dev);
>>       }
>> +
>> +#ifndef KVM_UPSTREAM
>> +    kvm_arch_load_regs(env);
>> +#endif
>>   }
>>    
>
> Why isn't cpu_synchronize_state() suitable?

Because the KVM fd's are not available yet.

>
>>
>>   static QEMUMachine s390_machine = {
>> diff --git a/kvm-tpr-opt.c b/kvm-tpr-opt.c
>> index 2565d79..e8c4185 100644
>> --- a/kvm-tpr-opt.c
>> +++ b/kvm-tpr-opt.c
>> @@ -6,6 +6,8 @@
>>    * Licensed under the terms of the GNU GPL version 2 or higher.
>>    */
>>
>> +#ifndef __s390x__
>>    
>
> @Makefile.
>
>
>> diff --git a/kvm/include/linux/kvm.h b/kvm/include/linux/kvm.h
>> index db10887..2d241da 100644
>> --- a/kvm/include/linux/kvm.h
>> +++ b/kvm/include/linux/kvm.h
>>    
>
> Please post kvm header changes as a separate patch.

Ok.

>
>> diff --git a/qemu-kvm-helper.c b/qemu-kvm-helper.c
>> index 9420eb1..d5b75bd 100644
>> --- a/qemu-kvm-helper.c
>> +++ b/qemu-kvm-helper.c
>> @@ -30,7 +30,9 @@ void qemu_kvm_call_with_env(void (*func)(void *),
>> void *data, CPUState *newenv)
>>
>>   static void call_helper_cpuid(void *junk)
>>   {
>> +#ifndef __s390x__
>>       helper_cpuid();
>> +#endif
>>   }
>>    
>
> That just has to die (but not in your patches).  It dates from the
> pre-tcg days.

Yep. IMHO all the code duplication should die :).

>
>> @@ -157,7 +157,7 @@ static void init_slots(void)
>>
>>   static int get_free_slot(kvm_context_t kvm)
>>   {
>> -    int i;
>> +    int i = 0;
>>       int tss_ext;
>>
>>   #if defined(KVM_CAP_SET_TSS_ADDR)&&  !defined(__s390__)
>> @@ -171,10 +171,12 @@ static int get_free_slot(kvm_context_t kvm)
>>        * slot 0 to hold the extended memory, as the vmx will use the
>> last 3
>>        * pages of this slot.
>>        */
>> +#ifndef __s390x__
>>       if (tss_ext>  0)
>>           i = 0;
>>       else
>>           i = 1;
>> +#endif
>>    
>
> That should conditioned on x86, not s390.  While you're at it, drop
> the i = 0 assignment please.

So it's useless for IA64 as well?

>
>>
>>       for (; i<  KVM_MAX_NUM_MEM_REGIONS; ++i)
>>           if (!slots[i].len)
>> @@ -450,6 +452,14 @@ static void kvm_create_vcpu(CPUState *env, int id)
>>       env->kvm_fd = r;
>>       env->kvm_state = kvm_state;
>>
>> +#ifdef __s390x__
>> +    r = kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, 0);
>> +    if (r<  0) {
>> +        fprintf(stderr, "kvm_s390_initial_reset: %m\n");
>> +        exit(1);
>> +    }
>> +#endif
>>    
>
> Isn't there an arch hook for this?
>
> TARGET_S390 or similar.

Yes, there is. I figured this is just a temporary hack, so who cares :-).

>
>>           switch (run->exit_reason) {
>>           case KVM_EXIT_UNKNOWN:
>> @@ -981,14 +990,6 @@ int kvm_run(CPUState *env)
>>           case KVM_EXIT_SHUTDOWN:
>>               r = handle_shutdown(kvm, env);
>>               break;
>> -#if defined(__s390__)
>> -        case KVM_EXIT_S390_SIEIC:
>> -            r = kvm_s390_handle_intercept(kvm, env, run);
>> -            break;
>> -        case KVM_EXIT_S390_RESET:
>> -            r = kvm_s390_handle_reset(kvm, env, run);
>> -            break;
>> -#endif
>>    
>
> Ditto.

Ditto what? This is code removal.

>
>>
>> +#ifdef __s390x__
>> +static
>> +#endif
>>    
>
> Lovely.  Why?

Because it collided with the init function provided by target-s390x/kvm.c.

>
>>
>>   int kvm_arch_init_vcpu(CPUState *env)
>>   {
>> @@ -86,12 +88,27 @@ int kvm_arch_init_vcpu(CPUState *env)
>>       return ret;
>>   }
>>
>> +#ifdef KVM_UPSTREAM
>>   void kvm_arch_reset_vcpu(CPUState *env)
>> +#else
>> +void kvm_arch_cpu_reset(CPUState *env)
>> +#endif
>>    
>
> :(

Yeah, feel like getting the naming a bit closer? :)

>
>> index 0000000..9a6bcd6
>> --- /dev/null
>> +++ b/target-s390x/libkvm.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * This header is for functions&  variables that will ONLY be
>> + * used inside libkvm for x86.
>> + * THESE ARE NOT EXPOSED TO THE USER AND ARE ONLY FOR USE
>> + * WITHIN LIBKVM.
>> + *
>> + * derived from libkvm.c
>> + *
>> + * Copyright (C) 2006 Qumranet, Inc.
>> + *
>> + * Authors:
>> + *    Avi Kivity<avi@qumranet.com>
>> + *    Yaniv Kamay<yaniv@qumranet.com>
>> + *
>> + * This work is licensed under the GNU LGPL license, version 2.
>> + */
>> +
>> +#ifndef KVM_X86_H
>> +#define KVM_X86_H
>> +
>> +#define PAGE_SIZE 4096ul
>> +#define PAGE_MASK (~(PAGE_SIZE - 1))
>> +
>> +#define smp_wmb()   asm volatile("" ::: "memory")
>> +
>> +#endif
>>    
>
> I thought we no longer include libkvm.h!
>


Some file failed to build without it. IIRC because PAGE_* was not defined.

Alex