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. 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? > >> } >> >> 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: cborntra@br96egxr:/space/qemu$ egrep "include.*hw" target-*/* | wc -l 39 [...9 >> - if (kvm_enabled()) { >> -#ifdef CONFIG_KVM >> - kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, >> - sccb & ~3, 0, 1); >> -#endif >> - } else { >> - env->psw.addr += 4; >> - ext_interrupt(env, EXT_SERVICE, sccb & ~3, 0); >> - } >> + r = sclp_read_info(env, &work_sccb); >> > > Maybe we should have a list of callbacks that hw/ code can register for? > Like the spapr hcalls. We will have a look if thats a way to go.