[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions

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_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/.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]