Re: [Qemu-devel] [PATCH 3/7] s390: sclp base support

[Alexander Graf]

On 24.07.2012, at 09:37, Christian Borntraeger 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>
> ---
> hw/s390-virtio.c         |    2 +
> hw/s390x/Makefile.objs   |    1 +
> hw/s390x/sclp.c          |  145 ++++++++++++++++++++++++++++++++++++++++++++++
> hw/s390x/sclp.h          |   79 +++++++++++++++++++++++++
> target-s390x/cpu.h       |   14 +----
> target-s390x/kvm.c       |    5 +-
> target-s390x/op_helper.c |   31 ++--------
> 7 files changed, 235 insertions(+), 42 deletions(-)
> create mode 100644 hw/s390x/sclp.c
> create mode 100644 hw/s390x/sclp.h
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 47eed35..28e320d 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/s390x/sclp.h"
> 
> //#define DEBUG_S390
> 
> @@ -183,6 +184,7 @@ static void s390_init(ram_addr_t my_ram_size,
> 
>     /* get a BUS */
>     s390_bus = s390_virtio_bus_init(&my_ram_size);
> +    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..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..4095ba6
> --- /dev/null
> +++ b/hw/s390x/sclp.c
> @@ -0,0 +1,145 @@
> +/*
> + * 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 <hw/sysbus.h>
> +#include "sclp.h"
> +
> +/* There is one SCLP bus per machine */
> +static SCLPS390Bus *sbus;

... but there isn't necessarily one machine per qemu instance. Today there is, 
but we shouldn't rely on that fact. Please move the bus variable into a machine 
struct that only the machine knows about.

> +
> +/* 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:
> +        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)

Does this have to be globally visible? If all the sclp source ends up in this 
file, it should only be necessary internal to it, right?

> +{
> +    if (!sccb) {

Please add a comment when this would trigger. In fact, I'm not sure I fully 
understand the reason for this function :).


Alex