[prev in list] [next in list] [prev in thread] [next in thread]
List: qemu-devel
Subject: Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions
From: Anthony Liguori <anthony () codemonkey ! ws>
Date: 2012-06-12 22:38:47
Message-ID: 4FD7C4F7.2090508 () codemonkey ! ws
[Download RAW message or body]
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;
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic