qemu-devel
[Top][All Lists]
Advanced

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

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions
Date: Tue, 12 Jun 2012 17:41:10 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1

On 06/12/2012 07:32 AM, Alexander Graf wrote:
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.

Indeed :-)


To me, the SCLP interface is similar to PIO, MMIO, SPAPR hypercalls, you name
it.

Yeah, the SPAPR hypercalls is a good one I think but I don't know enough about SCLP yet. From what's here, it would be pretty easy to model with qemu_irq I think.

We do that for target-i386 for things like the a20 line which is another case where random hardware interacts with the cpu in a far too personal fashion.

Regards,

Anthony Liguori

 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







reply via email to

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