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

[Heinz Graalfs]

Hi Alex,

there was one leftover ...

On Mon, 2012-07-30 at 14:38 +0200, Alexander Graf wrote: 
> 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.
> 

we removed the complete sclp bus now and keep the event_facility pointer
within the current machines global property-list as opaque pointer

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