Re: [Minios-devel] [UNIKRAFT PATCH 2/2] plat/kvm: Introducing virtio base driver

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

Please find some of the comments inline:


On 06/27/2018 12:59 PM, Costin Lupu wrote:
> From: Costin Lupu <costin.lup@xxxxxxxxx>
>
> Currently, the virtio base driver contains the implementation only for
> virtio rings. The implementation was ported from Solo5 and adapted to
> Unikraft APIs. Subsequent virtio drivers should depend on this base
> driver.
>
> This patch also introduces the plat/drivers/ directory which should
> contain the drivers implementations which may be used by more than a
> single platform.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   plat/Makefile.uk                             |   1 +
>   plat/common/include/pci/virtio/virtio_pci.h  |  66 +++++++
>   plat/common/include/pci/virtio/virtio_ring.h | 257 +++++++++++++++++++++++++++
>   plat/drivers/virtio/virtio_ring.c            | 153 ++++++++++++++++
>   plat/kvm/Config.uk                           |   8 +
>   plat/kvm/Makefile.uk                         |   8 +
>   6 files changed, 493 insertions(+)
>   create mode 100644 plat/common/include/pci/virtio/virtio_pci.h
>   create mode 100644 plat/common/include/pci/virtio/virtio_ring.h
>   create mode 100644 plat/drivers/virtio/virtio_ring.c
>
> diff --git a/plat/Makefile.uk b/plat/Makefile.uk
> index 6ff632c..72155bd 100644
> --- a/plat/Makefile.uk
> +++ b/plat/Makefile.uk
> @@ -1,5 +1,6 @@
>   UK_PLAT_BASE        := $(CONFIG_UK_BASE)/plat
>   UK_PLAT_COMMON_BASE := $(UK_PLAT_BASE)/common
> +UK_PLAT_DRIVERS_BASE:= $(UK_PLAT_BASE)/drivers
>   
>   $(eval $(call _import_lib,$(UK_PLAT_BASE)/xen))
>   $(eval $(call _import_lib,$(UK_PLAT_BASE)/kvm))
> diff --git a/plat/common/include/pci/virtio/virtio_pci.h 
> b/plat/common/include/pci/virtio/virtio_pci.h
> new file mode 100644
> index 0000000..02bad4c
> --- /dev/null
> +++ b/plat/common/include/pci/virtio/virtio_pci.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: ISC */
> +/*
> + * Authors: Dan Williams
> + *          Costin Lupu <costin.lupu@xxxxxxxxx>
> + *
> + * Copyright (c) 2015, IBM
> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation
> + *
> + * Permission to use, copy, modify, and/or distribute this software
> + * for any purpose with or without fee is hereby granted, provided
> + * that the above copyright notice and this permission notice appear
> + * in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
> + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
> + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +/* Taken and adapted from solo5 virtio_pci.h */
> +
> +#ifndef __PLAT_CMN_PCI_VIRTIO_PCI_H__
> +#define __PLAT_CMN_PCI_VIRTIO_PCI_H__
> +
> +#include <uk/arch/limits.h>
> +
Do  we protect (MARCO definition) for the legacy mode of virtio vs 
nonlegacy virtio mode? As the current layout support for legacy mode.
> +/* virtio config space layout */
> +#define VIRTIO_PCI_HOST_FEATURES        0    /* 32-bit r/o */
> +#define VIRTIO_PCI_GUEST_FEATURES       4    /* 32-bit r/w */
> +#define VIRTIO_PCI_QUEUE_PFN            8    /* 32-bit r/w */
> +#define VIRTIO_PCI_QUEUE_SIZE           12   /* 16-bit r/o */
> +#define VIRTIO_PCI_QUEUE_SEL            14   /* 16-bit r/w */
> +#define VIRTIO_PCI_QUEUE_NOTIFY         16   /* 16-bit r/w */
> +
> +/*
> + * Shift size used for writing physical queue address to QUEUE_PFN
> + */
The spec explicitly mentions the size of the page size to be 4096 in 
section 4.1.5.1.5.4.1.
Should we not hard code it to 12 instead of combining it with 
__PAGE_SHIFT as there is also virtio-PCI which is driving this 
configuration?
> +#define VIRTIO_PCI_QUEUE_ADDR_SHIFT     __PAGE_SHIFT
> +
> +
> +/*
> + * The status register lets us tell the device where we are in
> + * initialization
> + */
> +#define VIRTIO_PCI_STATUS               18   /* 8-bit r/w */
> +#define VIRTIO_PCI_STATUS_ACK           0x1  /* we recognize device as virtio 
> */
> +#define VIRTIO_PCI_STATUS_DRIVER        0x2  /* we want to drive it */
> +#define VIRTIO_PCI_STATUS_DRIVER_OK     0x4  /* initialization is complete */
Missing definition for the device reset or device error flag (0x40). Do 
we need to care for the device status?
> +#define VIRTIO_PCI_STATUS_FAIL          0x80 /* tell device something's wrong 
> */
> +
> +/*
> + * Reading the value will return the current contents of the interrupt
> + * status register and will also clear it.  This is effectively a
> + * read-and-acknowledge.
> + */
> +#define VIRTIO_PCI_ISR                  19   /* 8-bit r/o */
> +#define VIRTIO_PCI_ISR_HAS_INTR         0x1  /* interrupt is for this device */
> +#define VIRTIO_PCI_ISR_CONFIG           0x2  /* config change bit */
> +
Do we also make assumption on MSI more explicitly?
> +/* xxx assuming msi is not configured */
> +#define VIRTIO_PCI_CONFIG_OFF           20
> +
> +#endif /* __PLAT_CMN_PCI_VIRTIO_PCI_H__ */
> diff --git a/plat/common/include/pci/virtio/virtio_ring.h 
> b/plat/common/include/pci/virtio/virtio_ring.h
> new file mode 100644
> index 0000000..549ae54
> --- /dev/null
> +++ b/plat/common/include/pci/virtio/virtio_ring.h
> @@ -0,0 +1,257 @@
As a general comment, we should use the virtio_ring.h from the official 
specification. It is available in the linux kernel repo 
include/uapi/linux/virtio_ring.h under BSD License.
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
> + *
> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +/* An interface for efficient virtio implementation.
> + *
> + * This header is BSD licensed so anyone can use the definitions
> + * to implement compatible drivers/servers.
> + *
> + * Copyright 2007, 2009, IBM Corporation
> + * Copyright 2011, Red Hat, Inc
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef __PLAT_CMN_PCI_VIRTIO_RING_H__
> +#define __PLAT_CMN_PCI_VIRTIO_RING_H__
> +
> +#include <uk/arch/limits.h>
> +#include <uk/alloc.h>
> +
> +/* This marks a buffer as continuing via the next field. */
> +#define VIRTQ_DESC_F_NEXT       1
> +/* This marks a buffer as write-only (otherwise read-only). */
> +#define VIRTQ_DESC_F_WRITE      2
> +/* This means the buffer contains a list of buffer descriptors. */
> +#define VIRTQ_DESC_F_INDIRECT   4
> +
> +/*
> + * The device uses this in used->flags to advise the driver:
> + * don't kick me when you add a buffer. It's unreliable, so
> + * it's simply an optimization.
> + */
> +#define VIRTQ_USED_F_NO_NOTIFY  1
> +
> +/*
> + * The driver uses this in avail->flags to advise the device:
> + * don't interrupt me when you consume a buffer. It's unreliable, so
> + * it's simply an optimization.
> + */
> +#define VIRTQ_AVAIL_F_NO_INTERRUPT      1
> +
> +/* Support for indirect descriptors */
> +#define VIRTIO_F_INDIRECT_DESC    28
> +
> +/* Support for avail_event and used_event fields */
> +#define VIRTIO_F_EVENT_IDX        29
> +
> +/* Arbitrary descriptor layouts. */
> +#define VIRTIO_F_ANY_LAYOUT       27
> +
> +
> +/*
> + * Virtqueue descriptors: 16 bytes.
> + * These can chain together via "next".
> + */
It is better to explicitly use virtio based datatype instead of generic 
u64, u32, u16 because in the specification it is mentioned that the data 
type to be little endian/guest endianess for different layout. We should 
be careful on the use of these data types.
It may also be useful to provide conversion functions for guest to host 
endianess.
> +struct virtq_desc {
> +       /* Address (guest-physical). */
> +       __u64 addr;
> +       /* Length. */
> +       __u32 len;
> +       /* The flags as indicated above. */
> +       __u16 flags;
> +       /* We chain unused descriptors via this, too */
> +       __u16 next;
> +};
> +
> +struct virtq_avail {
> +       __u16 flags;
> +       __u16 idx;
> +       __u16 ring[];
> +       /* Only if VIRTIO_F_EVENT_IDX: __u16 used_event; */
> +};
> +
> +/* __u32 is used here for ids for padding reasons. */
> +struct virtq_used_elem {
> +       /* Index of start of used descriptor chain. */
> +       __u32 id;
> +       /* Total length of the descriptor chain which was written to. */
> +       __u32 len;
> +};
> +
> +struct virtq_used {
> +       __u16 flags;
> +       volatile __u16 idx;
> +       struct virtq_used_elem ring[];
> +       /* Only if VIRTIO_F_EVENT_IDX: __u16 avail_event; */
> +};
> +
Why did we choose 1526 as the length? Is it based on the size of the 
Ethernet frame size of 1514 and 12 for the meta data? If yes, should we 
make this choice based on this use case?
> +/* This is the max buffer length per descriptor. */
> +#define MAX_BUFFER_LEN 1526
> +
> +/*
> + * Each one of these io_buffer's map to a descriptor.
> + * An array of io_buffer's of size virtq->num (same
> + * as virtq->desc) is allocated during init.
> + */
> +struct io_buffer {
> +       __u8 data[MAX_BUFFER_LEN];
> +
> +       /*
> +        * Data length in bytes. It is written by the driver on
> +        * a tx/write, or by the device on a rx/read on interrupt
> +        * handling (do not remove the volatile).
> +        */
> +       volatile __u32 len;
> +
> +       /* Extra flags to be added to the corresponding descriptor. */
> +       __u16 extra_flags;
> +};
> +
We should also consider packing the structure as we want a specific 
layout of memory allocation. As mentioned in the spec [1].
> +struct virtq {
> +       unsigned int num;
> +
> +       struct virtq_desc *desc;
> +       struct virtq_avail *avail;
> +       struct virtq_used *used;
> +       struct io_buffer *bufs;
> +
> +       /* Keep track of available (free) descriptors */
> +       __u16 num_avail;
> +
> +       /* Indexes in the descriptors array */
> +       __u16 last_used;
> +       __u16 next_avail;
> +};
> +
> +/*
> + * Helper macros for accessing virtqueue fields
> + */
> +
> +#define VIRTQ_OFF_DESC(q)         0
> +
> +#define VIRTQ_OFF_AVAIL(q)        ((q)->num * sizeof(struct virtq_desc))
> +#define VIRTQ_OFF_AVAIL_RING(q) \
> +       (VIRTQ_OFF_AVAIL(q) + sizeof(struct virtq_avail))
> +
> +#define VIRTQ_OFF_PADDING(q) \
> +       (VIRTQ_OFF_AVAIL_RING(q) + (sizeof(__u16) * (q)->num))
> +
> +#define VIRTQ_OFF_USED(q) \
> +       ((VIRTQ_OFF_PADDING(q) + __PAGE_SIZE - 1) & __PAGE_MASK)
> +#define VIRTQ_OFF_USED_RING(q) \
> +       (VIRTQ_OFF_USED(q) + sizeof(struct virtq_used))
> +
> +#define VIRTQ_SIZE(q) \
> +       (VIRTQ_OFF_USED_RING(q) + (sizeof(struct virtq_used_elem) * (q)->num))
> +
> +static inline
> +int virtq_need_event(__u16 event_idx, __u16 new_idx, __u16 old_idx)
> +{
> +       return (new_idx - event_idx - 1) < (new_idx - old_idx);
> +}
> +
> +/* Get location of event indices (only with VIRTIO_F_EVENT_IDX) */
> +static inline __u16 *virtq_used_event(struct virtq *vq)
> +{
> +       /*
> +        * For backwards compatibility, used event index
> +        * is at *end* of avail ring.
> +        */
> +       return &vq->avail->ring[vq->num];
> +}
> +
> +static inline __u16 *virtq_avail_event(struct virtq *vq)
> +{
> +       /*
> +        * For backwards compatibility, avail event index
> +        * is at *end* of used ring.
> +        */
> +       return (__u16 *) &vq->used->ring[vq->num];
> +}
> +
> +/*
> + * Create a descriptor chain starting at index head,
> + * using vq->bufs also starting at index head.
> + * @param vq Virtual queue
> + * @param head Starting descriptor index
> + * @param num Number of descriptors (and number of bufs).
> + * @return 0 on success, < 0 otherwise
> + */
> +int virtq_add_descriptor_chain(struct virtq *vq,
> +               __u16 head, __u16 num);
> +
> +/*
> + * Initializes a virtual queue
> + * @param vq Virtual queue
> + * @param pci_base Base in PCI configuration space
> + * @param selector Virtual queue selector
> + * @param a Memory allocator
> + * @return 0 on success, < 0 otherwise
> + */
> +int virtq_rings_init(struct virtq *vq, __u16 pci_base, int selector,
> +               struct uk_alloc *a);
> +
> +/*
> + * Deinitializes a virtual queue
> + * @param vq Virtual queue
> + * @param pci_base Base in PCI configuration space
> + * @param selector Virtual queue selector
> + * @param a Memory allocator
> + */
> +void virtq_rings_fini(struct virtq *vq, __u16 pci_base, int selector,
> +               struct uk_alloc *a);
> +
> +#endif /* __PLAT_CMN_PCI_VIRTIO_RING_H__ */
> diff --git a/plat/drivers/virtio/virtio_ring.c 
> b/plat/drivers/virtio/virtio_ring.c
> new file mode 100644
> index 0000000..c82cc86
> --- /dev/null
> +++ b/plat/drivers/virtio/virtio_ring.c
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: ISC */
> +/*
> + * Authors: Dan Williams
> + *          Martin Lucina
> + *          Ricardo Koller
> + *          Costin Lupu <costin.lupu@xxxxxxxxx>
> + *
> + * Copyright (c) 2015-2017 IBM
> + * Copyright (c) 2016-2017 Docker, Inc.
> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation
> + *
> + * Permission to use, copy, modify, and/or distribute this software
> + * for any purpose with or without fee is hereby granted, provided
> + * that the above copyright notice and this permission notice appear
> + * in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> + * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
> + * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
> + * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
> + * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +/* Taken and adapted from solo5 virtio_ring.c */
> +
> +#include <string.h>
> +#include <uk/print.h>
> +#include <cpu.h>
> +#include <pci/virtio/virtio_pci.h>
> +#include <pci/virtio/virtio_ring.h>
> +
 In section 3.2.1, the specification mention a max size of 32768.

