On Wed, Aug 8, 2012 at 8:28 AM, Cornelia Huck <cornelia.huck@xxxxxxxxxx> wrote:
> On Tue, 7 Aug 2012 20:47:22 +0000
> Blue Swirl <blauwirbel@xxxxxxxxx> 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 <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 <hw/hw.h>
>> > +#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.
OK. Then please use CamelCase for these too.
>
>>
>
>> > + 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;
>> > +}
>> > +
>
--
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
