From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:32976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeQH5-0006Rf-Rw for qemu-devel@nongnu.org; Tue, 12 Jun 2012 08:32:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SeQGz-0004UK-Hf for qemu-devel@nongnu.org; Tue, 12 Jun 2012 08:32:43 -0400 Received: from cantor2.suse.de ([195.135.220.15]:48525 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeQGz-0004Tf-8a for qemu-devel@nongnu.org; Tue, 12 Jun 2012 08:32:37 -0400 Message-ID: <4FD736DF.60301@suse.de> Date: Tue, 12 Jun 2012 14:32:31 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1338984323-21914-1-git-send-email-jfrei@de.ibm.com> <1338984323-21914-6-git-send-email-jfrei@de.ibm.com> <4FD712D4.3040401@suse.de> <4FD73507.3040204@de.ibm.com> In-Reply-To: <4FD73507.3040204@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger Cc: Jens Freimann , Heinz Graalfs , qemu-devel , Jens Freimann , Cornelia Huck , =?ISO-8859-1?Q?Andreas_F=E4r?= =?ISO-8859-1?Q?ber?= On 06/12/2012 02:24 PM, Christian Borntraeger wrote: > Yes we will re-split the sclp patches. > > besides that, some comments: > > On 12/06/12 11:58, Alexander Graf wrote: >>> +#include "hw/s390-sclp.h" >>> >> No need for hw/. > will fix. > > >>> +void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb) >>> +{ >>> + if (!sccb) { >>> + return; >>> + } >>> + >>> + if (kvm_enabled()) { >>> +#ifdef CONFIG_KVM >>> >> You shouldn't know about CONFIG_KVM in hw/. So we have to generalize >> this code. > Ok, Maybe an exported interface for sending interrupts to the guest > under target-s390/ that hides the kvm/tcg thing. Yeah, or have KVM hook into the tcg interrupt dispatch loop at cpu_exec.c:cpu_exec(). Not sure which way is easier. > > > ice_call(CPUS390XState *env, struct kvm_run *run, >>> r = sclp_service_call(env, sccb, code); >>> if (r) { >>> setcc(env, 3); >>> + } else { >>> + setcc(env, 0); >>> >> This one looks like an actual fix that is not part of the cleanup? > Yes it is. Separate patch? Yes, please :). > >>> } >>> >>> return 0; >>> diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c >>> index 7b72473..74bd9ad 100644 >>> --- a/target-s390x/op_helper.c >>> +++ b/target-s390x/op_helper.c >>> @@ -31,6 +31,7 @@ >>> >>> #if !defined (CONFIG_USER_ONLY) >>> #include "sysemu.h" >>> +#include "hw/s390-sclp.h" >>> >> #include in hw/ from target-XXX is a no-go. It means our abstraction >> layer is broken. > Disagree here. The sclp is a processor that helps the CPU and there is a > tight link. This is similar to a PIC/APIC etc which are also under hw AND > included from target-386/ - among others: Which is exactly why Anthony is suggesting for years now to pull the APIC code into target-i386. To me, the SCLP interface is similar to PIO, MMIO, SPAPR hypercalls, you name it. We can certainly have sclp awareness in target-s390x, but please don't just blindly include headers from hw/. Split the few bits of information that we need in target-s390x into a separate header (clean) or target-s390x/cpu.h (hacky, but ok for now) and rather include that from hw/. Alex