| Message ID | 1338984323-21914-6-git-send-email-jfrei@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Jens Freimann wrote: > From: Christian Borntraeger <borntraeger@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/. > Phew. I'm not sure this is a great idea. At least not the way the code is structured now. Andreas, do you have any idea how to get this done nicely? We'd have to invent our own bus to communicate to the device, right? And then also actually spawn one. > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Jens Freimann <jfrei@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 <borntraeger@de.ibm.com> > + * > + */ > + > +#include "cpu.h" > +#include "kvm.h" > +#include "hw/s390-sclp.h" > No need for hw/. > + > +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 > You shouldn't know about CONFIG_KVM in hw/. So we have to generalize this code. > + 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); > + } > +} > + > 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> > + > + > +/* 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 > Please don't intertwine defines and struct definitions. > + uint8_t function_code; > + uint8_t control_mask[3]; > + uint16_t response_code; > +} __attribute__((packed)); > + > +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); > 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); > This one looks like an actual fix that is not part of the cleanup? > } > > 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. > #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)); > > 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); > Maybe we should have a list of callbacks that hw/ code can register for? Like the spapr hcalls. Alex > 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); > + cpu_physical_memory_unmap(guest_sccb, 4096, true, 4096); > + if (!r) { > + sclp_service_interrupt(env, sccb); > + } > > out: > return r; >
On 12/06/12 11:58, Alexander Graf wrote: > Jens Freimann wrote: >> From: Christian Borntraeger <borntraeger@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/. >> > > Phew. I'm not sure this is a great idea. At least not the way the code > is structured now. Andreas, do you have any idea how to get this done > nicely? We'd have to invent our own bus to communicate to the device, > right? And then also actually spawn one. There are followup patches which add qom etc see patch 6. Can you look at the end result?
On 12.06.2012, at 12:07, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 12/06/12 11:58, Alexander Graf wrote: >> Jens Freimann wrote: >>> From: Christian Borntraeger <borntraeger@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/. >>> >> >> Phew. I'm not sure this is a great idea. At least not the way the code >> is structured now. Andreas, do you have any idea how to get this done >> nicely? We'd have to invent our own bus to communicate to the device, >> right? And then also actually spawn one. > > There are followup patches which add qom etc see patch 6. Can you look at > the end result? Yeah, still trying to get my head around them :). Alex
On 12.06.2012, at 12:07, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 12/06/12 11:58, Alexander Graf wrote: >> Jens Freimann wrote: >>> From: Christian Borntraeger <borntraeger@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/. >>> >> >> Phew. I'm not sure this is a great idea. At least not the way the code >> is structured now. Andreas, do you have any idea how to get this done >> nicely? We'd have to invent our own bus to communicate to the device, >> right? And then also actually spawn one. > > There are followup patches which add qom etc see patch 6. Can you look at > the end result? Btw, the fact that this is a preparation step for more rework later is a pretty crucial part of the patch description :). Alex
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.
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/. Alex
On 06/06/2012 07:05 AM, Jens Freimann wrote: > From: Christian Borntraeger<borntraeger@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<borntraeger@de.ibm.com> > Signed-off-by: Jens Freimann<jfrei@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<borntraeger@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;
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 > > >
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 <borntraeger@de.ibm.com> + * + */ + +#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); + } +} + 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> + + +/* 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)); + +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); 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)); 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); + cpu_physical_memory_unmap(guest_sccb, 4096, true, 4096); + if (!r) { + sclp_service_interrupt(env, sccb); + } out: return r;