Re: [Qemu-devel] [PATCH 1/4] s390: sclp base support

[Alexander Graf]

On 20.08.2012, at 16:28, Jens Freimann wrote:

> From: Heinz Graalfs <address@hidden>
> 
> This adds a more generic infrastructure for handling Service-Call
> requests on s390. Currently we only support a small subset of Read
> SCP Info directly in target-s390x. This patch provides the base
> infrastructure for supporting more commands and moves Read SCP
> Info.
> In the future we could add additional commands for hotplug, call
> home and event handling.
> 
> Signed-off-by: Heinz Graalfs <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> Signed-off-by: Jens Freimann <address@hidden>
> ---
> hw/s390x/Makefile.objs   |   1 +
> hw/s390x/sclp.c          | 107 +++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/sclp.h          |  76 +++++++++++++++++++++++++++++++++
> target-s390x/cpu.h       |  14 +------
> target-s390x/kvm.c       |   5 +--
> target-s390x/op_helper.c |  31 +++-----------
> 6 files changed, 192 insertions(+), 42 deletions(-)
> create mode 100644 hw/s390x/sclp.c
> create mode 100644 hw/s390x/sclp.h
> 
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index dcdcac8..1c14b96 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -1,3 +1,4 @@
> obj-y = s390-virtio-bus.o s390-virtio.o
> 
> obj-y := $(addprefix ../,$(obj-y))
> +obj-y += sclp.o
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> new file mode 100644
> index 0000000..322a0e2
> --- /dev/null
> +++ b/hw/s390x/sclp.c
> @@ -0,0 +1,107 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <address@hidden>
> + *  Heinz Graalfs <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at 
> your
> + * option) any later version.  See the COPYING file in the top-level 
> directory.
> + *
> + */
> +
> +#include "cpu.h"
> +#include "kvm.h"
> +
> +#include "sclp.h"
> +
> +/* Provide information about the configuration, CPUs and storage */
> +static void read_SCP_info(SCCB *sccb)
> +{
> +    ReadInfo *read_info = (ReadInfo *) sccb;
> +    int shift = 0;
> +
> +    while ((ram_size >> (20 + shift)) > 65535) {
> +        shift++;
> +    }
> +    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> +    read_info->rnsize = 1 << shift;

Any reason we can't just always expose rnmax2 and rnsize2 instead? This way 
we're quite limited on the amount of RAM we can support, right?

> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +}
> +
> +static void sclp_execute(SCCB *sccb, uint64_t code)
> +{
> +    switch (code) {
> +    case SCLP_CMDW_READ_SCP_INFO:
> +    case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +        read_SCP_info(sccb);
> +        break;
> +    default:
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        break;
> +    }
> +}
> +
> +int do_sclp_service_call(uint32_t sccb, uint64_t code)
> +{
> +    int r = 0;
> +    SCCB work_sccb;
> +
> +    target_phys_addr_t sccb_len = sizeof(SCCB);
> +
> +    /*
> +     * 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
> +     */
> +    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
> +
> +    /* Valid sccb sizes */
> +    if (be16_to_cpu(work_sccb.h.length) < 8 ||

sizeof(SCCBHeader)

> +        be16_to_cpu(work_sccb.h.length) > 4096) {

SCCB_SIZE

> +        r = -PGM_SPECIFICATION;
> +        goto out;
> +    }
> +
> +    sclp_execute((SCCB *)&work_sccb, code);
> +
> +    cpu_physical_memory_write(sccb, &work_sccb,
> +                              be16_to_cpu(work_sccb.h.length));
> +
> +    sclp_service_interrupt(sccb);
> +
> +out:
> +    return r;
> +}
> +
> +void sclp_service_interrupt(uint32_t sccb)
> +{
> +    s390_sclp_extint(sccb & ~3);
> +}
> +
> +/* qemu object creation and initialization functions */
> +
> +static void s390_sclp_device_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *dc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    dc->init = s390_sclp_dev_init;
> +}
> +
> +static TypeInfo s390_sclp_device_info = {
> +    .name = TYPE_DEVICE_S390_SCLP,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(S390SCLPDevice),
> +    .class_init = s390_sclp_device_class_init,
> +    .class_size = sizeof(S390SCLPDeviceClass),
> +    .abstract = true,
> +};
> +
> +static void s390_sclp_register_types(void)
> +{
> +    type_register_static(&s390_sclp_device_info);
> +}
> +
> +type_init(s390_sclp_register_types)
> diff --git a/hw/s390x/sclp.h b/hw/s390x/sclp.h
> new file mode 100644
> index 0000000..e9ad42b
> --- /dev/null
> +++ b/hw/s390x/sclp.h
> @@ -0,0 +1,76 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at 
> your
> + * option) any later version.  See the COPYING file in the top-level 
> directory.
> + *
> + */
> +
> +#ifndef HW_S390_SCLP_H
> +#define HW_S390_SCLP_H
> +
> +#include <hw/sysbus.h>
> +#include <hw/qdev.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_NORMAL_READ_COMPLETION          0x0010
> +#define SCLP_RC_INVALID_SCLP_COMMAND            0x01f0
> +
> +/* Service Call Control Block (SCCB) and its elements */
> +
> +#define SCCB_SIZE 4096
> +
> +/*
> + * Normally packed structures are not the right thing to do, since all code
> + * must take care of endianess. We cant use ldl_phys and friends for two
> + * reasons, though:
> + * - some of the embedded structures below the SCCB can appear multiple times
> + *   at different locations, so there is no fixed offset
> + * - we work on a private copy of the SCCB, since there are several length
> + *   fields, that would cause a security nightmare if we allow the guest to
> + *   alter the structure while we parse it. We cannot use ldl_p and friends
> + *   either without doing pointer arithmetics
> + * So we have to double check that all users of sclp data structures use the
> + * right endianess wrappers.
> + */
> +typedef struct SCCBHeader {
> +    uint16_t length;
> +    uint8_t function_code;
> +    uint8_t control_mask[3];
> +    uint16_t response_code;
> +} QEMU_PACKED SCCBHeader;
> +
> +#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> +
> +typedef struct ReadInfo {
> +    SCCBHeader h;
> +    uint16_t rnmax;
> +    uint8_t rnsize;
> +} QEMU_PACKED ReadInfo;
> +
> +typedef struct SCCB {
> +    SCCBHeader h;
> +    char data[SCCB_DATA_LEN];
> + } QEMU_PACKED SCCB;
> +
> +typedef struct S390SCLPDevice {
> +    SysBusDevice busdev;
> +} S390SCLPDevice;
> +
> +typedef struct S390SCLPDeviceClass {
> +    DeviceClass qdev;
> +    int (*init)(S390SCLPDevice *sdev);
> +} S390SCLPDeviceClass;
> +
> +void sclp_service_interrupt(uint32_t sccb);
> +
> +#endif
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 18ac6e3..efd2cda 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -294,6 +294,7 @@ void s390x_tod_timer(void *opaque);
> void s390x_cpu_timer(void *opaque);
> 
> int s390_virtio_hypercall(CPUS390XState *env, uint64_t mem, uint64_t 
> hypercall);
> +int do_sclp_service_call(uint32_t sccb, uint64_t code);
> 
> #ifdef CONFIG_KVM
> void kvm_s390_interrupt(CPUS390XState *env, int type, uint32_t code);
> @@ -596,17 +597,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 */
> @@ -955,7 +945,7 @@ static inline void ebcdic_put(uint8_t *p, const char 
> *ascii, int len)
> void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
> int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t 
> asc,
>                   target_ulong *raddr, int *flags);
> -int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code);
> +int sclp_service_call(uint32_t sccb, uint64_t code);
> uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t 
> dst,
>                  uint64_t vr);
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 07edf93..bcb6b2e 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
> };
> @@ -272,7 +269,7 @@ static int kvm_sclp_service_call(CPUS390XState *env, 
> struct kvm_run *run,
>     sccb = env->regs[ipbh0 & 0xf];
>     code = env->regs[(ipbh0 & 0xf0) >> 4];
> 
> -    r = sclp_service_call(env, sccb, code);
> +    r = sclp_service_call(sccb, code);
>     if (r < 0) {
>         enter_pgmcheck(env, -r);
>     }
> diff --git a/target-s390x/op_helper.c b/target-s390x/op_helper.c
> index abc35dd..e7bb980 100644
> --- a/target-s390x/op_helper.c
> +++ b/target-s390x/op_helper.c
> @@ -2363,13 +2363,11 @@ static void program_interrupt(CPUS390XState *env, 
> uint32_t code, int ilc)
> }
> 
> /*
> + * we handle here the part that belongs to the cpu, e.g. program checks
>  * ret < 0 indicates program check, ret = 0,1,2,3 -> cc
>  */
> -int sclp_service_call(CPUS390XState *env, uint32_t sccb, uint64_t code)
> +int sclp_service_call(uint32_t sccb, uint64_t code)

