On Wed, Aug 8, 2012 at 8:17 AM, Cornelia Huck <cornelia.huck@xxxxxxxxxx> wrote:
> On Tue, 7 Aug 2012 21:00:59 +0000
> Blue Swirl <blauwirbel@xxxxxxxxx> wrote:
>
>
>> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> > new file mode 100644
>> > index 0000000..7941c44
>> > --- /dev/null
>> > +++ b/hw/s390x/css.c
>> > @@ -0,0 +1,440 @@
>> > +/*
>> > + * Channel subsystem base support.
>> > + *
>> > + * Copyright 2012 IBM Corp.
>> > + * Author(s): Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>> > + *
>> > + * 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 "qemu-thread.h"
>> > +#include "qemu-queue.h"
>> > +#include <hw/qdev.h>
>> > +#include "kvm.h"
>> > +#include "cpu.h"
>> > +#include "ioinst.h"
>> > +#include "css.h"
>> > +
>> > +struct chp_info {
>>
>> CamelCase, please.
>
> OK.
>>
>> > + uint8_t in_use;
>> > + uint8_t type;
>> > +};
>> > +
>> > +static struct chp_info chpids[MAX_CSSID + 1][MAX_CHPID + 1];
>> > +
>> > +static css_subch_cb_func css_subch_cb;
>>
>> Probably these can be put to a container structure which can be passed around.
>
> Still trying to come up with a good model for that.
>
>>
>
>> > + case CCW_CMD_SENSE_ID:
>> > + {
>> > + uint8_t sense_bytes[256];
>> > +
>> > + /* Sense ID information is device specific. */
>> > + memcpy(sense_bytes, &sch->id, sizeof(sense_bytes));
>> > + if (check_len) {
>> > + if (ccw->count != sizeof(sense_bytes)) {
>> > + ret = -EINVAL;
>> > + break;
>> > + }
>> > + }
>> > + len = MIN(ccw->count, sizeof(sense_bytes));
>> > + /*
>> > + * Only indicate 0xff in the first sense byte if we actually
>> > + * have enough place to store at least bytes 0-3.
>> > + */
>> > + if (len >= 4) {
>> > + stb_phys(ccw->cda, 0xff);
>> > + } else {
>> > + stb_phys(ccw->cda, 0);
>> > + }
>> > + i = 1;
>> > + for (i = 1; i < len - 1; i++) {
>> > + stb_phys(ccw->cda + i, sense_bytes[i]);
>> > + }
>>
>> cpu_physical_memory_write()
>
> Hm, what's wrong with storing byte-by-byte?
cpu_physical_memory_write() could be more optimal, for example resolve
guest addresses only once per page.
>
>>
>> > + sch->curr_status.scsw.count = ccw->count - len;
>> > + ret = 0;
>> > + break;
>> > + }
>> > + case CCW_CMD_TIC:
>> > + if (sch->last_cmd->cmd_code == CCW_CMD_TIC) {
>> > + ret = -EINVAL;
>> > + break;
>> > + }
>> > + if (ccw->flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
>> > + ret = -EINVAL;
>> > + break;
>> > + }
>> > + sch->channel_prog = qemu_get_ram_ptr(ccw->cda);
>> > + ret = sch->channel_prog ? -EAGAIN : -EFAULT;
>> > + break;
>> > + default:
>> > + if (sch->ccw_cb) {
>> > + /* Handle device specific commands. */
>> > + ret = sch->ccw_cb(sch, ccw);
>> > + } else {
>> > + ret = -EOPNOTSUPP;
>> > + }
>> > + break;
>> > + }
>> > + sch->last_cmd = ccw;
>> > + if (ret == 0) {
>> > + if (ccw->flags & CCW_FLAG_CC) {
>> > + sch->channel_prog += 8;
>> > + ret = -EAGAIN;
>> > + }
>> > + }
>> > +
>> > + return ret;
>
>> > diff --git a/hw/s390x/css.h b/hw/s390x/css.h
>> > new file mode 100644
>> > index 0000000..b8a95cc
>> > --- /dev/null
>> > +++ b/hw/s390x/css.h
>> > @@ -0,0 +1,62 @@
>> > +/*
>> > + * Channel subsystem structures and definitions.
>> > + *
>> > + * Copyright 2012 IBM Corp.
>> > + * Author(s): Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>> > + *
>> > + * 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.
>> > + */
>> > +
>> > +#ifndef CSS_H
>> > +#define CSS_H
>> > +
>> > +#include "ioinst.h"
>> > +
>> > +/* Channel subsystem constants. */
>> > +#define MAX_SCHID 65535
>> > +#define MAX_SSID 3
>> > +#define MAX_CSSID 254 /* 255 is reserved */
>> > +#define MAX_CHPID 255
>> > +
>> > +#define MAX_CIWS 8
>> > +
>> > +struct senseid {
>>
>> SenseID
>
> OK.
>>
>> > + /* common part */
>> > + uint32_t reserved:8; /* always 0x'FF' */
>>
>> The standard syntax calls for 'unsigned' instead of uint32_t for bit
>> fields. But bit fields are not very well defined, it's better to avoid
>> them.
>
> Well, the equivalent Linux structure also looks like that :) But I can
> switch this to a uint8_t/uint16_t structure.
>
>>
>> > + uint32_t cu_type:16; /* control unit type */
>> > + uint32_t cu_model:8; /* control unit model */
>> > + uint32_t dev_type:16; /* device type */
>> > + uint32_t dev_model:8; /* device model */
>> > + uint32_t unused:8; /* padding byte */
>> > + /* extended part */
>> > + uint32_t ciw[MAX_CIWS]; /* variable # of CIWs */
>> > +};
>> > +
>
>> > diff --git a/target-s390x/ioinst.h b/target-s390x/ioinst.h
>> > new file mode 100644
>> > index 0000000..79628b4
>> > --- /dev/null
>> > +++ b/target-s390x/ioinst.h
>> > @@ -0,0 +1,173 @@
>> > +/*
>> > + * S/390 channel I/O instructions
>> > + *
>> > + * Copyright 2012 IBM Corp.
>> > + * Author(s): Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>> > + *
>> > + * 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.
>> > +*/
>> > +
>> > +#ifndef IOINST_S390X_H
>> > +#define IOINST_S390X_H
>> > +
>> > +/*
>> > + * Channel I/O related definitions, as defined in the Principles
>> > + * Of Operation (and taken from the Linux implementation).
>>
>> Is this a copy and if so, is the license of original Linux file also GPLv2+?
>
> It's not a verbatim copy.
But a copy of some sort? Can we use the original instead?
>
>>
>> > + */
>> > +
>> > +/* subchannel status word (command mode only) */
>> > +struct scsw {
>>
>> Please use more descriptive names instead of acronyms, for example SubChStatus.
>
> I'd rather leave these at the well-known scsw, pmcw, etc. names. These
> have been around for decades, and somebody familiar with channel I/O
> will instantly know what a struct scsw is, but will need to look hard
> at the code to figure out the meaning of SubChStatus.
If they are well-known and have been around for so long time, are
there any suitable header files (with compatible licenses) where they
are defined which could be reused?
Otherwise, please follow CODING_STYLE.
>
>>
>> > + uint32_t key:4;
>> > + uint32_t sctl:1;
>> > + uint32_t eswf:1;
>> > + uint32_t cc:2;
>> > + uint32_t fmt:1;
>> > + uint32_t pfch:1;
>> > + uint32_t isic:1;
>> > + uint32_t alcc:1;
>> > + uint32_t ssi:1;
>> > + uint32_t zcc:1;
>> > + uint32_t ectl:1;
>> > + uint32_t pno:1;
>> > + uint32_t res:1;
>> > + uint32_t fctl:3;
>> > + uint32_t actl:7;
>> > + uint32_t stctl:5;
>> > + uint32_t cpa;
>> > + uint32_t dstat:8;
>> > + uint32_t cstat:8;
>> > + uint32_t count:16;
>> > +};
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