> +/*
> + * There is no official max queue size. But we've seen 4096,
> + * so let's use the double of that.
> + */
> +#define VIRTQ_MAX_QUEUE_SIZE  (2 * __PAGE_SIZE)
> +
> +
> +/*
> + * Create a descriptor chain starting at index head, using vq->bufs
> + * also starting at index head.
> + * Make sure the vq-bufs are cleaned before using them again.
> + */
> +int virtq_add_descriptor_chain(struct virtq *vq, __u16 head, __u16 num)
> +{
> +       struct virtq_desc *desc;
> +       __u16 used_descs, mask, i;
> +
> +       UK_ASSERT(vq != NULL);
> +       UK_ASSERT(num > 0);
> +
> +       if (vq->num_avail < num) {
> +               uk_printd(DLVL_WARN, "virtq full! next_avail:%d last_used:%d\n",
> +                               vq->next_avail, vq->last_used);
Should it not be -ENOMEM or -ENOSPC?

> +               return -EINVAL;
> +       }
> +
> +       used_descs = num;
> +       mask = vq->num - 1;
> +
> +       for (i = head; used_descs > 0; used_descs--) {
> +               struct io_buffer *buf = &vq->bufs[i];
> +
> +               /*
> +                * The first field of a "struct io_buffer" is the "data" field,
> +                * so in the interrupt handler we can just cast this pointer
> +                * back into a 'struct io_buffer'.
> +                */
> +               UK_ASSERT(buf->data == (__u8 *) buf);
> +               UK_ASSERT(buf->len <= MAX_BUFFER_LEN);
Do we assert here or report an error to an upper layer? ASSERT might be 
skipped in the optimized code?
> +
> +               desc = &vq->desc[i];
> +               desc->addr = (__u64) buf->data;
> +               desc->len = buf->len;
> +               desc->flags = VIRTQ_DESC_F_NEXT | buf->extra_flags;
> +
> +               i = (i + 1) & mask;
> +               desc->next = i;
> +       }
> +
> +       /* The last descriptor in the chain does not have a next */
> +       desc->next = 0;
> +       desc->flags &= ~VIRTQ_DESC_F_NEXT;
> +
> +       vq->num_avail -= num;
I think we need a write memory barrier before the next statement as we 
transferring the buffer to the virtio device which could run on a SMP 
host. What do you think?
> +       /* Memory barriers should be unnecessary with one processor */
> +       vq->avail->ring[vq->avail->idx & mask] = head;
> +       /* avail->idx always increments and wraps naturally at 65536 */
> +       vq->avail->idx++;
> +       vq->next_avail += num;
> +
> +       return 0;
> +}
> +
> +int virtq_rings_init(struct virtq *vq, __u16 pci_base, int selector,
> +               struct uk_alloc *a)
> +{
> +       __u8 *data;
> +       __sz vq_size;
> +
> +       UK_ASSERT(vq != NULL);
> +       UK_ASSERT(a != NULL);
> +
> +       vq->last_used = vq->next_avail = 0;
> +
> +       /* read queue size */
> +       outw(pci_base + VIRTIO_PCI_QUEUE_SEL, selector);
> +       vq->num = vq->num_avail = inw(pci_base + VIRTIO_PCI_QUEUE_SIZE);
Check for zero on the num, as it is a check if the queue exist or not.

For better alignment, it may be wise to check for a power of two.
> +
> +       UK_ASSERT(vq->num <= VIRTQ_MAX_QUEUE_SIZE);
The specification provides a cleaner implementation for calculating the 
size of the queue. Personally, I would use that implementation as it 
improve on readability.
> +
> +       vq_size = VIRTQ_SIZE(vq);
Initialize data variable as the memalign return error without modifying 
the data variable.
> +
> +       /* allocate queue memory */
> +       uk_posix_memalign_ifpages(a, (void **) &data, __PAGE_SIZE, vq_size);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       memset(data, 0, vq_size);
> +
> +       vq->desc = (struct virtq_desc *) (data + VIRTQ_OFF_DESC(vq));
> +       vq->avail = (struct virtq_avail *) (data + VIRTQ_OFF_AVAIL(vq));
> +       vq->used = (struct virtq_used *) (data + VIRTQ_OFF_USED(vq));
> +
> +       /* set queue memory */
> +       outw(pci_base + VIRTIO_PCI_QUEUE_SEL, selector);
The specification mentions about providing the physical address of the 
guest OS. We are currently assigning the address return from malloc, 
which will not work always.
> +       outl(pci_base + VIRTIO_PCI_QUEUE_PFN,
> +                       (__u64) data >> VIRTIO_PCI_QUEUE_ADDR_SHIFT);
> +
> +       return 0;
> +}
> +
> +void virtq_rings_fini(struct virtq *vq, __u16 pci_base, int selector,
> +               struct uk_alloc *a)
> +{
> +       __u8 *data;
> +
> +       UK_ASSERT(vq != NULL);
> +       UK_ASSERT(a != NULL);
> +
> +       /* reset queue memory */
> +       outw(pci_base + VIRTIO_PCI_QUEUE_SEL, selector);
> +       outl(pci_base + VIRTIO_PCI_QUEUE_PFN, 0);
> +
> +       /* free queue memory */
> +       data = (__u8 *) vq->desc - VIRTQ_OFF_DESC(vq->num);
> +       uk_free(a, data);
> +
> +       /* cleanup the queue */
> +       memset(vq, 0, sizeof(*vq));
> +}
> diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
> index 967a07b..893de74 100644
> --- a/plat/kvm/Config.uk
> +++ b/plat/kvm/Config.uk
> @@ -17,4 +17,12 @@ config KVM_PCI
>          select LIBUKBUS
>          help
>                   PCI bus driver for probing and operating PCI devices
> +
> +if (KVM_PCI)
> +config KVM_PCI_VIRTIO
> +       bool "Virtio Drivers"
> +       default n
> +       help
> +                Virtio drivers for KVM
> +endif
>   endif
> diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> index c1e79a2..1ae1b08 100644
> --- a/plat/kvm/Makefile.uk
> +++ b/plat/kvm/Makefile.uk
> @@ -8,6 +8,7 @@ $(eval $(call addplat_s,kvm,$(CONFIG_PLAT_KVM)))
>   ##
>   $(eval $(call addplatlib,kvm,libkvmplat))
>   $(eval $(call addplatlib_s,kvm,libkvmpci,$(CONFIG_KVM_PCI)))
> +$(eval $(call addplatlib_s,kvm,libkvmpcivirtio,$(CONFIG_KVM_PCI_VIRTIO)))
>   
>   ##
>   ## Platform library definitions
> @@ -46,3 +47,10 @@ LIBKVMPLAT_SRCS-y              += 
> $(UK_PLAT_COMMON_BASE)/memory.c|common
>   LIBKVMPCI_ASINCLUDES-$(CONFIG_ARCH_X86_64)  += 
> -I$(UK_PLAT_COMMON_BASE)/include
>   LIBKVMPCI_CINCLUDES-$(CONFIG_ARCH_X86_64)   += 
> -I$(UK_PLAT_COMMON_BASE)/include
>   LIBKVMPCI_SRCS-y                     += 
> $(UK_PLAT_COMMON_BASE)/pci_bus.c|common
> +
> +##
> +## Virtio library definitions
> +##
> +LIBKVMPCIVIRTIO_ASINCLUDES-y   += -I$(UK_PLAT_COMMON_BASE)/include
> +LIBKVMPCIVIRTIO_CINCLUDES-y    += -I$(UK_PLAT_COMMON_BASE)/include
> +LIBKVMPCIVIRTIO_SRCS-y         += $(UK_PLAT_DRIVERS_BASE)/virtio/virtio_ring.c
We may also need to append to export symbols for the api functions as 
virtio_ring expose function to be used by other libraries.
Reference
[1] http://docs.oasis-open.org/virtio/virtio/v1.0/cs01/virtio-v1.0-cs01.pdf

Thanks & Regards
Sharan Santhanam

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel