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 <graa...@linux.vnet.ibm.com>
> >
> > 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 <graa...@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntrae...@de.ibm.com>
> > ---
> > 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 <borntrae...@de.ibm.com>
> > + * Heinz Graalfs <graa...@linux.vnet.ibm.com>
> > + *
> > + * 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
>
