Re: [Qemu-devel] [PATCH 5/8] s390: Cleanup sclp functions

[Alexander Graf]

Jens Freimann wrote:
> From: Christian Borntraeger <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Jens Freimann <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + */
> +
> +#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;
>