From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45520) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz1d4-0006O6-Id for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:28:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz1cy-0004Dm-Af for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:28:34 -0400 Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:39115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz1cx-0004Dg-UX for qemu-devel@nongnu.org; Wed, 08 Aug 2012 04:28:28 -0400 Received: from /spool/local by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Aug 2012 09:28:27 +0100 Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by d06nrmr1707.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q788SNPT2711744 for ; Wed, 8 Aug 2012 09:28:23 +0100 Received: from d06av08.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q788SMkM027468 for ; Wed, 8 Aug 2012 02:28:23 -0600 Date: Wed, 8 Aug 2012 10:28:20 +0200 From: Cornelia Huck Message-ID: <20120808102820.799d3d9a@BR9GNB5Z> In-Reply-To: References: <1344351173-2716-1-git-send-email-cornelia.huck@de.ibm.com> <1344351173-2716-4-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 3/5] s390: Add new channel I/O based virtio transport. 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 20:47:22 +0000 Blue Swirl wrote: > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > > new file mode 100644 > > index 0000000..8a90c3a > > --- /dev/null > > +++ b/hw/s390x/virtio-ccw.c > > @@ -0,0 +1,962 @@ > > +/* > > + * virtio ccw target implementation > > + * > > + * 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 > > +#include "block.h" > > +#include "blockdev.h" > > +#include "sysemu.h" > > +#include "net.h" > > +#include "monitor.h" > > +#include "qemu-thread.h" > > +#include "../virtio.h" > > +#include "../virtio-serial.h" > > +#include "../virtio-net.h" > > +#include "../sysbus.h" > > "hw/virtio..." for the above OK. > > > +#include "bitops.h" > > + > > +#include "ioinst.h" > > +#include "css.h" > > +#include "virtio-ccw.h" > > + > > +static const TypeInfo virtio_ccw_bus_info = { > > + .name = TYPE_VIRTIO_CCW_BUS, > > + .parent = TYPE_BUS, > > + .instance_size = sizeof(VirtioCcwBus), > > +}; > > + > > +static const VirtIOBindings virtio_ccw_bindings; > > + > > +typedef struct sch_entry { > > + SubchDev *sch; > > + QLIST_ENTRY(sch_entry) entry; > > +} sch_entry; > > SubchEntry, see CODING_STYLE. Also other struct and typedef names below. > > > + > > +QLIST_HEAD(subch_list, sch_entry); > > static, but please put this to a structure that is passed around instead. > > > + > > +typedef struct devno_entry { > > + uint16_t devno; > > + QLIST_ENTRY(devno_entry) entry; > > +} devno_entry; > > + > > +QLIST_HEAD(devno_list, devno_entry); > > Ditto > > > + > > +struct subch_set { > > + struct subch_list *s_list[256]; > > + struct devno_list *d_list[256]; > > +}; > > + > > +struct css_set { > > + struct subch_set *set[MAX_SSID + 1]; > > +}; > > + > > +static struct css_set *channel_subsys[MAX_CSSID + 1]; OK, will try to come up with some kind of structure for this and CamelCasify it. > > + > > +VirtIODevice *virtio_ccw_get_vdev(SubchDev *sch) > > +{ > > + VirtIODevice *vdev = NULL; > > + > > + if (sch->driver_data) { > > + vdev = ((VirtioCcwData *)sch->driver_data)->vdev; > > + } > > + return vdev; > > +} > > + > > +VirtioCcwBus *virtio_ccw_bus_init(void) > > +{ > > + VirtioCcwBus *bus; > > + BusState *_bus; > > Please avoid identifiers with leading underscores. OK. > > > + DeviceState *dev; > > + > > + css_set_subch_cb(virtio_ccw_find_subch); > > + > > + /* Create bridge device */ > > + dev = qdev_create(NULL, "virtio-ccw-bridge"); > > + qdev_init_nofail(dev); > > + > > + /* Create bus on bridge device */ > > + _bus = qbus_create(TYPE_VIRTIO_CCW_BUS, dev, "virtio-ccw"); > > + bus = DO_UPCAST(VirtioCcwBus, bus, _bus); > > + > > + /* Enable hotplugging */ > > + _bus->allow_hotplug = 1; > > + > > + return bus; > > +} > > + > > +struct vq_info_block { > > + uint64_t queue; > > + uint16_t num; > > +} QEMU_PACKED; > > + > > +struct vq_config_block { > > + uint16_t index; > > + uint16_t num; > > +} QEMU_PACKED; > > Aren't these KVM structures? They should be defined in a KVM header > file file in linux-headers. Not really, virtio-ccw isn't tied to kvm. I see this more as command blocks that are specific to the "control unit" - like something that would be defined in an attachment specification for a classic s390 device (and in the virtio spec in this case) and modeled as C structures here. > > > + case CCW_CMD_WRITE_CONF: > > + if (check_len) { > > + if (ccw->count > data->vdev->config_len) { > > + ret = -EINVAL; > > + break; > > + } > > + } > > + len = MIN(ccw->count, data->vdev->config_len); > > + config = qemu_get_ram_ptr(ccw->cda); > > Please use cpu_physical_memory_read() (or DMA versions) instead of > this + memcpy(). Will check. > > > + if (!config) { > > + ret = -EFAULT; > > + } else { > > + memcpy(data->vdev->config, config, len); > > + if (data->vdev->set_config) { > > + data->vdev->set_config(data->vdev, data->vdev->config); > > + } > > + sch->curr_status.scsw.count = ccw->count - len; > > + ret = 0; > > + } > > + break; > > + case CCW_CMD_READ_VQ_CONF: > > + if (check_len) { > > + if (ccw->count != sizeof(vq_config)) { > > + ret = -EINVAL; > > + break; > > + } > > + } else if (ccw->count < sizeof(vq_config)) { > > + /* Can't execute command. */ > > + ret = -EINVAL; > > + break; > > + } > > + if (!qemu_get_ram_ptr(ccw->cda)) { > > + ret = -EFAULT; > > + } else { > > + vq_config.index = lduw_phys(ccw->cda); > > lduw_{b,l}e_phys() > > > + vq_config.num = virtio_queue_get_num(data->vdev, vq_config.index); > > + stw_phys(ccw->cda + sizeof(vq_config.index), vq_config.num); > > stw_{b,l]e_phys(), likewise elsewhere. Will check as well. > > > + sch->curr_status.scsw.count = ccw->count - sizeof(vq_config); > > + ret = 0; > > + } > > + break; > > + default: > > + ret = -EOPNOTSUPP; > > + break; > > + } > > + return ret; > > +} > > +