From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz1Sq-0003sx-VU for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:18:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz1Sp-0001WE-EC for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:18:00 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:37968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz1Sp-0001W0-3S for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:17:59 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Aug 2012 09:17:57 +0100 Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by d06nrmr1806.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q788HsLg2384030 for ; Wed, 8 Aug 2012 09:17:54 +0100 Received: from d06av09.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q788HrX3029649 for ; Wed, 8 Aug 2012 02:17:54 -0600 Date: Wed, 8 Aug 2012 10:17:50 +0200 From: Cornelia Huck Message-ID: <20120808101750.35800579@BR9GNB5Z> In-Reply-To: References: <1344351173-2716-1-git-send-email-cornelia.huck@de.ibm.com> <1344351173-2716-3-git-send-email-cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/5] s390: Virtual channel subsystem support. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: linux-s390 , Anthony Liguori , Marcelo Tosatti , KVM , Carsten Otte , Heiko Carstens , Rusty Russell , Sebastian Ott , qemu-devel , Alexander Graf , Christian Borntraeger , Avi Kivity , Martin Schwidefsky On Tue, 7 Aug 2012 21:00:59 +0000 Blue Swirl 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 > > + * > > + * 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 > > +#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? > > > + 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 > > + * > > + * 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 > > + * > > + * 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. > > > + */ > > + > > +/* 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. > > > + 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; > > +};