On 06/06/2012 07:05 AM, Jens Freimann wrote:
From: Christian Borntraeger<borntrae...@de.ibm.com>
The sclp facility on s390 is a hardware that is external to the cpu.
Lets cleanup the definitions and move the functionality into a separate
file under hw/.
Signed-off-by: Christian Borntraeger<borntrae...@de.ibm.com>
Signed-off-by: Jens Freimann<jf...@de.ibm.com>
---
Makefile.target | 2 +-
hw/s390-sclp.c | 42 ++++++++++++++++++++++++++++++++++++++++++
hw/s390-sclp.h | 34 ++++++++++++++++++++++++++++++++++
target-s390x/cpu.h | 11 -----------
target-s390x/kvm.c | 5 ++---
target-s390x/op_helper.c | 39 +++++++++++++++++----------------------
6 files changed, 96 insertions(+), 37 deletions(-)
create mode 100644 hw/s390-sclp.c
create mode 100644 hw/s390-sclp.h
diff --git a/Makefile.target b/Makefile.target
index 1582904..fed2d72 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -374,7 +374,7 @@ obj-sh4-y += ide/mmio.o
obj-m68k-y = an5206.o mcf5206.o mcf_uart.o mcf_intc.o mcf5208.o mcf_fec.o
obj-m68k-y += m68k-semi.o dummy_m68k.o
-obj-s390x-y = s390-virtio-bus.o s390-virtio.o
+obj-s390x-y = s390-virtio-bus.o s390-virtio.o s390-sclp.o
obj-alpha-y = mc146818rtc.o
obj-alpha-y += alpha_pci.o alpha_dp264.o alpha_typhoon.o
diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
new file mode 100644
index 0000000..c046441
--- /dev/null
+++ b/hw/s390-sclp.c
@@ -0,0 +1,42 @@
+/*
+ * sclp facility
+ * Copyright IBM Corp. 2012
+ * Author: Christian Borntraeger<borntrae...@de.ibm.com>
+ *
+ */
Each file needs a license statement. Take a look at virtio.c for an example.
+#include "cpu.h"
+#include "kvm.h"
+#include "hw/s390-sclp.h"
+
+int sclp_read_info(CPUS390XState *env, struct sccb *sccb)
+{
+ int shift = 0;
+
+ while ((ram_size>> (20 + shift))> 65535) {
+ shift++;
+ }
+ sccb->c.read_info.rnmax = cpu_to_be16(ram_size>> (20 + shift));
+ sccb->c.read_info.rnsize = 1<< shift;
+ sccb->h.response_code = cpu_to_be16(0x10);
+
+ return 0;
+}
+
+void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb)
+{
+ if (!sccb) {
+ return;
+ }
+
+ 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;
+ cpu_inject_ext(env, EXT_SERVICE, (sccb& ~3), 0);
+ }
+}
+
As a basic rule, if it's in hw/, it shouldn't interact with CPUState.
If you need to raise an interrupt, you should use a qemu_irq.
I don't know anything about sclp. Does it use a reasonable calling convention
where the arguments are within specific registers such that you could pass an
array of ulongs as inputs and return a ulong as output?
diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
new file mode 100644
index 0000000..e335b21
--- /dev/null
+++ b/hw/s390-sclp.h
@@ -0,0 +1,34 @@
+#include<stdint.h>
+#include<qemu-common.h>
qemu-common.h is not a system header and stdint should not be required. You
also need a copyright/license statement.
+
+
+/* SCLP command codes */
+#define SCLP_CMDW_READ_SCP_INFO 0x00020001
+#define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
+
+/* SCLP response codes */
+#define SCLP_RC_SCCB_RESOURCE_INSUFFICENT 0x07f0
+
+struct sccb_header {
+ uint16_t length;
+#define SCLP_FC_NORMAL_WRITE 0
+ uint8_t function_code;
+ uint8_t control_mask[3];
+ uint16_t response_code;
+} __attribute__((packed));
This violates CodingStyle. The use of packed is always suspicious. It
typically indicates you aren't handling endianness correctly.
+
+struct read_info_sccb {
+ uint16_t rnmax;
+ uint8_t rnsize;
+} __attribute__((packed));
+
+struct sccb {
+ struct sccb_header h;
+ union {
+ struct read_info_sccb read_info;
+ char data[4088];
+ } c;
+ } __attribute__((packed));
+
+int sclp_read_info(CPUS390XState *env, struct sccb *sccb);
+void sclp_service_interrupt(CPUS390XState *env, uint32_t sccb);
You have no #ifdef guards on this header...
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 2f3f394..d0199d7 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -591,17 +591,6 @@ static inline const char *cc_name(int cc_op)
return cc_names[cc_op];
}
-/* SCLP PV interface defines */
-#define SCLP_CMDW_READ_SCP_INFO 0x00020001
-#define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
-
-#define SCP_LENGTH 0x00
-#define SCP_FUNCTION_CODE 0x02
-#define SCP_CONTROL_MASK 0x03
-#define SCP_RESPONSE_CODE 0x06
-#define SCP_MEM_CODE 0x08
-#define SCP_INCREMENT 0x0a
-
typedef struct LowCore
{
/* prefix area: defined by architecture */
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 73cfd1f..7a7604b 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -60,9 +60,6 @@
#define SIGP_STORE_STATUS_ADDR 0x0e
#define SIGP_SET_ARCH 0x12
-#define SCLP_CMDW_READ_SCP_INFO 0x00020001
-#define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
-
const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
KVM_CAP_LAST_INFO
};
@@ -246,6 +243,8 @@ static int kvm_sclp_service_call(CPUS390XState *env, struct
kvm_run *run,
r = sclp_service_call(env, sccb, code);
if (r) {
setcc(env, 3);
+ } else {
+ setcc(env, 0);
}
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"
#endif
/*****************************************************************************/
@@ -2360,16 +2361,13 @@ static void program_interrupt(CPUS390XState *env,
uint32_t code, int ilc)
}
}
-static void ext_interrupt(CPUS390XState *env, int type, uint32_t param,
- uint64_t param64)
-{
- cpu_inject_ext(env, type, param, param64);
-}
int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
{
int r = 0;
- int shift = 0;
+ struct sccb work_sccb;
+ struct sccb *guest_sccb;
+ target_phys_addr_t sccb_len = sizeof(*guest_sccb);
#ifdef DEBUG_HELPER
printf("sclp(0x%x, 0x%" PRIx64 ")\n", sccb, code);
@@ -2380,26 +2378,18 @@ int sclp_service_call(CPUS390XState *env, uint32_t
sccb, uint64_t code)
r = -1;
goto out;
}
+ /*
+ * we want to work on a private copy of the sccb, to prevent guests
+ * from playing dirty tricks by modifying the memory content after
+ * the host has checked the values
+ */
+ guest_sccb = cpu_physical_memory_map(sccb,&sccb_len, true);
+ memcpy(&work_sccb, guest_sccb, sizeof(*guest_sccb));
This is definitely wrong. You should use cpu_physical_mmeory_read()
switch(code) {
case SCLP_CMDW_READ_SCP_INFO:
case SCLP_CMDW_READ_SCP_INFO_FORCED:
- while ((ram_size>> (20 + shift))> 65535) {
- shift++;
- }
- stw_phys(sccb + SCP_MEM_CODE, ram_size>> (20 + shift));
- stb_phys(sccb + SCP_INCREMENT, 1<< shift);
- stw_phys(sccb + SCP_RESPONSE_CODE, 0x10);
-
- 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);
break;
default:
#ifdef DEBUG_HELPER
@@ -2408,6 +2398,11 @@ int sclp_service_call(CPUS390XState *env, uint32_t sccb,
uint64_t code)
r = -1;
break;
}
+ memcpy(guest_sccb,&work_sccb, work_sccb.h.length);
And then cpu_physical_memory_write(). Now you are handling endianness correctly
but I still think it's better to not rely on packed and instead read the
structure from memory using ldl_phys, etc.
+ cpu_physical_memory_unmap(guest_sccb, 4096, true, 4096);
It's very odd that you're passing 4096 here...
Regards,
Anthony Liguori
+ if (!r) {
+ sclp_service_interrupt(env, sccb);
+ }
out:
return r;
