From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeZjn-0004au-0w for qemu-devel@nongnu.org; Tue, 12 Jun 2012 18:39:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SeZjk-0006jK-Kb for qemu-devel@nongnu.org; Tue, 12 Jun 2012 18:38:58 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:46188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeZjk-0006j4-Am for qemu-devel@nongnu.org; Tue, 12 Jun 2012 18:38:56 -0400 Received: by dadv2 with SMTP id v2so78132dad.4 for ; Tue, 12 Jun 2012 15:38:53 -0700 (PDT) Message-ID: <4FD7C4F7.2090508@codemonkey.ws> Date: Tue, 12 Jun 2012 17:38:47 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1338984323-21914-1-git-send-email-jfrei@de.ibm.com> <1338984323-21914-6-git-send-email-jfrei@de.ibm.com> In-Reply-To: <1338984323-21914-6-git-send-email-jfrei@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jens Freimann Cc: Heinz Graalfs , Alexander Graf , qemu-devel , Christian Borntraeger , Jens Freimann , Cornelia Huck On 06/06/2012 07:05 AM, Jens Freimann wrote: > From: Christian Borntraeger > > 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 > Signed-off-by: Jens Freimann > --- > 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 > + * > + */ 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 > +#include 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;