Re: [Qemu-devel] [PATCH 2/6] s390: sclp base support

[Andreas Färber]

Am 13.07.2012 12:52, schrieb Christian Borntraeger:
> 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>
> ---
>  hw/s390-sclp.c           |  148 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390-sclp.h           |   80 +++++++++++++++++++++++++
>  hw/s390-virtio.c         |    3 +
>  hw/s390x/Makefile.objs   |    1 +
>  target-s390x/cpu.c       |   17 ++++++
>  target-s390x/cpu.h       |   18 ++----
>  target-s390x/kvm.c       |    5 +-
>  target-s390x/op_helper.c |   45 ++------------
>  8 files changed, 261 insertions(+), 56 deletions(-)
>  create mode 100644 hw/s390-sclp.c
>  create mode 100644 hw/s390-sclp.h
> 
> diff --git a/hw/s390-sclp.c b/hw/s390-sclp.c
> new file mode 100644
> index 0000000..74a3e66
> --- /dev/null
> +++ b/hw/s390-sclp.c
> @@ -0,0 +1,148 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <address@hidden>
> + *  Heinz Graalfs <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "cpu.h"
> +#include "kvm.h"
> +#include "sysbus.h"
> +
> +#include "s390-sclp.h"
> +
> +/* Provide information about the configuration, CPUs and storage */
> +static int 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;
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +
> +    return 0;
> +}
> +
> +static int sclp_execute(SCCB *sccb, uint64_t code)
> +{
> +    int r = 0;
> +
> +    switch (code) {
> +    case SCLP_CMDW_READ_SCP_INFO:
> +    case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +        r = read_SCP_info(sccb);
> +        break;
> +    default:
> +#ifdef DEBUG_HELPER
> +        printf("KVM: invalid sclp call 0x%x / 0x%" PRIx64 "x\n", sccb, code);

sccb is a pointer so %x seems wrong for 64-bit host. %p?

> +#endif
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        break;
> +    }
> +    return r;
> +}
> +
> +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 ||
> +        be16_to_cpu(work_sccb.h.length) > 4096) {
> +        r = -PGM_SPECIFICATION;
> +        goto out;
> +    }
> +
> +    r = sclp_execute((SCCB *)&work_sccb, code);
> +
> +    cpu_physical_memory_write(sccb, &work_sccb,
> +                              be16_to_cpu(work_sccb.h.length));
> +    if (!r) {
> +        sclp_service_interrupt(sccb);
> +    }
> +
> +out:
> +    return r;
> +}
> +
> +void sclp_service_interrupt(uint32_t sccb)
> +{
> +    if (!sccb) {
> +        return;
> +    }
> +    s390_sclp_extint(sccb & ~3);
> +}
> +
> +/* qemu object creation and initialization functions */
> +
> +#define S390_SCLP_BUS(obj) OBJECT_CHECK(SCLPS390Bus, (obj), 
> TYPE_S390_SCLP_BUS)

This should probably be in the header alongside TYPE_S390_SCLP_BUS?
Otherwise a separating white line would be nice.

> +static const TypeInfo s390_sclp_bus_info = {
> +    .name = TYPE_S390_SCLP_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(SCLPS390Bus),
> + };
> +
> +SCLPS390Bus *s390_sclp_bus_init(void)
> +{
> +    SCLPS390Bus *bus;
> +    BusState *bus_state;
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "s390-sclp-bridge");
> +    qdev_init_nofail(dev);
> +
> +    bus_state = qbus_create(TYPE_S390_SCLP_BUS, dev, "s390-sclp-bus");

Use TYPE_S390_SCLP_BUS constant?

