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

[Jens Freimann]

On Wed, Sep 19, 2012 at 06:12:55PM +0200, Alexander Graf wrote:
> Just a really quick glimpse. This patch is huge :).
> 
> On 04.09.2012, at 17:13, Cornelia Huck wrote:
> 
> > Add a new virtio transport that uses channel commands to perform
> > virtio operations.
> > 
> > Add a new machine type s390-ccw that uses this virtio-ccw transport
> > and make it the default machine for s390.
> > 
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> > 
> > Changes v1->v2:
> > - update to virtio-ccw interface changes
> > 
> > ---
> > hw/qdev-monitor.c      |   5 +
> > hw/s390-virtio.c       | 277 ++++++++++++----
> > hw/s390x/Makefile.objs |   1 +
> > hw/s390x/css.c         |  45 +++
> > hw/s390x/css.h         |   3 +
> > hw/s390x/virtio-ccw.c  | 875 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/s390x/virtio-ccw.h  |  79 +++++
> > vl.c                   |   1 +
> > 8 files changed, 1215 insertions(+), 71 deletions(-)
> > create mode 100644 hw/s390x/virtio-ccw.c
> > create mode 100644 hw/s390x/virtio-ccw.h
> > 
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index 33b7f79..92b7c59 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -42,6 +42,11 @@ static const QDevAlias qdev_alias_table[] = {
> >     { "virtio-blk-s390", "virtio-blk", QEMU_ARCH_S390X },
> >     { "virtio-net-s390", "virtio-net", QEMU_ARCH_S390X },
> >     { "virtio-serial-s390", "virtio-serial", QEMU_ARCH_S390X },
> > +    { "virtio-blk-ccw", "virtio-blk", QEMU_ARCH_S390X },
> > +    { "virtio-net-ccw", "virtio-net", QEMU_ARCH_S390X },
> > +    { "virtio-serial-ccw", "virtio-serial", QEMU_ARCH_S390X },
> > +    { "virtio-balloon-ccw", "virtio-balloon", QEMU_ARCH_S390X },
> > +    { "virtio-scsi-ccw", "virtio-scsi", QEMU_ARCH_S390X },
> 
> How does this work? We want to default to virtio-xxx-pci on !s390, to 
> virtio-xxx-s390 on the legacy machine and to virtio-xxx-ccw for the s390-ccw 
> machine, right?

See patch 5. As you said, we need to find a nice way to solve this. 

> > [...]  
> > +
> > +static void virtio_ccw_notify(void *opaque, uint16_t vector)
> > +{
> > +    VirtioCcwData *dev = opaque;
> > +    SubchDev *sch = dev->sch;
> > +    uint64_t indicators;
> > +
> > +    if (vector >= VIRTIO_PCI_QUEUE_MAX) {
> > +        return;
> > +    }
> > +
> > +    qemu_mutex_lock(&sch->mutex);
> 
> Why do you need this lock?

The locks are probably a leftover from an earlier version where Conny used
a separate thread for subchannel work. We will double check if the lock would
be needed when the big QEMU lock is gone and, if yes, leave a comment in the 
code 
as a reminder.

Jens