From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Make S390x work in qemu-kvm Date: Tue, 15 Dec 2009 14:30:25 +0200 Message-ID: <4B278161.7080600@redhat.com> References: <1260286554-18253-1-git-send-email-agraf@suse.de> <4B276804.9080605@redhat.com> <4B276F4A.805@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Alexander Graf Return-path: Received: from mx1.redhat.com ([209.132.183.28]:4912 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751370AbZLOMa3 (ORCPT ); Tue, 15 Dec 2009 07:30:29 -0500 In-Reply-To: <4B276F4A.805@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On 12/15/2009 01:13 PM, Alexander Graf wrote: > >> 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 :). > I'm sure Juan will be glad to help. >>> + >>> +#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. > Then kvm_arch_load_regs() will fail as well, no? >>> 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? > Yes. >>> 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 :-). > The whole of qemu-kvm.c is a temporary hack. No reason to make it uglier than it needs to be. >>> 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. > Um, yes. >>> +#ifdef __s390x__ >>> +static >>> +#endif >>> >>> >> Lovely. Why? >> > Because it collided with the init function provided by target-s390x/kvm.c. > I'd prefer a separate rename then. >>> 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? :) > A renaming patch would be welcome. >> I thought we no longer include libkvm.h! >> >> > Some file failed to build without it. IIRC because PAGE_* was not defined. > There's a TARGET_PAGE_SIZE or something, we can use that instead. -- error compiling committee.c: too many arguments to function