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@xxxxxxxxxxxx>
>> + * Yaniv Kamay<yaniv@xxxxxxxxxxxx>
>> + *
>> + * 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
