Re: [Qemu-devel] [PATCH v2 27/27] 9pfs: disentangle V9fsState

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Wei Liu <wei.liu2@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.liu2@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. 

>      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 


>
> @@ -205,16 +210,12 @@ struct V9fsFidState
>

-aneesh