qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/5] s390: Add new channel I/O based virtio tran


From: Blue Swirl
Subject: Re: [Qemu-devel] [PATCH 3/5] s390: Add new channel I/O based virtio transport.
Date: Wed, 8 Aug 2012 19:03:14 +0000

On Wed, Aug 8, 2012 at 8:28 AM, Cornelia Huck <address@hidden> wrote:
> On Tue, 7 Aug 2012 20:47:22 +0000
> Blue Swirl <address@hidden> 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 <address@hidden>
>> > + *
>> > + * 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;
>> > +}
>> > +
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]