Re: [Qemu-devel] [PATCH -V3 01/32] vitio-9p: Add a virtio 9p device to qemu

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 03/25/2010 11:43 AM, Aneesh Kumar K.V wrote:
> From: Anthony Liguori<aliguori@us.ibm.com>
>
> This patch doesn't implement the 9p protocol handling
> code. It add a simple device which dump the protocl data
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
> ---
>   Makefile.target      |    1 +
>   hw/virtio-9p-debug.c |  442 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio-9p.c       |  275 +++++++++++++++++++++++++++++++
>   hw/virtio-9p.h       |   70 ++++++++
>   hw/virtio-pci.c      |   25 +++
>   hw/virtio.h          |    1 +
>   6 files changed, 814 insertions(+), 0 deletions(-)
>   create mode 100644 hw/virtio-9p-debug.c
>   create mode 100644 hw/virtio-9p.c
>   create mode 100644 hw/virtio-9p.h
>
> diff --git a/Makefile.target b/Makefile.target
> index eb4d010..178ddce 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -166,6 +166,7 @@ obj-y += qemu-timer.o
>   # virtio has to be here due to weird dependency between PCI and virtio-net.
>   # need to fix this properly
>   obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
> +obj-y += virtio-9p.o virtio-9p-debug.o
>   obj-y += rwhandler.o
>   obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>   LIBS+=-lz
> diff --git a/hw/virtio-9p-debug.c b/hw/virtio-9p-debug.c
> new file mode 100644
> index 0000000..9230659
> --- /dev/null
> +++ b/hw/virtio-9p-debug.c
> @@ -0,0 +1,442 @@
> +/*
> + * Virtio 9p PDU debug
> + *
> + * Copyright IBM, Corp. 2010
> + *
> + * Authors:
> + *  Anthony Liguori<aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +#include "virtio.h"
> +#include "pc.h"
> +#include "virtio-9p.h"
> +
> +#include<assert.h>
> +#include<sys/uio.h>
>    

Can't directly include sys/uio here (doesn't exist on windows).  
qemu-common.h will do the right thing.

> +
> +#define BUG_ON(cond) assert(!(cond))
> +
> +extern int dotu;
> +static FILE *llogfile;
> +
> +static struct iovec *get_sg(V9fsPDU *pdu, int rx)
> +{
> +    if (rx)
> +        return pdu->elem.in_sg;
>    

Coding style.

> +    return pdu->elem.out_sg;
> +}
> +
> +static void pprint_int8(V9fsPDU *pdu, int rx, size_t *offsetp,
> +                        const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int8_t value;
> +
> +    BUG_ON((offset + sizeof(value))>  sg[0].iov_len);
> +
> +    memcpy(&value, sg[0].iov_base + offset, sizeof(value));
> +    offset += sizeof(value);
> +
> +    fprintf(llogfile, "%s=0x%x", name, value);
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_int16(V9fsPDU *pdu, int rx, size_t *offsetp,
> +                        const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int16_t value;
> +
> +    BUG_ON((offset + sizeof(value))>  sg[0].iov_len);
> +
> +    memcpy(&value, sg[0].iov_base + offset, sizeof(value));
> +    offset += sizeof(value);
> +
> +    fprintf(llogfile, "%s=0x%x", name, value);
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_int32(V9fsPDU *pdu, int rx, size_t *offsetp,
> +                        const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int32_t value;
> +
> +    BUG_ON((offset + sizeof(value))>  sg[0].iov_len);
> +
> +    memcpy(&value, sg[0].iov_base + offset, sizeof(value));
> +    offset += sizeof(value);
> +
> +    fprintf(llogfile, "%s=0x%x", name, value);
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_int64(V9fsPDU *pdu, int rx, size_t *offsetp,
> +                        const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int64_t value;
> +
> +    BUG_ON((offset + sizeof(value))>  sg[0].iov_len);
> +
> +    memcpy(&value, sg[0].iov_base + offset, sizeof(value));
> +    offset += sizeof(value);
> +
> +    fprintf(llogfile, "%s=0x%" PRIx64, name, value);
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_str(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int16_t size;
> +    size_t result;
> +
> +    BUG_ON((offset + 2)>  sg[0].iov_len);
> +    memcpy(&size, sg[0].iov_base + offset, 2);
> +    offset += 2;
> +
> +    BUG_ON((offset + size)>  sg[0].iov_len);
> +    fprintf(llogfile, "%s=", name);
> +    result = fwrite(sg[0].iov_base + offset, 1, size, llogfile);
> +    BUG_ON(result != size);
> +    offset += size;
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_qid(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    fprintf(llogfile, "%s={", name);
> +    pprint_int8(pdu, rx, offsetp, "type");
> +    pprint_int32(pdu, rx, offsetp, ", version");
> +    pprint_int64(pdu, rx, offsetp, ", path");
> +    fprintf(llogfile, "}");
> +}
> +
> +static void pprint_stat(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    fprintf(llogfile, "%s={", name);
> +    pprint_int16(pdu, rx, offsetp, "size");
> +    pprint_int16(pdu, rx, offsetp, ", type");
> +    pprint_int32(pdu, rx, offsetp, ", dev");
> +    pprint_qid(pdu, rx, offsetp, ", qid");
> +    pprint_int32(pdu, rx, offsetp, ", mode");
> +    pprint_int32(pdu, rx, offsetp, ", atime");
> +    pprint_int32(pdu, rx, offsetp, ", mtime");
> +    pprint_int64(pdu, rx, offsetp, ", length");
> +    pprint_str(pdu, rx, offsetp, ", name");
> +    pprint_str(pdu, rx, offsetp, ", uid");
> +    pprint_str(pdu, rx, offsetp, ", gid");
> +    pprint_str(pdu, rx, offsetp, ", muid");
> +    if (dotu) {
> +        pprint_str(pdu, rx, offsetp, ", extension");
> +        pprint_int32(pdu, rx, offsetp, ", uid");
> +        pprint_int32(pdu, rx, offsetp, ", gid");
> +        pprint_int32(pdu, rx, offsetp, ", muid");
> +    }
> +    fprintf(llogfile, "}");
> +}
> +
> +static void pprint_strs(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int16_t count, i;
> +
> +    fprintf(llogfile, "%s={", name);
> +
> +    BUG_ON((offset + 2)>  sg[0].iov_len);
> +    memcpy(&count, sg[0].iov_base + offset, 2);
> +    offset += 2;
> +
> +    for (i = 0; i<  count; i++) {
> +        char str[512];
> +        if (i)
> +            fprintf(llogfile, ", ");
> +        snprintf(str, sizeof(str), "[%d]", i);
> +        pprint_str(pdu, rx,&offset, str);
> +    }
> +
> +    fprintf(llogfile, "}");
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_qids(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    int16_t count, i;
> +
> +    fprintf(llogfile, "%s={", name);
> +
> +    BUG_ON((offset + 2)>  sg[0].iov_len);
> +    memcpy(&count, sg[0].iov_base + offset, 2);
> +    offset += 2;
> +
> +    for (i = 0; i<  count; i++) {
> +        char str[512];
> +        if (i)
> +            fprintf(llogfile, ", ");
> +        snprintf(str, sizeof(str), "[%d]", i);
> +        pprint_qid(pdu, rx,&offset, str);
> +    }
> +
> +    fprintf(llogfile, "}");
> +
> +    *offsetp = offset;
> +}
> +
> +static void pprint_sg(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    unsigned int count;
> +    int i;
> +
> +    if (rx)
> +        count = pdu->elem.in_num;
> +    else
> +        count = pdu->elem.out_num;
> +
> +    fprintf(llogfile, "%s={", name);
> +    for (i = 0; i<  count; i++) {
> +        if (i)
> +            fprintf(llogfile, ", ");
> +        fprintf(llogfile, "(%p, 0x%zx)", sg[i].iov_base, sg[i].iov_len);
> +    }
> +    fprintf(llogfile, "}");
> +}
> +
> +/* FIXME: read from a directory fid returns serialized stat_t's */
> +#ifdef DEBUG_DATA
> +static void pprint_data(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> +{
> +    struct iovec *sg = get_sg(pdu, rx);
> +    size_t offset = *offsetp;
> +    unsigned int count;
> +    int32_t size;
> +    int total, i, j;
> +    ssize_t len;
> +
> +    if (rx)
> +        count = pdu->elem.in_num;
> +    else
> +        count = pdu->elem.out_num;
>    

Coding style.diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> new file mode 100644
> index 0000000..115c93b
> --- /dev/null
> +++ b/hw/virtio-9p.c
> @@ -0,0 +1,275 @@
> +/*
> + * Virtio 9p backend
> + *
> + * Copyright IBM, Corp. 2010
> + *
> + * Authors:
> + *  Anthony Liguori<aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "virtio.h"
> +#include "pc.h"
> +#include "qemu_socket.h"
> +#include "virtio-9p.h"
> +
> +#include<assert.h>
> +
> +/* from Linux's linux/virtio_9p.h */
> +
> +/* The ID for virtio console */
> +#define VIRTIO_ID_9P	9
> +/* Maximum number of virtio channels per partition (1 for now) */
> +#define MAX_9P_CHAN	1
> +
> +#define MAX_REQ		128
> +
> +#define BUG_ON(cond) assert(!(cond))
> +
> +typedef struct V9fsFidState V9fsFidState;
> +
> +typedef struct V9fsString
> +{
> +    int16_t size;
> +    char *data;
> +} V9fsString;
> +
> +typedef struct V9fsQID
> +{
> +    int8_t type;
> +    int32_t version;
> +    int64_t path;
> +} V9fsQID;
> +
> +typedef struct V9fsStat
> +{
> +    int16_t size;
> +    int16_t type;
> +    int32_t dev;
> +    V9fsQID qid;
> +    int32_t mode;
> +    int32_t atime;
> +    int32_t mtime;
> +    int64_t length;
> +    V9fsString name;
> +    V9fsString uid;
> +    V9fsString gid;
> +    V9fsString muid;
> +    /* 9p2000.u */
> +    V9fsString extension;
> +    int32_t n_uid;
> +    int32_t n_gid;
> +    int32_t n_muid;
> +} V9fsStat;
> +
> +struct V9fsFidState
> +{
> +    int32_t fid;
> +    V9fsString path;
> +    int fd;
> +    DIR *dir;
> +    uid_t uid;
> +    V9fsFidState *next;
> +};
> +
> +typedef struct V9fsState
> +{
> +    VirtIODevice vdev;
> +    VirtQueue *vq;
> +    V9fsPDU pdus[MAX_REQ];
> +    V9fsPDU *free_pdu;
> +    V9fsFidState *fid_list;
> +    char *root;
> +    uid_t uid;
> +} V9fsState;
> +
> +int dotu = 1;
> +int debug_9p_pdu = 1;
>    

This shouldn't be enabled by default.

> +
> +extern void pprint_pdu(V9fsPDU *pdu);
> +
> +static V9fsPDU *alloc_pdu(V9fsState *s)
> +{
> +    V9fsPDU *pdu = NULL;
> +
> +    if (s->free_pdu) {
> +        pdu = s->free_pdu;
> +        s->free_pdu = pdu->next;
> +    }
> +
> +    return pdu;
> +}
> +
> +static void free_pdu(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (pdu) {
> +        pdu->next = s->free_pdu;
> +        s->free_pdu = pdu;
> +    }
> +}
> +
> +static void v9fs_version(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_attach(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_stat(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_walk(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_clunk(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +    static void v9fs_open(V9fsState *s, V9fsPDU *pdu)
>    

Whitespace is off.

> +{    if (debug_9p_pdu)
> +    pprint_pdu(pdu);
> +}
> +
> +static void v9fs_read(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_write(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_create(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_flush(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_remove(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
> +
> +static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
> +{
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +}
>    

Coding style is consistently off.

> +
> +typedef void (pdu_handler_t)(V9fsState *s, V9fsPDU *pdu);
> +
> +static pdu_handler_t *pdu_handlers[] = {
> +    [P9_TVERSION] = v9fs_version,
> +    [P9_TATTACH] = v9fs_attach,
> +    [P9_TSTAT] = v9fs_stat,
> +    [P9_TWALK] = v9fs_walk,
> +    [P9_TCLUNK] = v9fs_clunk,
> +    [P9_TOPEN] = v9fs_open,
> +    [P9_TREAD] = v9fs_read,
> +#if 0
> +    [P9_TAUTH] = v9fs_auth,
> +#endif
> +    [P9_TFLUSH] = v9fs_flush,
> +    [P9_TCREATE] = v9fs_create,
> +    [P9_TWRITE] = v9fs_write,
> +    [P9_TWSTAT] = v9fs_wstat,
> +    [P9_TREMOVE] = v9fs_remove,
> +};
> +
> +static void submit_pdu(V9fsState *s, V9fsPDU *pdu)
> +{
> +    pdu_handler_t *handler;
> +
> +    if (debug_9p_pdu)
> +        pprint_pdu(pdu);
> +
> +    BUG_ON(pdu->id>= ARRAY_SIZE(pdu_handlers));
> +
> +    handler = pdu_handlers[pdu->id];
> +    BUG_ON(handler == NULL);
> +
> +    handler(s, pdu);
> +}
> +
> +static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    V9fsState *s = (V9fsState *)vdev;
> +    V9fsPDU *pdu;
> +    ssize_t len;
> +
> +    while ((pdu = alloc_pdu(s))&&
> +            (len = virtqueue_pop(vq,&pdu->elem)) != 0) {
> +        uint8_t *ptr;
> +
> +        BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> +        BUG_ON(pdu->elem.out_sg[0].iov_len<  7);
>    

We shouldn't assume that everything is in the first sg element.diff 
--git a/hw/virtio-9p.h b/hw/virtio-9p.h
>   static PCIDeviceInfo virtio_info[] = {
>       {
>           .qdev.name = "virtio-blk-pci",
> @@ -606,6 +622,15 @@ static PCIDeviceInfo virtio_info[] = {
>           },
>           .qdev.reset = virtio_pci_reset,
>       },{
> +        .qdev.name = "virtio-9p-pci",
> +        .qdev.size = sizeof(VirtIOPCIProxy),
> +        .init      = virtio_9p_init_pci,
> +        .qdev.props = (Property[]) {
> +            DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> +            DEFINE_PROP_STRING("share_path", VirtIOPCIProxy, share_path),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
>    

It's odd to introduce -fsdev after this and have a transient qdev property.

Regards,

Anthony Liguori

> +    },{
>           /* end of list */
>       }
>   };
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 3baa2a3..4032a96 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -174,6 +174,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
>   VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf);
>   VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
>   VirtIODevice *virtio_balloon_init(DeviceState *dev);
> +VirtIODevice *virtio_9p_init(DeviceState *dev, const char *path);
>
>   void virtio_net_exit(VirtIODevice *vdev);
>
>