> +    bus_state->allow_hotplug = 0;
> +
> +    bus = DO_UPCAST(SCLPS390Bus, bus, bus_state);
> +    return bus;
> +}
> +
> +static int s390_sclp_bridge_init(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static void s390_sclp_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = s390_sclp_bridge_init;
> +    dc->no_user = 1;
> +}
> +
> +static TypeInfo s390_sclp_bridge_info = {
> +    .name          = "s390-sclp-bridge",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SysBusDevice),
> +    .class_init    = s390_sclp_bridge_class_init,
> +};
> +
> +static void s390_sclp_register_types(void)
> +{
> +    type_register_static(&s390_sclp_bridge_info);
> +    type_register_static(&s390_sclp_bus_info);
> +}
> +type_init(s390_sclp_register_types)
> diff --git a/hw/s390-sclp.h b/hw/s390-sclp.h
> new file mode 100644
> index 0000000..f7bf140
> --- /dev/null
> +++ b/hw/s390-sclp.h
> @@ -0,0 +1,80 @@
> +/*
> + * SCLP Support
> + *
> + * Copyright IBM, Corp. 2007, 2012
> + *
> + * Authors:
> + *  Christian Borntraeger <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_S390_SCLP_H
> +#define _QEMU_S390_SCLP_H
> +
> +#include "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;
> +} __attribute__((packed)) SCCBHeader;
> +
> +#define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> +
> +typedef struct ReadInfo {
> +    SCCBHeader h;
> +    uint16_t rnmax;
> +    uint8_t rnsize;
> +} __attribute__((packed)) ReadInfo;
> +
> +typedef struct SCCB {
> +    SCCBHeader h;
> +    char data[SCCB_DATA_LEN];
> + } __attribute__((packed)) SCCB;
> +
> +#define TYPE_S390_SCLP_BUS "s390-sclp-bus"
> +
> +typedef struct S390SCLPDevice {
> +    DeviceState qdev;

I note that this is probably the first device directly derived from
DeviceState. This should be possible now after having merged the QOM
QBus series. Test case to check is "info qdm" from the monitor.

> +} S390SCLPDevice;
> +
> +typedef struct SCLPS390Bus {
> +    BusState bus;
> +} SCLPS390Bus;
> +
> +extern SCLPS390Bus *sclp_bus;
> +
> +SCLPS390Bus *s390_sclp_bus_init(void);
> +
> +void sclp_service_interrupt(uint32_t sccb);
> +
> +#endif
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 47eed35..577fcee 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -32,6 +32,7 @@
>  #include "exec-memory.h"
>  
>  #include "hw/s390-virtio-bus.h"
> +#include "hw/s390-sclp.h"
>  
>  //#define DEBUG_S390
>  
> @@ -61,6 +62,7 @@
>  #define MAX_BLK_DEVS                    10
>  
>  static VirtIOS390Bus *s390_bus;
> +SCLPS390Bus *sclp_bus;

I don't like this so much. We shouldn't be following that example and
adding global state for each bus there. Can't we add a child property to
the machine for the hosting device so that we can obtain that via QOM
path and access the bus from there?

>  static S390CPU **ipi_states;
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> @@ -183,6 +185,7 @@ static void s390_init(ram_addr_t my_ram_size,
>  
>      /* get a BUS */
>      s390_bus = s390_virtio_bus_init(&my_ram_size);
> +    sclp_bus = s390_sclp_bus_init();
>  
>      /* allocate RAM */
>      memory_region_init_ram(ram, "s390.ram", my_ram_size);
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index dcdcac8..b2d577b 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 += s390-sclp.o
>  
>  obj-y := $(addprefix ../,$(obj-y))

IIUC we have a dependency on the CPU inside the device code. If so,
please move the s390-sclp.c file into hw/s390x/ and move the obj-y +=
line below the addprefix line.

> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..8900872 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -21,9 +21,26 @@
>   */
>  
>  #include "cpu.h"
> +#include "kvm.h"
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
>  
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm)
> +{
> +    S390CPU *dummy_cpu = s390_cpu_addr2state(0);
> +    CPUS390XState *env = &dummy_cpu->env;
> +
> +    if (kvm_enabled()) {
> +#ifdef CONFIG_KVM
> +        kvm_s390_interrupt_internal(env, KVM_S390_INT_SERVICE, parm, 0, 1);
> +#endif
> +    } else {
> +        env->psw.addr += 4;
> +        cpu_inject_ext(env, EXT_SERVICE, parm, 0);
> +    }
> +}

Not so happy about this placement, it is not called s390_cpu_, the
prototype is in cpu.h not cpu-qom.h and it operates only indirectly on
the CPU. Is there another place for it?

> +
>  
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index c30ac3a..abdd838 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);
> @@ -315,11 +316,15 @@ static inline void 
> kvm_s390_interrupt_internal(CPUS390XState *env, int type,
>                                                 int vm)
>  {
>  }
> +
>  #endif

Stray whitespace change? If we want to do it, we might want to add one
after #endif as well and after #ifdef in the preceding hunk.

Regards,
Andreas

>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  void s390_add_running_cpu(CPUS390XState *env);
>  unsigned s390_del_running_cpu(CPUS390XState *env);
>  
> +/* service interrupts are floating therefore we must not pass an cpustate */
> +void s390_sclp_extint(uint32_t parm);
> +
>  /* from s390-virtio-bus */
>  extern const target_phys_addr_t virtio_size;
>  
> @@ -593,17 +598,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 */
> @@ -952,7 +946,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 654f87d..68c4f3e 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
>  };
> @@ -237,7 +234,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 91dd8dc..e7bb980 100644
> --- a/target-s390x/op_helper.c
> +++ b/target-s390x/op_helper.c
> @@ -2362,20 +2362,12 @@ 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);
> -}
> -
>  /*
> + * 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)
>  {
> -    int r = 0;
> -    int shift = 0;
> -
>  #ifdef DEBUG_HELPER
>      printf("sclp(0x%x, 0x%" PRIx64 ")\n", sccb, code);
>  #endif
> @@ -2388,35 +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);
> -
> -            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);
> -            }
> -            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 */
> @@ -2424,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;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg