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

[Anthony Liguori <anthony@codemonkey.ws>]

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;