Why not move the whole thing into sclp.c? The only thing remaining here are a 
few sanity checks that would just as well work in generic sclp handling code, 
right?


Alex

> {
> -    int r = 0;
> -    int shift = 0;
> -
> #ifdef DEBUG_HELPER
>     printf("sclp(0x%x, 0x%" PRIx64 ")\n", sccb, code);
> #endif
> @@ -2382,27 +2380,8 @@ int sclp_service_call(CPUS390XState *env, uint32_t 
> sccb, uint64_t code)
>         return -PGM_SPECIFICATION;
>     }
> 
> -    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);
> -
> -            s390_sclp_extint(sccb & ~3);
> -            break;
> -        default:
> -#ifdef DEBUG_HELPER
> -            printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, 
> code);
> -#endif
> -            r = 3;
> -            break;
> -    }
> -
> -    return r;
> +    /* the complex part is handled by external components */
> +    return do_sclp_service_call(sccb, code);
> }
> 
> /* SCLP service call */
> @@ -2410,7 +2389,7 @@ uint32_t HELPER(servc)(uint32_t r1, uint64_t r2)
> {
>     int r;
> 
> -    r = sclp_service_call(env, r1, r2);
> +    r = sclp_service_call(r1, r2);
>     if (r < 0) {
>         program_interrupt(env, -r, 4);
>         return 0;
> -- 
> 1.7.11.5
>