On Fri, Jan 08, 2016 at 11:39:37AM +0530, Aneesh Kumar K.V wrote:
> Wei Liu <wei.l...@citrix.com> writes:
>
> > V9fsState now only contains generic fields. Introduce V9fsVirtioState
> > for virtio transport. Change virtio-pci and virtio-ccw to use
> > V9fsVirtioState. Handle transport enumeration in generic routines.
> >
>
> Few comments below
>
>
> > Signed-off-by: Wei Liu <wei.l...@citrix.com>
> > ---
> > hw/9pfs/9p.c | 41 ++++++++++++++++++-----
> > hw/9pfs/9p.h | 19 +++++++----
> > hw/9pfs/virtio-9p-device.c | 82
> > ++++++++++++++++++++++++++++------------------
> > hw/9pfs/virtio-9p.h | 12 ++++++-
> > hw/s390x/virtio-ccw.h | 2 +-
> > hw/virtio/virtio-pci.h | 2 +-
> > 6 files changed, 109 insertions(+), 49 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 6858b21..2cf8580 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -45,7 +45,13 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const
> > char *fmt, ...)
> > va_list ap;
> >
> > va_start(ap, fmt);
> > - ret = virtio_pdu_vmarshal(pdu, offset, fmt, ap);
> > + switch (pdu->transport) {
> > + case VIRTIO:
> > + ret = virtio_pdu_vmarshal(pdu, offset, fmt, ap);
> > + break;
> > + default:
> > + ret = -1;
> > + }
> > va_end(ap);
> >
>
>
> All that switch(pdu->transport) can go in the next series along with Xen
> support. It is not really needed now and when we complete Xen transport
> we will pull that.
>
No problem.
> > return ret;
> > @@ -57,7 +63,13 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const
> > char *fmt, ...)
> > va_list ap;
> >
> > va_start(ap, fmt);
> > - ret = virtio_pdu_vunmarshal(pdu, offset, fmt, ap);
> > + switch (pdu->transport) {
> > + case VIRTIO:
> > + ret = virtio_pdu_vunmarshal(pdu, offset, fmt, ap);
> > + break;
> > + default:
> > + ret = -1;
> > + }
> > va_end(ap);
> >
> > return ret;
> > @@ -65,7 +77,11 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const
> > char *fmt, ...)
> >
> > static void pdu_push_and_notify(V9fsPDU *pdu)
> > {
> > - virtio_9p_push_and_notify(pdu);
> > + switch (pdu->transport) {
> > + case VIRTIO:
> > + virtio_9p_push_and_notify(pdu);
> > + break;
> > + }
> > }
> >
> > static int omode_to_uflags(int8_t mode)
> > @@ -1696,7 +1712,11 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector
> > *qiov, V9fsPDU *pdu,
> > struct iovec *iov;
> > unsigned int niov;
> >
> > - virtio_init_iov_from_pdu(pdu, &iov, &niov, is_write);
> > + switch (pdu->transport) {
> > + case VIRTIO:
> > + virtio_init_iov_from_pdu(pdu, &iov, &niov, is_write);
> > + break;
> > + }
> >
> > qemu_iovec_init_external(&elem, iov, niov);
> > qemu_iovec_init(qiov, niov);
> > @@ -3272,8 +3292,10 @@ void pdu_submit(V9fsPDU *pdu)
> > }
> >
> > /* Returns 0 on success, 1 on failure. */
> > -int v9fs_device_realize_common(V9fsState *s, Error **errp)
> > +int v9fs_device_realize_common(V9fsState *s, enum p9_transport transport,
> > + Error **errp)
> > {
> > + V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > int i, len;
> > struct stat stat;
> > FsDriverEntry *fse;
> > @@ -3284,8 +3306,10 @@ int v9fs_device_realize_common(V9fsState *s, Error
> > **errp)
> > QLIST_INIT(&s->free_list);
> > QLIST_INIT(&s->active_list);
> > for (i = 0; i < (MAX_REQ - 1); i++) {
> > - QLIST_INSERT_HEAD(&s->free_list, &s->pdus[i], next);
> > - s->pdus[i].s = s;
> > + QLIST_INSERT_HEAD(&s->free_list, &v->pdus[i], next);
> > + v->pdus[i].s = s;
> > + v->pdus[i].idx = i;
> > + v->pdus[i].transport = transport;
> > }
> >
> > v9fs_path_init(&path);
> > @@ -3360,7 +3384,8 @@ out:
> > return rc;
> > }
> >
> > -void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
> > +void v9fs_device_unrealize_common(V9fsState *s, enum p9_transport
> > transport,
> > + Error **errp)
> > {
> > g_free(s->ctx.fs_root);
> > g_free(s->tag);
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 3fe4da4..bd8588d 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -14,6 +14,10 @@
> > #include "qemu/thread.h"
> > #include "qemu/coroutine.h"
> >
> > +enum p9_transport {
> > + VIRTIO = 0x1,
> > +};
> > +
> > enum {
> > P9_TLERROR = 6,
> > P9_RLERROR,
> > @@ -131,9 +135,10 @@ struct V9fsPDU
> > uint8_t id;
> > uint8_t cancelled;
> > CoQueue complete;
> > - VirtQueueElement elem;
> > struct V9fsState *s;
> > QLIST_ENTRY(V9fsPDU) next;
> > + uint32_t idx; /* index inside the array */
> > + enum p9_transport transport;
> > };
> >
>
>
> Can you do this change as a separate patch ? ie, Make V9fsPDU
> independent of virtio . Also introduce V9fsVirtioState
>
No problem.
Wei.
>
> >
> > @@ -205,16 +210,12 @@ struct V9fsFidState
> >
>
> -aneesh
>
