|
From: | Alexander Graf |
Subject: | Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions |
Date: | Tue, 12 Jun 2012 14:32:31 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:10.0.3) Gecko/20120306 Thunderbird/10.0.3 |
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_KVMYou 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
[Prev in Thread] | Current Thread | [Next in Thread] |